Beefy Boxes and Bandwidth Generously Provided by pair Networks
Just another Perl shrine
 
PerlMonks  

How do you critique another person's code?

by Rhose (Priest)
on Dec 19, 2001 at 22:21 UTC ( #133203=perlmeditation: 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.

Comment on How do you critique another person's code?
Select or Download Code
Re: How do you critique another person's code?
by merlyn (Sage) on Dec 19, 2001 at 22:25 UTC
    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?
    Your company isn't paying you to be polite. If the "style" of the code makes future maintenance and upgrades difficult, that's something the company needs to know.

    But you should make it clear what the purpose of your feedback is, ahead of the actual feedback. Remember that "purpose", "trust", and "respect" all go hand-in-hand: if one is missing or unclear, the rest will erode rapidly. So, make it clear that the purpose of your feedback is to reduce the risk to the company, and then remain true to that purpose. Within that purpose, the means by which the content is spoken matters little, as long as the purpose is clear and respected. Hidden agendas will undermine the process, so you must ensure that the cards are all face up on the table.

    -- Randal L. Schwartz, Perl hacker

      This is excellent advice. I can't emphasize this point strongly enough:

      ensure that the cards are all face up on the table
      This includes your own. Make sure you're clear about your motivations and what you want to achieve. If you're only after improving the code, then your style will not matter. If you have a personal beef with the programmer, it will.

      I agree. Remember this is not a personal attack, only a critiquing of his code. Calmly let him know that the comments are unprofessional and reflect badly on him and the company. Let his manager reprimand him for the poor choice in comments that's his job, concentrate on the code. Remind him that well used comments and understandable variables help bring real understanding as to what the code is doing. If you feel that the code needs to be rewritten show them why.
      From the looks of it he needs mentoring. I am one who likes to give people a chance, but I am not suggesting that you take that job. Does he have potential? Yes, we all do. If they don't already have it suggest a code review process. bmcatt has an excellent out line. This is an excellent site for code review as well.

      Ideas for designing and conducting code reviews at TechRepublic has links to articles and tips from readers. These have some good info, you may have to sign up at the site, it's free. If you have problems let me know and I'll get you the articles.

      Good luck,
      Brad

        Does he have potential? Yes, we all do.
        If by that you mean "Does he have potential to be an expert programmer?", I'm going to disagree with you.

        Over the years, I've had lots of opportunity to watch various people attempt and master programming.

        Some people "get it". Others... just don't.

        Now there's nothing lesser about those that don't. Let's take the inverse as an example. Most people consider me to be an expert programmer. But I can't draw worth beans. It mystifies me. I can't figure out how people know where to put the lines or the colors, or how to transform the 3-D world they are viewing into the 2-D projection of it for the paper.

        So, I don't draw. I'm not braindead. I just don't think that way. Similarly, I've seen that most people don't think in the way that it takes to be an expert programmer. Nothing wrong with them. They just don't have that particular aptitude.

        And I may get flamed on this. Lemme also say that I think as children, we probably can and do pick a path, and maybe only then were all possibilities equally probable. But at some point, we start specializing, and after that, there's a commitment to the wiring of a particular brain. My theory, anyway.

        Now if you meant "has potential to get a bit better", certainly, I can support that. But maybe he oughta take up another line of work. Seriously.

        -- Randal L. Schwartz, Perl hacker

(Ovid) Re: How do you critique another person's code?
by Ovid (Cardinal) on Dec 19, 2001 at 22:47 UTC

    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.

Re: How do you critique another person's code?
by Masem (Monsignor) on Dec 19, 2001 at 23:01 UTC
    Besides keeping a cool head, start with the issues that are going to end up as potental bugs; the file1/file2 array assignments for example, with the lack of definition for cnts. The phone number trick will also possibly be a source of problems later.

    Then refer to the code that is inefficient but where it's obvious the programmer knows a more efficient method. In this case, for example, the dealercode could be generated from the substr function, which he's used previously, with much less code and storage overhead.

    Note that you shouldn't refer much to *style* (specifically the variable naming implied, the comments, or to some extent the HTML inclusion w/o using CGI's HTML) unless it's against the company policy. Style's something that is a personal feel, and issues that deal with style should not have a very large impact on speed or execution of the code.

    Finally, make sure to look for *good* things in the code as well. While code critique implies "negative elements", try to find something that the programmer appears to have done well. Don't try too hard to find something good about it, of course, but if there is something that impresses you, make sure you note that.

    -----------------------------------------------------
    Dr. Michael K. Neylon - mneylon-pm@masemware.com || "You've left the lens cap of your mind on again, Pinky" - The Brain
    "I can see my house from here!"
    It's not what you know, but knowing how to find it if you don't know that's important

