Perl project needs review!

[perl_]

I'm new to perl and was wondering if I can get some help. I have a project and I got this code to work...was wondering if anyone can take a look at it for me.

the project calls for these items

- get user input,
- accept command line arguments,
- read and parse a data file,
- use lists, arrays or hashes,
- clean, parse and organize the information in a usable format,
- write the data to a file,
- use control structures and loops,
- include subroutines,
- use Regular Expressions and advanced string functions.

#!/bin/perl -w

$ARGVCHK = "N" ;
$ARGVUSER = "" ;
$DATE = `date "+%m/%d/%Y %H:%M:%S"` ;
chomp $DATE ;

#loop to check for cmd arg below -r <username>  -a <username>  -l <username>

while (@ARGV) {
if ( $ARGV[0] eq "-l" ) {
# -l list the users in twdb
        $ARGVCHK = "L";
        shift @ARGV;
}
# -a Adds a user to twdb from /etc/passwd (needs to chk if usr is already in twdb)
elsif ($ARGV[0] eq "-a") {
     $ARGVCHK = "A";
    shift @ARGV ;
     if ( @ARGV) {
$ARGVUSER = $ARGV[0] ;
shift @ARGV;
}
else {
   print "missing username\n";
   exit (1);
  }
}


# -r removes a users from twdb
elsif ($ARGV[0] eq "-r" ) {
        $ARGVCHK = "R" ;
    shift @ ARGV ;
if ( @ARGV ) {
        $ARGVUSER = $ARGV[0] ;
        shift @ARGV ;
}
else {
     print " missing username\n" ;
     exit (1) ;
  }
}
else {
     print "proj.pl [-l -a -r <username>]\n";

     exit (0);
   }
}

print " the arg chk is $ARGVCHK\n";
print "the user is $ARGVUSER\n";

if ( $ARGVCHK eq "R" ) { DELUSER() ; }
if ( $ARGVCHK eq "A" ) {ADDUSER() ; }

sub DELUSER {   # start sub
  $IFEXIST = "N" ;
  open (TW, "project.users" );
  open (LOG, ">>audit.log" );

  while ( $tw = <TW> ) {
    @dbline = split (":" , $tw);
    if ( $dbline[0] eq $ARGVUSER ) {
      $IFEXIST = "Y" ;
    } else {
      push ( @newtw , $tw ) ;
    }
  }

  close TW ;

  if ( $IFEXIST eq "Y" ) {
    open (TW, ">project.users");
    print TW @newtw ;
    print LOG "$DATE deleted $ARGVUSER\n";
  }
  else {
    print STDERR " This user is not in the db \n";
    exit (3);
  }

}


sub ADDUSER {
$process = `ps -u $ARGVUSER | wc -l`;
$process--;
chomp( $diskUsuage = `du -sk ` );
$diskUsuage = substr($diskUsuage, 0, 6);
      print "This is the ADDUSER \n" ;

   open ( TW , "project.users" ) ;
   while  ( $tw = <TW> ) {
     @dbline = split (":" , $tw );
     if ( $dbline[0] eq $ARGVUSER) {
       print STDERR" This user is in the  db \n";
       exit (3);
     }
   }
   close TW ;
  open ( PWFILE , "/etc/passwd" ) ;
  open ( TW, ">>project.users" );
  open ( LOG, ">>audit.log" );
  open ( PD, ">>process.users" );

  while ( $line = <PWFILE> ) {
    @pwl = split ( ":" , $line);
    if ($pwl[0] eq $ARGVUSER ) {
       print TW "$pwl[0]:$pwl[4]:$pwl[3]:$pwl[5]\n" ;
       print LOG "$DATE added $pwl[0]\n";
       print PD "$ARGVUSER:$process:$diskUsuage:$pwl[5]\n";
        }

  }

  close (PWFILE);
  close (TW);
  close (LOG);
}

[dsheroh]

You didn't say what sort of review you wanted or in what level of detail, but here are some initial impressions from a quick read over it:

- If you're in a unix/linux environment (which I assume you are, based on your use of `date ...`), it's generally considered better to start off with "#!/usr/bin/env perl" rather than a hardcoded path to the perl binary. Going through "env" will cause it to use the first perl found in your $PATH, which gives a bit more flexibility and portability.

- Always include "use strict;" and "use warnings;" at the beginning of your Perl programs unless you know exactly what they do and have a clearly-explainable reason why you don't want them to do it. Having them enabled will catch a lot of potential problems for you and help to explain what's going on when things don't go quite the way you expect. (If you do this, don't call the perl binary with -w; "use warnings" does the same thing, but also allows you finer control over the warnings emitted if you end up needing it.)

- My impression from your post is that this is a class project/homework, so you may not be allowed to use external modules, but, if you can use them, consider using DateTime instead of `date ...` and Getopt::Std or Getopt::Long for processing your command-line switches.

- Normal Perl style is to use lower_case_names_with_underscores for variable and sub names. ALLCAPS names are treated as reserved for variables/functions defined by Perl itself. (Some people also use ALL_CAPS_WITH_UNDERSCORES for constant names, but others treat them like normal user-defined variables because Perl doesn't really have "constants" as such.)

- You should pass the user name into ADDUSER and DELUSER instead of treating $ARGVUSER as a global.

- You're opening files in an old way which is generally considered insecure these days (although it isn't actually insecure in this particular case). The current best practice would be to use, e.g.,

open my $log, '>>', 'audit.log' or die "Couldn't open log file: $!";

instead. You would then use $log instead of LOG to access the file.

- "$diskUsuage" is misspelled.

- The '-l' (list users) switch is recognized, but does nothing.

If there's any other help I can offer, feel free to ask for more details on any of these points or for a different type of review.

[Stomme poes]

Awesome, dave!