Comebacks for When They Say ‘Your Code Sucks’

It’s tough enough to have people criticize your code, especially when the comments are coming from an intern. On Slashdot, one anonymous user had precisely that gripe. After ten years as a developer, the kid “started ripping into my on how terrible it is,” with complaints that are “simply because he does not have the experience with it to actually understand what the code is doing.” His bottom line question:

He is a smart guy with lots of promise, he is asking good questions, but how do I get him to look past his own self perceived greatness enough to slow down and learn what we are doing and how we have pulled it off?

Seven Hot Programming Languages... Including CobolSlashdot being Slashdot, there was no shortage of suggestions. Of course, there was the tongue in cheek, like: Hire a second, unpaid intern to do the first intern’s work, then have the first intern “do nothing but beautify your existing code, with the understanding that he can’t change how any of it is written.”

Most people, though, suggested that the OP turn the situation into a teachable moment. The user Anonymous Coward suggested presenting the critic with some of his own code,  without identifying the source, and have him critique it. “After they rip into it and query why the code was written that way, reply: ‘You tell me, this is your stuff from a couple years ago.’ ”

Another thought some perspective might be in order. “Junior guys often say things like ‘this code is crap,’ not because they really want to change it, but because they want you to notice that they’re smart,” observed Dintech. “Instead, give the intern greenfield work — plenty of room to prove himself without setting you off.”

PolygamousRanchKid said having the intern read The Psychology of Computer Programming would expose him to the idea of ego-less programming. Crazyjj took the hard-nosed approach: “Tell him to write the log-in page himself – after all, he’s fresh from a CS program where they taught him everything.”

One of the more interesting bits of advice was to tell the intern to “Define Bad.” It came from Sir Garlon, who wrote, “As an engineer, I’ve adopted the maxim that there is no good and bad, only fitness for a particular purpose. I prefer a discussion of trade-offs to statements of principle. I tend to ask ‘what requirements does this code fail to meet?’ And very often, the reviewer has invented his own new requirement.”

“Depending on your process, your response might be anything from ‘good point, let’s add a test case for that,’ to, ‘you should submit a Requirements Change Form for that,” he said.

But what about critics who hammer your code for things that are hard to quantify, like “readability” or “maintainability?” Sir Garlon has thoughts about that, too. Make the critic justify the cost of fixing the code to the boss.

That would be a conversation worth listening in on.