Re: How do you critique another person's code?
by bmcatt (Friar) on Dec 19, 2001 at 23:27 UTC
    Quite a while back, I was managing a group of C programmers who'd developed some extraordinarily bad habits, not the least of which was no code reviews. In the course of instituting code reviews, I wound up writing a document explaining (to the uninitiated) some of the whys of doing a code review. With these principles in hand, it was much easier for people to understand why we were doing things differently.

    Excerpts follow (I've not included items which were specific to that project or clearly overly C-related, although there's still a bit of the latter poking through):

    Why Do We Do Code Reviews?

    • To find and eliminate bugs before anyone important sees them

      Every bug which is removed during the code/unit test/integration test phases is one less bug which has a chance of finding its way into the finished product and into a customer's workflow. Code reviews allow others to look at a piece of code and to consider if there are logic errors.

    • To find ways to prevent bugs or make them automatically apparent

      We need to determine ways to either not write bugs or make them jump up and announce themselves when they occur. Our goal is to do the fun stuff of writing more, better, code, not the dull part of spending our time finding bugs.

    • To find and eliminate styles of coding which are bug-prone

      Bugs can be introduced from a variety of sources, including techniques which make it difficult to either track what is happening or to see what is changing. We need to eliminate the stylistic approaches which make this sort of development possible so we increase our chances of not writing buggy code to begin with.

    • To encourage and enforce common coding styles

      If we have common coding styles, it makes it easier for any developer to pick up a piece of code and have a chance at figuring out what it does. This is especially important for the 2:00 AM phone call about a system problem.

    • To allow others to suggest better methods and coding practices

      There is almost always a better algorithm or approach to any problem. By conducting a code review, other developers can provide input to find out if there are better (simpler, easier) ways to do anything. If so, let's implement them.

    • To disseminate methods and coding practices to others for their benefit

      Sometimes we stumble across a truly better way of doing anything. We need to make sure that other developers find out about it so we can spread it through the team and project.

    Various Stylistic Thoughts

    • The function of code is for our customers; the form is for our successors

      Every program which is written has both a function and a form - the what and how of the code. The customer for a program's function is, indeed, the end-user customer. However, the customer for a program's form is the programmer who must, bleary-eyed, debug a production problem at 2:00 in the morning. Make the form clear to even a novice programmer so even a novice can fix it.

    • Compiler warnings are bad

      There are very few warnings which are impossible to make go away. For these warnings, understand them and make a comment about it in the code. Otherwise, create type casts or use parentheses or whatever is necessary to make the warnings go away.

    • Don't try to out-optimize the compiler except in technique

      Clear code is more important than hand-tuned code. Most modern compilers can out-optimize hand-tuned assembly language. Spend your time productively - writing new, clear functionality, not trying to out-think the compiler. It is more important to make the purpose of code clear than it is to come up with the absolutely, positively most efficient approach (but make sure the purpose is clear).

    • Don't write the same code more than once unless you absolutely have to

      If you find yourself writing the same exact code more than once, this is a clear indication that one of the following is true:

      • You have mis-rolled a loop and should rethink it.
      • YOu have a common routine which should be put in a separate function.

    • Make variable use obvious

      Variables should either be completely self-documenting (by grace of their name) or should have a comment explaining their use when they are declared.

    I also handed out a copy of Steve McConnell's Code Complete to everyone, along with the comment that I didn't agree with everything he said, but I could explain where and why I didn't.

