It's "Peter Writes Bad Perl" time again!

Post #192,061 by pwhysall 1/28/05 1:24:32 AM Reply	It's "Peter Writes Bad Perl" time again! Consider the following little script: \n#!/usr/bin/perl -w\n\nuse strict;\n\nmy %messages;\n\nforeach my $arg(@ARGV)\n{\n open (MESSAGE, $arg) or die "Can't open $arg: $!";\n while (<MESSAGE>)\n {\n chomp;\n if (/X-Spam-Status: Yes, hits=(\\d+.\\d+)/)\n {\n $messages{$arg} = $1;\n }\n }\n close (MESSAGE);\n}\n\nmy @scores = reverse sort values %messages;\nmy %byval = reverse %messages;\n\nprint "$byval{$scores[0]}"; Is there a tidier way of finding the highest scoring message this? I know I don't deal with duplicate scores. It doesn't matter, so I don't bother. Peter [link\|http://www.ubuntulinux.org\|Ubuntu Linux] [link\|http://www.kuro5hin.org\|There is no K5 Cabal] [link\|http://guildenstern.dyndns.org\|Home] Use P2P for legitimate purposes!
Post #192,063 by ben_tilly 1/28/05 2:18:05 AM Reply	Bad Perl indeed! You forgot that sort is by default alphabetical, not numerical. Useless use of chomp detected. If you're going to be only using %byval, just create that hash. Better yet if you just want the one value, do something like this (untested): \n\n#!/usr/bin/perl -w\n\nuse strict;\n\nmy $max_val;\nmy $max_message;\n\nforeach my $arg(@ARGV)\n{\n open (my $msg, $arg) or die "Can't open $arg: $!";\n while (<$msg>)\n {\n if (/X-Spam-Status: Yes, hits=(\\d+.\\d+)/)\n {\n my $val = $1;\n if (not defined($max_val) or $max_val < $val)\n {\n $max_val = $val;\n $max_message = $arg;\n }\n }\n }\n}\n\nif (defined($max_val))\n{\n print "$max_message\\n";\n}\n (I've left your indentation style alone.) If this was in a larger sample I'd fault the excessive indentation. But in this short of code it isn't worth breaking out functions. I'm not entirely sure what the messages are either, if full emails then I'd break out early at the end of the header. That would be faster and avoid possible spurious matches. Cheers, Ben I have come to believe that idealism without discipline is a quick road to disaster, while discipline without idealism is pointless. -- Aaron Ward (my brother)
Post #192,072 by pwhysall 1/28/05 3:38:52 AM Reply	Re: Bad Perl indeed! You forgot that sort is by default alphabetical, not numerical. D'oh! Useless use of chomp detected. Old habit. {snip handy re-write, will test later} If this was in a larger sample I'd fault the excessive indentation. But in this short of code it isn't worth breaking out functions. What's wrong with the indentation? I'm not entirely sure what the messages are either, if full emails then I'd break out early at the end of the header. That would be faster and avoid possible spurious matches. Yes, they're full emails and this is a damn fine idea. Moochos grassy arse for your help. Peter [link\|http://www.ubuntulinux.org\|Ubuntu Linux] [link\|http://www.kuro5hin.org\|There is no K5 Cabal] [link\|http://guildenstern.dyndns.org\|Home] Use P2P for legitimate purposes!
Post #192,126 by ben_tilly 1/28/05 4:14:01 PM Reply	The indentation is too deep. That's a code smell indicating poorly factored complexity. Cheers, Ben I have come to believe that idealism without discipline is a quick road to disaster, while discipline without idealism is pointless. -- Aaron Ward (my brother)
Post #192,129 by drewk 1/28/05 5:08:47 PM Reply	Okay, now I have to post this Sorry about the right scroll, but you need it for this one. \nif (is_array($foonums)) { \n for($i=0;$i<count($foonums);$i++) { \n $foonum = $foonums[$i]; \n if (isByHand($foonum)) { \n $foos[$foonum] = "hand"; \n } else { \n $sql = "SELECT idnum FROM foos_transferred WHERE foonum='".$foonum."'"; \n if ($db->dbQuery($sql)) { \n $foos[$foonum] = "transferred"; \n } else { \n $sql = "SELECT idnum FROM foos_deleted WHERE foonum='".$foonum."'"; \n if ($db->dbQuery($sql)) { \n $foos[$foonum] = "deleted"; \n } else { \n $sql = "SELECT idnum FROM foos_release WHERE foonum='".$foonum."'"; \n if ($db->dbQuery($sql)) { \n $foos[$foonum] = "released"; \n } else if (substr($foonum,0,1)!='9') { \n $foos[$foonum] = "old_system"; \n } else { \n $foos[$foonum] = "alive"; \n } \n } \n } \n } \n } \n} \n \n... \n \n \nif ($conn->dbQuery($query)) { \n $success++; \n logIt('pl_foo/mainlog', $query, $o_User->username); \n#\t\t\techo "<font color='purple' size='2' face='arial'><b>QUERY: ".$query."</b></font><br>"; \n if ($conn2->dbQuery($query2)) { \n $success2++; \n logIt('pl_foo/mainlog', $query2, $o_User->username); \n#\t\t\t\techo "<font color='green' size='2' face='arial'><b>QUERY2: ".$query2."</b></font><br>"; \n if ($conn3->dbQuery($query3)) { \n $success3++; \n logIt('pl_foo/mainlog', $query3, $o_User->username); \n#\t\t\t\t\techo "<font color='magenta' size='2' face='arial'><b>QUERY3: ".$query3."</b></font><br>"; \n } else { \n logIt('pl_foo/mainlog', "FAILURE: ".$query3, $o_User->username); \n } \n } else { \n logIt('pl_foo/mainlog', "FAILURE: ".$query2, $o_User->username); \n } \n // keep going. all is well so far.\n} else {\n logIt('pl_foo/mainlog', "FAILURE: ".$query, $o_User->username);\n echo "There's an error in the loop";\n break;\n}\n\n\n...\n\n\n } else {\n logIt('pl_foo/transferlog', "DID NOT HAPPEN: ".$sql, $o_User->username);\n echo "DID NOT HAPPEN: ".$sql." ";\n }\n } else {\n logIt('pl_foo/transferlog', "FAILURE: ".$sql, $o_User->username);\n echo "FAILURE: ".$sql." ";\n }\n } else {\n logIt('pl_foo/transferlog', "FAILURE: ".$sql, $o_User->username);\n echo "FAILURE: ".$sql." ";\n }\n } else {\n logIt('pl_foo/transferlog', "FAILURE: ".$sql, $o_User->username);\n echo "FAILURE: ".$sql." ";\n }\n } else {\n logIt('pl_foo/transferlog', "FAILURE: ".$sql, $o_User->username);\n echo "FAILURE: ".$sql." ";\n }\n } else {\n logIt('pl_foo/transferlog', "FAILURE: ".$sql, $o_User->username);\n echo "FAILURE: ".$sql." ";\n }\n Things to note: The clsDB->dbQuery method will return -1 on error. PHP interprets 'if( -1 )' as true. The last snippet has 6 different failure modes. Count the number of uniquely identifiable error and/or log messages. Pretty colored debugging messages. Actual linefeeds instead of \\n in strings. Using double quotes instead of single for strings that shouldn't be evaluated, then closing the quotes and concatenating the variables that could be parsed in a double-quoted string. This isn't even the worst code from this guy. === Purveyor of Doc Hope's [link\|http://DocHope.com\|fresh-baked dog biscuits and pet treats]. [link\|http://DocHope.com\|http://DocHope.com]
Post #192,153 by ben_tilly 1/28/05 6:23:15 PM Reply	Ugh I have come to believe that idealism without discipline is a quick road to disaster, while discipline without idealism is pointless. -- Aaron Ward (my brother)
Post #192,079 by broomberg 1/28/05 6:49:17 AM Reply	If you can pipe from STDIN you save a bunch of code and error possibilities. No opens, closes, die on missing file, etc. Cut your code by 2/3s.
Post #192,080 by pwhysall 1/28/05 7:02:01 AM Reply	It's processing a Maildir format directory... ...and therefore I need to identify which file contains the matching line. If I can do this whilst reading from STDIN, I'd be chuffed. Peter [link\|http://www.ubuntulinux.org\|Ubuntu Linux] [link\|http://www.kuro5hin.org\|There is no K5 Cabal] [link\|http://guildenstern.dyndns.org\|Home] Use P2P for legitimate purposes!
Post #192,156 by broomberg 1/28/05 7:07:15 PM Reply	OK, you're chuffed Whatever that means. Don't use a cat \| pipe, use: \n./prog.pl file1 file2 ...\n\nwhile (<>){\n\tprint "Hi, I'm current working on $ARGV\\n";\n}\n
Post #192,191 by drewk 1/29/05 2:15:53 AM Reply	ICLRPD (new thread) Created as new thread #192190 titled [link\|/forums/render/content/show?contentid=192190\|ICLRPD] === Purveyor of Doc Hope's [link\|http://DocHope.com\|fresh-baked dog biscuits and pet treats]. [link\|http://DocHope.com\|http://DocHope.com]

Welcome to IWETHEY!