Comments

  1. BY Brian says:

    Perhaps before getting all defensive check to see if they are right and exercise the humility that you expect of others. There are plenty of experienced programmers that write beautifully cryptic code that is clear only to themselves.

    If they are wrong, then explain to them why they are wrong. If they are right, learn something, then teach them about using tact and conducting themselves as a professional.

    Be honest to yourself and to them, or you’ll be doing you both a disservice.

  2. BY Brian says:

    Perhaps before getting all defensive check to see if they are right and exercise the humility that you expect of others. There are plenty of experienced programmers that write beautifully cryptic code that is clear only to themselves.

    If they are wrong, then explain to them why they are wrong. If they are right, learn something, then teach them about using tact and conducting themselves as a professional.

    Be honest to yourself and to them, or you’ll be doing you both a disservice.

  3. BY Brendon says:

    I am an empiricist and I might be a little bit inexperience but could both parties just perform code analytics on their code? Using a set of grading on how well the code is written? Although this doesn’t go into all the intricacies of software design, it might be a starting place.
    Another way of resolving this is maybe to get the internet or person in question to write a report outlining the problems with the code, with references to style guides and best programming practices. This could start an intellectual dialog between the two.
    I am not sure if this is wishful thinking or not but I think this is a nice middle ground.

  4. BY Mark says:

    It is more often the lack of comments that is the problem. Once comments are fully included any code deficiencies, fallacies, or excessive complexity become obvious to all.

  5. BY Mark says:

    It is more often the lack of comments that is the problem. Once comments are fully included any code deficiencies, fallacies, or excessive complexity become obvious to all.

  6. BY RobS says:

    What I’ve typically found of novice is that they can’t understand the concept or reason for abstraction.

    For example, why write this:
    void DoIt()
    { console.write(AddStuff(5,3));}

    int AddStuff(int FirstNumber, int SecondNumber)
    { return FirstNumber + SecondNumber; }

    When you can simply do this:
    void DoIt()
    { console.write(5+3);}

    The latter is so much simpler and so much more elegant…and so much more inflexible.

    Experienced people know that the above will likely start asking for values from the user and not always be hard-coded with 5 and 3; the novice lacks that knowledge and often just takes the simplest approach…and sometimes that’s the way to go but typically it’s not.

    Another place where I see differences is when the novice comes out of school armed with the latest “flavor-of-the-month” technique for programming and assumes that this should be applied everywhere. And maybe on new projects that’s true but on legacy projects it’s rarely a good idea to wedge these new techniques into a stable system…and make it unstable. Again, experience often tells you that these new techniques need to be reviewed but likely won’t be worth the cost+effort to add and keep things stable.
    On the flip side, any time a novice has an idea, I think the experiences people need to listen carefully to see if there a better way to do things with the new technology that arrive without us realizing it.

  7. BY RobS says:

    What I’ve typically found of novice is that they can’t understand the concept or reason for abstraction.

    For example, why write this:
    void DoIt()
    { console.write(AddStuff(5,3));}

    int AddStuff(int FirstNumber, int SecondNumber)
    { return FirstNumber + SecondNumber; }

    When you can simply do this:
    void DoIt()
    { console.write(5+3);}

    The latter is so much simpler and so much more elegant…and so much more inflexible.

    Experienced people know that the above will likely start asking for values from the user and not always be hard-coded with 5 and 3; the novice lacks that knowledge and often just takes the simplest approach…and sometimes that’s the way to go but typically it’s not.

    Another place where I see differences is when the novice comes out of school armed with the latest “flavor-of-the-month” technique for programming and assumes that this should be applied everywhere. And maybe on new projects that’s true but on legacy projects it’s rarely a good idea to wedge these new techniques into a stable system…and make it unstable. Again, experience often tells you that these new techniques need to be reviewed but likely won’t be worth the cost+effort to add and keep things stable.
    On the flip side, any time a novice has an idea, I think the experiences people need to listen carefully to see if there a better way to do things with the new technology that arrive without us realizing it.

  8. BY Cletus Junk says:

    If the senior programmer does not know how to defend his work on its merits, perhaps that fact speaks for itself.

    The intern requires no more schooling than a clear, egoless explanation of why the senior engineer’s approach is more suitable to the problem. After a few such spankings, the intern will change his attitude.

    If the senior engineer cannot deliver such spankings, he should bow to his new mentor.

  9. BY Cletus Junk says:

    If the senior programmer does not know how to defend his work on its merits, perhaps that fact speaks for itself.

    The intern requires no more schooling than a clear, egoless explanation of why the senior engineer’s approach is more suitable to the problem. After a few such spankings, the intern will change his attitude.

    If the senior engineer cannot deliver such spankings, he should bow to his new mentor.

  10. BY Tim Jowers says:

    Tell him “Fix it”. :”)
    What you really need to do is give him some hard, self-contained effort. He’ll learn fastest that way. Last team I managed I had one joker who’d been there 14 years and another young man who had 7 years of experience and knew Spring: I assigned them to retrieve some data from some POX RESTful services. They proceeded to use some crap XML marshaller from the Internet. I told them directly the service layer was using JAXB to marshal and even showed them the config files. For weeks they continued to do the stupid, lots of extra work approach. My point is a lot of old timers know historical approaches and don’t keep abreast on technology. (well, those dudes were both at least 5 years behind in Java; so, I can’t really say “abreast”. :-(
    See if the kid has any good ideas by giving him a mini-project. Most kids cannot even understand the project scope and you end up babysitting though. But sometimes you get lucky and get a person with a head on their shoulders. Not usually though.

  11. BY Raju says:

    We used standards and guidelines. This was a living document that changed every time an unaddressed conflict arose. With this one document, you can address issues like ‘readability’. This was c programming. We had defined a standard of using only 72 chars per line. Break the line at an opening parenthesis. And using a 4 space tab for indentations. Every opening curly brace had to be at the end of the keyword line and the closing curly brace in the same col as the first letter of the keyword. This was good…
    If ( x == y ) {
    Z = x;
    W = 2*x;
    }

    This was not…
    If ( x== y )
    {
    Z = x; W = 2*x; /* this comment line extends beyond seventy two characters on this line. */
    }

    • BY RobS says:

      That’s a matter of coding *style* rather than coding.
      For code to suck, you’d have something like this:

      For each record in a collection{
      recordid = record.id
      for each record in that collection
      if recordid = record.id then {
      fullrecord = record
      process fullrecord}
      }

      This is based on something a co-worker did once and it took about 2 hours to go through all records. When I saw that the inner loop was simply looping through and looking for the the record that you just found, I took out the inner loop and it processed in a few minutes. THAT code sucked!!!

  12. BY Raju says:

    We used standards and guidelines. This was a living document that changed every time an unaddressed conflict arose. With this one document, you can address issues like ‘readability’. This was c programming. We had defined a standard of using only 72 chars per line. Break the line at an opening parenthesis. And using a 4 space tab for indentations. Every opening curly brace had to be at the end of the keyword line and the closing curly brace in the same col as the first letter of the keyword. This was good…
    If ( x == y ) {
    Z = x;
    W = 2*x;
    }

    This was not…
    If ( x== y )
    {
    Z = x; W = 2*x; /* this comment line extends beyond seventy two characters on this line. */
    }

    • BY RobS says:

      That’s a matter of coding *style* rather than coding.
      For code to suck, you’d have something like this:

      For each record in a collection{
      recordid = record.id
      for each record in that collection
      if recordid = record.id then {
      fullrecord = record
      process fullrecord}
      }

      This is based on something a co-worker did once and it took about 2 hours to go through all records. When I saw that the inner loop was simply looping through and looking for the the record that you just found, I took out the inner loop and it processed in a few minutes. THAT code sucked!!!

  13. BY Jason says:

    On Slashdot, Anonymous Coward isn’t a particular username. It’s used for any anonymous user.

  14. BY Jason says:

    On Slashdot, Anonymous Coward isn’t a particular username. It’s used for any anonymous user.

  15. BY Adrian says:

    The best ways in software development:
    1) KISS
    2) SOLID
    3) Multitier architecture

    C’est vrai?

Post a Comment

Your email address will not be published. Required fields are marked *

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>