My first Perl application! mailsumm.pl

I've just started learning Perl a few days ago, and this is my first program. mailsumm.pl prints out a summary of a Maildir mailbox. It takes 2 arguments: the first is the path to the Maildir, and the second is a comma-separated list of subdirectories under the Maildir you wish to look at. Here's the code:

#! /usr/bin/perl
# mailsumm.pl

use strict;
use warnings;

sub readmail;
sub checkdir;
sub checkmsg;

my $maildir = shift;
my @subdirs = split ",", shift;

unless ($maildir =~ '/$') {
    $maildir = $maildir . '/'
}

for (@subdirs) {
    unless ($_ =~ '/$') {
    $_ = $_ . '/';
    }
    $_ = $maildir . $_
}

readmail @subdirs;

sub checkdir {
    (-e $_[0] and -d $_[0]) or return 1;
    return 0;
}

sub readmsg {
    open FH, $_[0] or die ($! . "\nFile: " . $_[0]);
    my $curline;
    my %curmsg = (Subject => '',
          Sender => '',
          Date => '');
    while ($curline = <FH>) {
    if ($curline =~ /^Subject: /) {
        $curmsg{Subject} = $'; #' (fixes syntax highlighting in emacs)
    } elsif ($curline =~ /^From: /) {
        $curmsg{Sender} = $'; #'
        } elsif ($curline =~ /^Date: /) {
        $curmsg{Date} = $'; #'
    } else {
        next;
    }
    }
    close FH;
    return %curmsg;
}

sub printhash {
    for (0...$#_) {
    if ($_ % 2) {
        print $_[$_] . "\n";
    } else {
        print $_[$_] . ": ";
    }
    }
}

sub readmail {
    for (@_) {
    unless (checkdir $_) {
        print "\nMail under $_\n" . '-'x(11 + length $_) . "\n\n";
        opendir DH, $_ or die ($! . "\nFolder: " . $_);
        while (my $curfile = readdir DH) {
        unless (($curfile eq '.') or ($curfile eq '..')) {
            my %curmsg = readmsg ($_ . $curfile);
            chomp $_ for %curmsg;
            printhash %curmsg;
            print "\n";
        }
        }
        closedir DH;
    }
    }
}
[download]

and some sample output:

gizmoguy@geek:~$ ./mailsumm.pl Maildir/ tmp/

Mail under Maildir/tmp/
-----------------------

Subject: new user mail
Sender: vroom@perlmonks.org
Date: Tue, 17 Aug 2010 21:49:36 -0400
[download]

I'd be very appreciative if anyone could give me some comments on coding style, etc., as I'm just starting out. Hope y'all like it. :) Thanks.

Comment on My first Perl application! mailsumm.pl Select or Download Code

Replies are listed 'Best First'.
Re: My first Perl application! mailsumm.pl by Utilitarian (Vicar) on Aug 19, 2010 at 09:05 UTC
Hi, Well done, useful script that works ;) ...However ;) `$'` is not advised, instead consider the form `if ($curline =~ /^Subject: (.)$/){ $curmsg{Subject} = $1;` [download] Note: That this also removes the need for the chomp statement in readmail, you should really do all relevant processing in one place. I prefer to use lexically scoped variables than `$_` in sub-routines and loops, it means that I can read the program naturally when I come back to it in 6 months.eg: `sub checkdir { my $directory = shift; my $is_directory=(( -e $directory ) && ( -d $directory))?0:1; $return $is_directory; }` [download] OR `sub printhash { my $hash = shift; for my $key (keys %{$hash}) { print "$key : $hash->{$key}\n"; } }` [download] In the checkdir example above I also modified it so that there was only one exit point. This makes life easier when it comes to debugging. In the printhash example above I illustrated the use of the `keys` keyword which returns the keys of a hash as a list Also in the printhash routine you will see that I changed the parameter from a hash to a hash reference, as a general rule this makes life easier since you can pass multiple hashes, arrays and scalars without one swallowing the others. Unlikely in this context, but imagine a file called `>0A1234567.msg`, you should use the three param form of open like so `open (my $file_handle , '<', "$filename");` When you parse through the mail file you should probably terminate the loop when you finish reading the headers ie `last if $curline=~/^$/;` and finally I prefer the `.=` operator rather than`$var = $var . "/"`. Believe me when I say my first Perl program had all these issues and more, this is a very good first program. Updated* as per hexcoder's advice below `print "Good ",qw(night morning afternoon evening)[(localtime)[2]/6]," fellow monks."`	[reply] [d/l] [select]
Re^2: My first Perl application! mailsumm.pl by hexcoder (Curate) on Aug 28, 2010 at 22:02 UTC
The `keys` function was missing in your example in line 3... `sub printhash { my $hash = shift; for my $key (keys %{$hash}) { print "$key : $hash->{$key}\n"; } }` [download]	[reply] [d/l] [select]
Re: My first Perl application! mailsumm.pl by jwkrahn (Abbot) on Aug 19, 2010 at 17:44 UTC
`sub checkdir { (-e $_[0] and -d $_[0]) or return 1; return 0; } ... unless (checkdir $_) {` [download] Why are you using a double negative to test for something instead of just testing for something? Why say "If not not directory" instead of "If directory"? Why are you using stat twice when only once is required? What do you think that `-e` does that `-d` doesn't do? In other words, if `-d` is true then `-e` will always be true and if `-d` is false then it doesn't matter what `-e` is. You should change: `unless (checkdir $_) {` [download] in the readmail sub to: `if (-d $_) {` [download] or simply: `if (-d) {` [download] `sub printhash { for (0...$#_) { if ($_ % 2) { print $_[$_] . "\n"; } else { print $_[$_] . ": "; } } } ... printhash %curmsg;` [download] You are complicating something that is very easy to do with a hash. In readmail change: `printhash %curmsg;` [download] to: `print "$_: $curmsg{$_}\n" for keys %curmsg;` [download] `while ($curline = <FH>) { if ($curline =~ /^Subject: /) { ... } else { next; } }` [download] You have a `next;` at the end of the loop, but the only place the loop could go to now is back to the beginning so that is a bit redundant.	[reply] [d/l] [select]