How do you critique another person's code?

I am a senior DBA (Oracle) for a large medium sized company (large medium = 1 billion a year in sales). My education, however, is in Systems Analysis, and as such, I work pretty closely with our application programmers. This week, I was given some perl code to review. The code is for a CGI script which helps people locate the nearest dealer based on ZIP code or city/state. The complaint was that the code was running slower than expected, and I was asked to review the code to see if I could find any problems.

I could not believe what I found in the script. The logic is very hard to follow... there are no subroutines... files are closed without ever being opened... file opens are never checked for success... variable names mean very little... and then we get to the big stuff. *Smiles*

The first two lines are:

#!/usr/local/bin/perl
use CGI qw/:all/;
[download]

however, all HTML is hard coded:

  print "
  </select>
  </td>
  </tr>

  <tr>
  <td colspan=2 valign=top>
  <font face=Arial size=2 color=#000000>
  <br>
  <b>Search by postal code.</b>
  <br><br>
  </font>
  </td>
  </tr>

  <tr>
  <td valign=top>
  <font face=Arial size=2 color=#000000>
  Postal Code: 
  </font>
  </td>
  <td valign=top>
  <input type=text name=zip size=5>
  </td>
  </tr>
  <tr>
  <td>
  &nbsp;
  </td>
  <td valign=top>
  <br><br>
  <input type=submit name=Search value=Search onClick=\"return validat
+e()\">
  </td>
  </tr>
  </table>
  </font>
  </form>
  </body>
  </html>
  ";
[download]

(Yep, that is actual code...)

The next thing I found were horrid comments... well, let me share my favorite three.

## yes, the naming convention sucks, but hey, you didn't program it, I
+ did

## come on now, I wrote the code, like there would ever be a problem. 
+he he he

## well genius, you probably figured that since they have not entered 
+anything, that we need to ask them for something
## I should not have to explain the following to you cause if I did, y
+ou should not be programming - moron.
[download]

The next thing I found were "cool" tricks. Please note, the variable $cnts is never defined.

if ($file1[0] eq $dealerno)
{
  @file2[$cnts++] = $file1[0];
  @file2[$cnts++] = $file1[1];
  @file2[$cnts++] = $file1[2];
  @file2[$cnts++] = $file1[3];
  @file2[$cnts++] = $file1[4];
  @file2[$cnts++] = $file1[5];
  @file2[$cnts++] = $file1[6];
  @file2[$cnts++] = $file1[7];
  @file2[$cnts++] = $file1[8];
  @file2[$cnts++] = $file1[9];
  @file2[$cnts++] = $file1[10];
}
[download]

I'll have to remember that one... just like the one that uses tr to find the length of a string.

Here is how you format a phone number:

$_ = $phone;
substr($_,3,0) = "/";
substr($_,7,0) = "-";
$phone = $_;
[download]

Here is how you get the last five characters of a string:

$dealer1 = chop ($city);
$dealer2 = chop ($city);
$dealer3 = chop ($city);
$dealer4 = chop ($city);
$dealer5 = chop ($city);
$dealercode = $dealer5 . $dealer4 . $dealer3 . $dealer2 . $dealer1;
[download]

I made a comment about the code to my manager, and have now been called into a meeting with my manager, and the programmer's manager where I am expected to provide feedback on the script and what I think the problems are. If I were the programmer's supervisor, I would have no problem providing him direction (and rejecting the entire script.) However, how can I politely provide factual feedback without "attacking" the style of programming?

This is kind of related to Ovid's recent thread (OT) Old blood versus new blood... the programmer is pretty young.

Back to Meditations