So you know Perl; but do you know programming?

I never considered myself a talented coder/programmer/scipter but I know enough to build some useful tools that don't break.....often ;)

As with many shops we have scripts that you "don't touch because they work." One in particular is our Unix account creater that was written by a "Perl Guru" who was paid a great deal of money to automate several funtions.

After taking a class, reading a few books, reading stuff from the Net, and writing a few reports. I was asked to add a "feature" to the account creater.

I reviewd the source and I now want to be a consultant. The guy who wrote this "automation" took a rather heavy handed approach.

The following is a piece of the mess. Basically, you are prompted for a username, the passwd file gets loaded to an array, it checks for duplicates, and it looks for an open userid:

system("clear");
print "CREATE ACCOUNT:\n\n";
$username = &ask("Enter the login id of the new user: ");

#
# Read the local passwd file.
#
open (PWD,"$LOCAL_PASSWD")|| die "can't open $LOCAL_PASSWD\n";
while (<PWD>) {
   push(@local_passwd,$_);
}
close (PWD);

#
# Figure out what uids have been used
#
for ($i=0 ; $i<=$#local_passwd && ($_ = $local_passwd[$i]); $i++) {
   ($uname,$pwd,$uid) = split(/:/);

   push(@user_ids,$uid);
}

foreach $i (200..2000) {
   foreach $uid (@user_ids) {
      $uid_ok=1;
      if ($i eq $uid) {
         $uid_ok=0;
         last;
      }
   }

   if ($uid_ok) {
      $g_user_id = $i;
      last;
   }
}


undef @user_ids;

#
# Read the local password
#
open(PWD,"$LOCAL_PASSWD")|| die "can't open the $LOCAL_PASSWD\n";
while (<PWD>) {
   push(@local_passwd,$_);
}
close (PWD);

#
# Check for duplicate names
#
for ($i=0 ; $i<=$#local_passwd && ($_ = $local_passwd[$i]); $i++) {
   ($uname) = split(/:/);

   if ($uname eq $username) {
      print "DUPLICATE NAME: $username: can't continue\n";
      exit(1);
   }
}
undef @local_passwd;
[download]

Like I said, it does the job but in a rather heavy handed way. Guess the guy never heard of hashing.

I suggested a re-write and of course the bosses heart rate went up. So I just got the above example and threw together some code to show a difference.

print $clear_screen;
print "CREATE ACCOUNT:\n\n";
$username = &ask("Enter the login id of the new user: ");
my %account;
my %uid;
#
# Read the local passwd file.
#
open (PWD,"$LOCAL_PASSWD") or die "\n\nUnable to open $LOCAL_PASSWD\n\
+n";
while (<PWD>) {
   chomp;
   my ($accountname, $accountid) = (split/:/)[0,2];
   $account{$accountname} = $accountid;
   $uid{$accountid} = $accountname;
}
close (PWD);

if (exists $account{$username}) {
   die "This account name already exists!\n";
}

#
# Loop through our range and see if we have an available number.
# If a hash entry does not exist, use the missing number.
foreach (200..2000) {
   unless ( exists $uid{$_} ) {
      print "Hey $_ is available!...assigning...\n";
      $g_user_id = $_;
      last;
   }
}
[download]

After seeing the reduction, he ok'd the rewrite.

Powerful computers can be a friend as well as an enemy. Sure the script does the job and it isn't even slow, but was the approach right? I don't think so. In this age of "put it in now, we will fix it later" bad code seems to slip in more and more. I should ask for a bonus for rewriting the "experts" scripts. ;)

All in all, I guess this is an example of how you should have people that understand the language and programming in general to interview consultants.

Edited: 04 July 2002, by footpad
Reason: Added <readmore>, per Consideration.

Back to Meditations