The stupid question is the question not asked | |
PerlMonks |
Re: popcheck - Simple POP3 mail checkerby Aristotle (Chancellor) |
on Aug 10, 2003 at 16:45 UTC ( [id://282649]=note: print w/replies, xml ) | Need Help?? |
The first thing that strikes me here is.. why are you using printf everywhere? Also, rather than print,exit, you can more idiomatically use die. Also, most of your globals needn't be. Also, instead of lining up two dozen prints, it's better to use a heredoc. Actually, in your case, since you're printing the usage message that way, it would be even better to use Pod::Usage (part of the core since 5.8, IIRC). It plays well with Getopt::Long, too, which allows you to easily handle multiple get and delete options at once. Note that for all of the long option names, you can automatically use a one-letter abbreviation if it is unambiguous, so you lose no brevity over Getopt::Std. As an side, passing passwords on the command line isn't wise. Any other using of the box can ps awx and steal it. The following piece of code is really bizarre. For readability, the condition should just check @ARGV. This is better written like so:
But the use of goto is strange, and the assignment following it will never be executed. The other goto also obscures the flow. There's no reason to use them there. Further down, you check if the number of messages " eq '0E0'", but the whole point of the excercise of returning that strange seeming value is that you get a 0 that is true in boolean context so you can or die during object construction, but then still test if the number is == 0. So I'd replace it with that, because that's what the intent of the code is. Many of your checks consist of something like if(error){print message}else{do stuff}. In Perl, it's much easier to say die "foo" if error; do stuff;. That usually means fewer levels of nesting to keep track of and usually makes the code more straightforward, so you should usually use it. Finally, creating temporary files is a tricky business best left to File::Temp, which has been part of the core forever. Also, please see Two-arg open() considered dangerous. Note, as well, that you can easily use lexical variables in more recent Perls to hold file handles. This is preferrable to polluting your namespace with global filehandle symbols. However, in this case, I'd not even use temporary files. You can spawn a pipe to less and feed the output to it directly, but why even bother? Just print the messages to STDOUT and let the user redirect them where he wants to (works even better if all regular messages (of which there is only one, "no messages" or "x messages on the server") are printed to STDERR). That's what he's got his Unix toolset for, anyway. In fact, this way you're generating bog standard mbox format output which can be fed to the stock mail handling tools. So here's all that, in code. Now all it needs is a way to list the subjects of all mails in your inbox.. Makeshifts last the longest.
In Section
Code Catacombs
|
|