Evaluation of Simple Pop3 Client

Jamnet has asked for the wisdom of the Perl Monks concerning the following question:

I had posted this program few days back under code section with the hope that the feedback receive would help fresh 'Perl' programmers like me to improve our skills. So I would request all the perl monks out there to guide me (us) on improving this program.

Regards

Jamnet


#!/usr/bin/perl -w

use Net::POP3;

# Connect to pop3 server
my $pop3 = Net::POP3->new("10.10.10.10") || die "Error : Couldn't log 
+on to server";

# Login to pop3 server
my $Num_Message = $pop3->login("dummy", "dummy");

my $Messages = $pop3->list();

my ($MsgId, $MsgDate, $MsgFrom, $MsgTo, $MsgCc, $MsgSub);
my ($MsgAttach, $MsgSize, $MsgHeader, $MsgHeadFlg, $MsgBody);

foreach $MsgNo (keys %$Messages)
{
  my $MsgContent = $pop3->get($MsgNo);
  my $count = 0;
  $MsgHeadFlg = 0;
  $MsgBody = "";
  print "Message No : $MsgNo\n";

  $MsgSize = $pop3->list($MsgNo);
  print "Message Size : $MsgSize Bytes\n";
  
  # Process message data
  while()
  {

    # Exit if last line of mail
    if ($count >= scalar(@$MsgContent))
    {
      print "$MsgBody";
      last;
    }

    # Check if end of mail header
    if (@$MsgContent[$count] =~ /^\n/)
    {
      $MsgHeadFlg = 1;
    }

    # Proceed if message header not processed
    if (not $MsgHeadFlg)
    {

      # Split the line 
      my @LineContent = split /: /, @$MsgContent[$count];

      # Check Header Info
      SWITCH:
      {
        # Get message date
        $LineContent[0] =~ /Date/i && do
                                       {
                                         $MsgDate = $LineContent[1];
                                         print "Date : $MsgDate";
                                         last SWITCH;
                                       };

        # Get message id
        $LineContent[0] =~ /Message-ID/i && do
                                       {
                                         $MsgId = $LineContent[1];
                                         print "Message ID : $MsgId";
                                         last SWITCH;
                                       };

        # Get message from
        $LineContent[0] =~ /From/i && do
                                      {
                                        $MsgFrom = $LineContent[1];
                                        print "From : $MsgFrom";
                                        last SWITCH;
                                      };

        # Get message to
        $LineContent[0] =~ /To/i && do
                                      {
                                        $MsgTo = $LineContent[1];
                                        print "To : $MsgTo";
                                        last SWITCH;
                                      };

        # Get message cc
        $LineContent[0] =~ /Cc/i && do
                                      {
                                        $MsgCc = $LineContent[1];
                                        print "Cc : $MsgCc";
                                        last SWITCH;
                                      };


        # Get message subject
        $LineContent[0] =~ /Subject/i && do
                                      {
                                        $MsgSub = $LineContent[1];
                                        print "Subject : $MsgSub";
                                        last SWITCH;
                                      };
      }
    }
    else
    {
      # Process message body
      $MsgBody .= @$MsgContent[$count];
    }

    $count++;
  }
}

# Disconnect from pop3 server
$pop3->quit();
[download]

Comment on Evaluation of Simple Pop3 Client Download Code

Replies are listed 'Best First'.
Re (tilly) 1: Evaluation of Simple Pop3 Client by tilly (Archbishop) on Mar 24, 2001 at 20:13 UTC
In addition to what Masem said, use strict, and unless you want an array slice, access anonymous arrays like this: `$MsgCount->[$count];` [download] But in fact I would drop the $count altogether. Rather than do an infinite loop in which you increment and test $count (sort of like a hand-rolled C-style for loop) just loop over the array: `foreach my $MsgLine (@$MsgContent) { ... }` [download] which is going to be faster and gives more hints to the person reading the code about what your intent is. This may mean moving actions you now have on the final iteration of the loop outside of it. That is another stylistic change I would approve of. Things in the loop are things you expect to do on each iteration. Things outside are not. Remember that debugging is largely an exercise in guessing where the code does not do what the programmer intended. Therefore subtle (and not so subtle) clues about your intent will help in debugging and maintainance. If you expect to have only 2 elements in a split, then use the third argument to split. I would rethink the scope of some of your variables. If they are private, they should be declared in a tight scope. If they are global, I would prefer to see them declared with vars. If they are per message (which they appear to be) then perhaps a hash would make more sense. And one bug alert. Microsoft a long time ago put into all of their tools the ability to recognize the end of a header by the fact that you have seen all required fields, making the return superfluous for them. About a year ago IIRC they then dropped the return separating header from body from Outlook 2000. The result was an upgrade which worked fine for anyone running Outlook, but would cause all sorts of fun for people using other email programs. The hope, of course, was that people would blame their own email programs for crashing since others got their email and it worked just fine. I won't say it didn't work. But I will say that this is the kind of dirty pool that makes me despise Microsoft. No, I don't appreciate their making other people's lives harder for reasons that I don't agree with. But the fact is that they do, and you need to be aware of it. So I recommend finding someone with Outlook 2000 (I believe that that is the one that mangled the standard) and getting them to send you sample messages so you can analyze their dirty pool and waste time and energy you shouldn't have had to working around their crap...	[reply] [d/l] [select]
Re: Evaluation of Simple Pop3 Client by Masem (Monsignor) on Mar 24, 2001 at 17:18 UTC
One rule of thumb for perl programming improvement is that if you are writing pretty much the same code over and over again in your program, you have the potental to do a lot of improvement in your program. Namely, in your case, you've got the switch block... if you take your header variables and move them into a hash, you can rewrite that as... `if (@$MsgContent[$count] =~ /^(To\|From\|CC\|Date\|Subject\|Message-ID)\s:\s(.)$/ ) { $MsgHash{ $1 } = $2; print "$1 : $2"; }` [download] (The extra \s before the : in the regex may be bad, but I can't recall if the specs allow a space there or not) Dr. Michael K. Neylon - mneylon-pm@masemware.com \|\| "You've left the lens cap of your mind on again, Pinky" - The Brain	[reply] [d/l]
(dkubb) Re: (2) Evaluation of Simple Pop3 Client by dkubb (Deacon) on Mar 25, 2001 at 01:52 UTC
Had your question been regarding "how do I parse HTML using a regular expression?", you would have been bombarded with a chorus of people saying "don't do that, use a module like HTML::Parser or it's derivatives". And in most cases they would be right. =) Why bother to reinvent parsing routines that someone else has done, and thousands of people use, and test, every day? I'm going to give you the same advice for your project, don't parse an email message using a regex. Instead, use some of MailTools's CPAN modules to parse the message headers and body for you. Here's your example re-worked to use the Mail::Internet module: #!/usr/bin/perl -w use strict; use Net::POP3; use Mail::Internet; use constant POP3_SERVER => '10.10.10.10'; use constant USERNAME => 'username'; use constant PASSWORD => 'password'; my $pop3 = Net::POP3->new(POP3_SERVER) or die 'Could not open a POP3 connection'; $pop3->login(USERNAME, PASSWORD) or die 'Could not login'; my $messages = $pop3->list; foreach my $msg_num (keys %$messages) { my $mail = Mail::Internet->new( $pop3->get($msg_num) ); foreach my $header (qw(Date Message-Id From To Cc Subject)) { print "$header: ", $mail->get($header) if defined $mail->get($header); } #Get the message body, and possibly do something with it my $body = join '', @{$mail->body}; } $pop3->quit; [download]	[reply] [d/l]