Beefy Boxes and Bandwidth Generously Provided by pair Networks
P is for Practical

comment on

( #3333=superdoc: print w/replies, xml ) Need Help??
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/;
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> ";
(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.
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]; }
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 = $_;
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;
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.

In reply to How do you critique another person's code? by Rhose

Use:  <p> text here (a paragraph) </p>
and:  <code> code here </code>
to format your post; it's "PerlMonks-approved HTML":

  • Posts are HTML formatted. Put <p> </p> tags around your paragraphs. Put <code> </code> tags around your code and data!
  • Titles consisting of a single word are discouraged, and in most cases are disallowed outright.
  • Read Where should I post X? if you're not absolutely sure you're posting in the right place.
  • Please read these before you post! —
  • Posts may use any of the Perl Monks Approved HTML tags:
    a, abbr, b, big, blockquote, br, caption, center, col, colgroup, dd, del, div, dl, dt, em, font, h1, h2, h3, h4, h5, h6, hr, i, ins, li, ol, p, pre, readmore, small, span, spoiler, strike, strong, sub, sup, table, tbody, td, tfoot, th, thead, tr, tt, u, ul, wbr
  • You may need to use entities for some characters, as follows. (Exception: Within code tags, you can put the characters literally.)
            For:     Use:
    & &amp;
    < &lt;
    > &gt;
    [ &#91;
    ] &#93;
  • Link using PerlMonks shortcuts! What shortcuts can I use for linking?
  • See Writeup Formatting Tips and other pages linked from there for more info.
  • Log In?

    What's my password?
    Create A New User
    and the web crawler heard nothing...

    How do I use this? | Other CB clients
    Other Users?
    Others exploiting the Monastery: (7)
    As of 2019-09-16 17:15 GMT
    Find Nodes?
      Voting Booth?
      The room is dark, and your next move is ...

      Results (196 votes). Check out past polls.