Re: How do you critique another person's code?
by runrig (Abbot) on Dec 19, 2001 at 23:30 UTC
    However, how can I politely provide factual feedback without "attacking" the style of programming?

    "Style" IMHO means whether I use 2 or 4 space indentation, whether or not I cuddle my else's, etc. That code is crap, to put it politely (but put it as politely as possible in the code review, as others suggest...).

    Update: Re: Dominus' reply: While the book you mentioned talks about good and bad programming style, I don't think of the code at the top of this thread as being in bad programming style - it's just bad programming, period. Saying that that programming has any style at all would be giving it too much credit, and if someone says that one's programming "has style," well, that's good in and of itself, isn't it?

    When I think about style, I tend to think more along the lines of what's in or out of fashion (think goto, and looking at those example FORTRAN programs in the book you mention (and boy, do those bring back memories...), I think indentation is a valid example of an element of style, though I admit, not an incredibly substantive element, and the book does discuss how to cuddle your if's, then's, and else's {no braces, those must be a modern invention}); anyway, I didn't mean for anyone to take my 'definition' of style so literally. Sorry for the misunderstanding :)

    Another update: Aww heck, now that I think about it, call it whatever you like - bad style, bad habits, bad technique, bad programming, or a bad hair day, its fine by me, and not all that much worth arguing about. (I predict that 'bad hair day' will become computer jargon for writing programs that suck, and although it will generally only refer to a temporary condition, the phrase "He's on a permananent bad hair day" will also take hold, and use of these phrases will proliferate after the publication of the book "Coding and Bad Hair Days Don't Mix" - and there is a 47% probability that it will be a Dilbert book :)

      Says runrig:
      "Style" IMHO means whether I use 2 or 4 space indentation, whether or not I cuddle my else's, etc.
      Well, it didn't always mean that. For example, if you look at Programming Style: Examples and Counterexamples by Brian Kernighan and P.J. Plaugher, you'll find that it's concerned with the sort of issues that Rhose raises, and not with trivial matters like indentation or brace placement.

      I think it's really unfortunate that the programming community's discussions of programming style have been sidetracked into meaningless arguments about indentation, and I'd like to see the useful term 'style' applied to more substantive issues.

      --
      Mark Dominus
      Perl Paraphernalia

        Style is always more than formatting.

        Look at 'Code Complete', a weighty tome all about formatting that covers a lot more ground than just where to put braces and the merits of tabs over spaces.

        Beyond that look at something like the 'Chicago Style Manual for Writers'. It also deals with style and is more than just formatting. It addresses passive sentences vs. active sentences, word order, proper grammar, etc.

        The array slices the unnamed programmer was using are bad Perl grammar, as egregious as "That's something I ain't puttin' up with" in the English language. Let's all say it together: 'Style is NOT formatting.'

        Ok, I'll relinquish the soap box now.
Re: How do you critique another person's code?
by dws (Chancellor) on Dec 19, 2001 at 23:35 UTC
    How can I politely provide factual feedback without "attacking" the style of programming? ... the programmer is pretty young.

    Given what you've shown, this seems to me to be as much a behavioral issue as it is a technical one. It looks like a situation where the older developer needs to sit the younger developer down and explain that behavior has consequences, and that coding (and commenting) practices are a behavior. This kind of talk is best done one-on-one, without managers present.

    Unfortunately, that opportunity has been missed, and it's now "meeting with managers" time. Given that, I would focus on the technical aspects of the problems with the code and their likely downstream consquences. Avoid making it personal, even if baited.

