(Ovid) Re: How do you critique another person's code?

[Ovid]

Frankly, I'd try to find out if the programmer is still working there (sounds like the programmer is) and I'd have a real row over it. Even if, somehow, someone wrote code this abysmal and it never broke, people who have been around the block a time or two know that requirements change and that means maintenance. If I were a manager, I'd be sorely tempted to fire that programmer (unless this is a first offense).

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];
}

Well, if $cnts is never defined, then, if the above code might very well equal:

if ($file1[0] eq $dealerno)
{
  @file2[0..10] = @file1[0..10];
}

Heck, you might be able to assign one array directly to the other, if they're the same length. Regardless, @file2[$cnts++] is a one element array slice. Rarely, if ever does anyone want that. If the programmer didn't know that, he or she had better learn. This causes bugs, for example, when you're trying to put a function's return value in a one-element array slice and the function uses wantarray. Of course, if the programmer bothered to use warnings, an error message similar to the following would have appeared:

Scalar value @file2[$cnts++] better written as @file2[$cnts++] at -foo
+.pl line 327.

If the programmer is going to call others a moron for not understanding the code, perhaps you can explain, to the programmer's face and in front of his/her manager, that you didn't understand why anyone would write code so awful and you'd like to know if you're a moron.

Sheesh, it disgusts me that people would do stuff like that. I think you have to attack the style of programming and the style of communication. This programmer appears to be (from the code) an arrogant jerk. If you pull punches, it's going to cause problems in the long run. Now, I'm not a Perl God, but if I were dealing with this person, I would want to know exactly where this person gets off calling others a moron when the code is so miserable. Don't get me wrong, I think that most people know that I am extremely patient with newer programmers. I have demonstrated this time and time again. Newer programmers who are arrogant, however, quickly discover that Ovid has a tongue that can clip a hedge.

Cheers,
Ovid

Join the Perlmonks Setiathome Group or just click on the the link and check out our stats.