Re: How do you critique another person's code?
by maverick (Curate) on Dec 19, 2001 at 23:36 UTC
    Along with everything else advised here...try to always keep the tone of reviewing the code and not the coder. This can be very tricky..and you'll have to phrase what you say with some thought. Unless the programmer has had his/her code reviewed before and comes into the meeting understanding that this is about helping his/her code be better, s/he is likely to take everything you say as a personal attack. This chunk of code is their creation and thus some programmers are very prideful of that and take great offense to anyone saying something bad about it.

    Unless the other managers are also aware of this (and you may have to prep them), they can inadvertantly torpedo even your most carefully worded phrases.

    For everything you find wrong, have a suggestion for how to improve it. Make the tone of the meeting be more like "we're offering our help to improve your skills and help you be a better programmer" instead of "Here's all the bad things you did, fix them and don't do it again".

    Good luck

    /\/\averick
    perl -l -e "eval pack('h*','072796e6470272f2c5f2c5166756279636b672');"

Re: How do you critique another person's code?
by perrin (Chancellor) on Dec 19, 2001 at 23:37 UTC
    Code reviews are hard. That's because it's really hard to take someone else picking at your code the first few times it happens. I've had a hard time accepting criticisms in code reviews, and I know how important it is to have them and to listen.

    My advice is to think about how you would feel on the other side of the table and act accordingly. Be as nice as you can about it, or the guy will reject everything you say out of spite.

    It's also a good idea for your company to invest some money into training resources for this guy. Maybe a good book about style and maintenance issues, like Code Complete. Maybe let him ride a more competent coder's coattails for a bit, doing maintenance on some solid code. I learned a lot that way.

Re: How do you critique another person's code?
by George_Sherston (Vicar) on Dec 19, 2001 at 23:39 UTC
    Thanks for sharing that in such detail. Painful but entertaining, particularly the comments. And always worthwhile: a lesson in folly is worth two in wisdom as the man said.

    As to tackling the feedback, my only observation wd be be as specific as possible. I think people who do a lot of programming know that abstract, aesthetic issues in code really do matter - they are very good pointers not just to little niggles but to systemic disasters that will happen. But for people who don't think that way, the most persuasive criticisms are, IMHO, ones of the form "This line of code would, under such-and-such circumstances, give rise to this specific bad thing happening".

    Having said that, Draino in the chap's coffee is probably a good fallback plan. Good luck!

    George Sherston
(ichimunki) Re: How do you critique another person's code?
by ichimunki (Priest) on Dec 20, 2001 at 00:15 UTC
    I'm not even going to comment on the crappy code which appears to be deliberate sabotage of any future attempts at maintenance or re-use. The first two comments are completely useless. If it's obvious the naming sucks by looking at it, why comment the fact? Only the third of the four comments you quote is even remotely worthwhile. I could see myself including a note like that to myself in code, except that the fourth comment is flat out abusive.

    Coupled with the obvious and deliberate attempts in the actual code to confound future coders, this points to issues far depper than this individual's apparent lack of programming talent. It's not as though this were some brilliant obfuscation that ran in eight lines of code and as many milliseconds-- this is someone who should not be programming for a living. After all, his/her deliberately foul code was performing so poorly that it was taken out for review.
Re: How do you critique another person's code?
by Juerd (Abbot) on Dec 20, 2001 at 00:17 UTC
    ## 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

    This reminds me of some comment in the MS fdisk source code:

    cmd\fdisk\fdisk.c /* P.S. - To whoever winds up maintaining this, I will */ /* apoligize in advance. I had just learned 'C' when */ /* writing this, so out of ignorance of the finer points*/ /* of the langauge I did a lot of things by brute force.*/ /* Hope this doesn't mess you up too much - MT 5/20/86 */ cmd\fdisk\profile.h #define BEGIN { #define END }

    I've got to admit the Microsoft programmer did a much better job: he apologized and respected the reader of his source code.
    (You might wonder where I got this from - A few months ago, someone gave me a text file with some quotes from the MS DOS 6.0 source code. I can't check if they're genuine, but a google search showed some hits, so I guess it's real.)

    BTW, I like the part where the programmer says that those who don't understand his source should not be programming.

    2;0 juerd@ouranos:~$ perl -e'undef christmas' Segmentation fault 2;139 juerd@ouranos:~$

Re: How do you critique another person's code?
by ellem (Hermit) on Dec 20, 2001 at 00:27 UTC
    I can empathize with the writer of this absolutely bizarre code; but only because I was recently singled out in a meeting as an example of "The Way Not To Do It".
    My bosses specific complaint was that my Perl code was too "boring", "obvious", "clear", "over commented", and "cautious". (I'm a Sys Admin not a programmer by profession.) Anylou...
    My code was displayed on a wall and my boss basically showed everyone why he didn't like this and that. One of the programmers did stick up for me and say that he could go into that code at anytime and work on it, and that was good, but mostly folks just ripped on me.
    It did nothing to my style... I still code the same basic way (although I do REM out use diagnostics ; now.)
    Just be very specific. And definitely make sure to make a point about that commenting style: That junk is fine for a homepage hosted on a personal computer but has no business in the real world.
    --
    lmoran@wtsg.com
    There's more than one way to do it, just don't use my way.
      Ellem, I can hardly believe your boss could do such a thing!? I remember myself at a few code reviews for my perl programs, and even that was a little tense. I later came to terms with the idea of code reviews and saw their impact as to the 'betterment' of my code.

      However, your boss just scolded you right there!

      I think that if your code was indeed "obvious", "clear", "over commented", and (above all) "cautious", then I'd step in and support it. Clearly, these are the qualities that do not hurt one's code but rather make it maintainable and less bug-prone.

      PS: I'm absolutely with you on your commnet about "that junk".

      "There is no system but GNU, and Linux is one of its kernels." -- Confession of Faith
      Do you like your job? Not to be a smart-ass, but this sounds really bad to me. ESPECIALLY other people going along with it. Now the other stuff is up to your boss I guess, but I find the "over commented" part hard to swallow. I mean comments are what allow people to easily look at code without thinking "what was the guy who wrote this trying to do". To me this all sounds like a formula for disaster down the road. I'm not sure I'd be too particular on a boss ripping my code in front of everyone for being too clear either.
Re: How do you critique another person's code?
by hsmyers (Canon) on Dec 20, 2001 at 01:35 UTC

    It occurs to me that at least one thing you might want to do is to determine whether the programmer is 'friend' or 'foe'. All of the advice given so far will still have to take this small fact to account. If this were an ideal world, things might be different, but just the existence of such code suggests that there is a significant problem lurking in your organization. You might also do well to find out what backing this person has, I assume that he or she must have some, how else to explain this sort of mess? Actually the more I think about it, the more I think that this isn't a coding problem, this is an office politics problem. I suppose to be generous, the code and the person involved might be remedied, but such an investment might well cost more than it's worth.

    Tactically speaking, I would assume the role of 'The Fool with Questions'. Essentially, have the programmer explain all of the anomalies you've found, to you, because you—of course, just don't understand. This way, the onus is one the author to either save themselves or not.

    And if things truely get ugly, be prepared and bring copies of your node and the responses to it to the meeting! Just a thought!

    –hsm

    "Never try to teach a pig to sing…it wastes your time and it annoys the pig."
Re: How do you critique another person's code?
by vladb (Vicar) on Dec 20, 2001 at 01:47 UTC
    Horrible piece of code.. who hired the guy to do this program for you?

    From all the posts in this thread, I may give you a sure bet that your company would be better off by hiring one of the capable monks ROFL ;-)


    "There is no system but GNU, and Linux is one of its kernels." -- Confession of Faith
Re: How do you critique another person's code?
by chromatic (Archbishop) on Dec 20, 2001 at 03:01 UTC
    If I were you, I would rewrite the sections in question. I might say something like, "The existing code relies on side effects and undocumented assumptions. It is fragile and could be made more clear. Here is my suggestion for an improvement."

    If you lay them out side by side, it'll be obvious... assuming the other people know sufficient Perl.

    I'd leave the issue of comments alone. Again, assuming the other people have a decent grasp (graps?!) of the English language, they'll likely come to the same conclusion you did without having to be told how to feel about them.

    If you're in a politically charged position, you may be better off leading people to your opinion than presenting it as an opposing position -- especially if the existing code is already in place and doing its job. (For what it's worth, the only problem I have with the substr code is the need to assign to $_. That's useless.)

Re: How do you critique another person's code?
by Biker (Priest) on Dec 20, 2001 at 14:51 UTC

    Just an idea. Why not (during the meeting?) propose the original programmer of that code to sign up at the Monastery? This might show your good intent by searching to improve the final product and also future products from the same programmer.

    "Livet är hårt" sa bonden.
    "Grymt" sa grisen...

Re: How do you critique another person's code?
by arhuman (Vicar) on Dec 20, 2001 at 16:19 UTC
    I think you already had plenty of good advices on this thread,
    however I'd like to add a question and a comment :

    Assuming that your goal was to make this guy code better :
    Why didn't you try to talk to the coder first ?
    You could have made him explain why he choosed to code this way.
    You could even have made him understand why it's bad!

    Don't get me wrong I'm not talking about morale/ethics/good...
    I'm talking about efficiency.

    When communicating with people you'll have to take as the fact that people listen/understand what you say
    depending on the way/the moment/the context you say it.

    Keeping that in mind, I'm pretty sure that he won't integrate your (good) advices during the planned meeting
    which will now probably sound more like a court for him...
    (He'll be the central point of a discussion about what he did wrong in front of judges(managers))
    Usually you can't have efficient discussion with people on defensive...

    It wouldn't have been the same if you had explained it to him, man to man, in front of a beer...
    Just a "I've seen some problems but I talked to the coder and he will correct them soon" would have been fine with your manager, IMHO.

    I'm not saying you did wrong ! I just show you another way to try, next time.

    "Only Bad Coders Code Badly In Perl" (OBC2BIP)
Re: How do you critique another person's code?
by Steve_p (Priest) on Dec 21, 2001 at 01:38 UTC
    Since he's an new programmer, I would suggest going easy. I'm going to assume no one else he works with knows Perl at all, otherwise, they wouldn't be going to the DBA's (I don't know, but you may be at one of those places that blame everything on the DBA's). I guess the best things to suggest are the most basic things such as modularization of code, declaring variables (and using meaningful variable names), encouraging him to use the full power of the CGI module (by the way, if you really need a performance boost, mod_perl may not hurt either), etc.

    Of course, make sure you present it in the proper manner. If you want to do a good job politically, start with the good things in the script. Then, go through the suggestions in a non-threatening manner, suggesting changes rather than starting with "This stinks!"

    Hope this helps.
Re: How do you critique another person's code?
by Dominus (Parson) on Dec 21, 2001 at 22:10 UTC
    This sort of problem is quite common. Here's some code that appeared in comp.lang.perl.misc today:
    if($match) { (($rank = @ranks[0]) && ($percent = "1%")) if $value == 1; (($rank = @ranks[1]) && ($percent = "2%")) if $value == 2; (($rank = @ranks[2]) && ($percent = "3%")) if $value == 3; (($rank = @ranks[3]) && ($percent = "4%")) if $value == 4; (($rank = @ranks[4]) && ($percent = "5%")) if $value == 5; (($rank = @ranks[5]) && ($percent = "6%")) if $value == 6; (($rank = @ranks[5]) && ($percent = "7%")) if $value == 7; (($rank = @ranks[6]) && ($percent = "8%")) if $value == 8; (($rank = @ranks[7]) && ($percent = "9%")) if $value == 9; (($rank = @ranks[8]) && ($percent = "10%")) if $value == 10; } $seniority = "$rank. ($percent)\n";
    This always reminds me of the story of the Cobol programmer who was paid by the line.

    The original article.

    --
    Mark Dominus
    Perl Paraphernalia

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: perlmeditation [id://133203]
Approved by root
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others cooling their heels in the Monastery: (4)
As of 2014-10-25 03:09 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    For retirement, I am banking on:










    Results (141 votes), past polls