Don't Criticize Code

Having been writing code for over 30 years, some of it good, some of it less so, I know there would be times that a consultant could justify critising my code, but I also know that unless done at the time the code is written, there is no point in critizing it. If the code does not work, say so, if circumstances have changed, and there is now a better way of achieving an outcome, say so, but never critise the coder. They may have moved on from coding and be a fantasic PM, they may be related to the person talking to you, they may just have been having a bad day when it was written, or they may have been working with code from an even older legacy system, the reasons for bad code are numerous. When I started it was immperative to reuse variables for something else to save the few bytes of memory (and tape) having another one would use-- Today, that looks like bad coding!

While I mostly do agree, I can not agree to 100%.

There is bad code and there is bad code. One udf of 2k rows calling a few more huge functions is not good code. When the functions or sp's has lost vision of their purpose it's not good code. Same for .net etc. When a class or sp trieds to do too much instead of keeping it simple, one method, one class, one responsibility, you know there is going to be issues with it.

I have a rule of thumb that has never let me down. Praise in public, criticise in private and make the apology as public as the sin that prompted it.

Phil is quite right to advise a programmer against criticising the code they've inherited except in certain specific instances that I would have hoped were obvious (such as making your boss aware of the quality of what's in place so he or she can factor it into their future decisions). Recognise the code's shortcomings by all means, but then either do something about it or keep quiet.

Perhaps the most insidious problem of criticism is the effect on the criticiser. I agree with other posters here that constructive criticism can be very positive, but even then one still has to be careful. When faced with an awkward situation - in this case questionable code - you have a choice. Either you can think, "What a mess; how can I be expected to work with this?" or you can take the view that "There's loads I can do to improve this." A lot of criticism, even some of the constructive type, tends to focus on the former view - the damage control - rather than seeing the potential, so is almost inherently self defeating. The code's the same, but your attitude towards it can make all the difference.

Do you want to do your best or would you be happy just doing "the best you can under the circumstances"? Your choice.

Great article, and on the spot.

The key here is to understand that you probably dont know the context in which the code was written.

"Just get it done by noon, or its a no go"

"Of course this system wont handle more than a few hundred customers"

"Hard code that stuff for now, and we'll get back to it when we've through the acceptance test"

"Lust mock up a prototype, I promise it wont be put into production"

"The production system will have a wicked fast SAN, but we wont get much cpu"

These kind of statements will affect how the code is written, and none of them will reward good coding practices. And when looked upon in an other context, the code will seem strange, sloppy, meaningless or what not. But in most cases it got the job done in the original context.

Then there is the strokes of genius you sometimes find in old code. Starting to look at it, it seems like total gibberish, but in the refactoring process you'll discover elegant trade off's between performance, readability and testability. And then there are blazing fast algorithms, creating heineous but effective data structures and odd processing.

I see a good coder as someone that gets the job done according to spec. If the spec will allow bad coding style or ineffective code, so be it. That's a management problem, not a coding problem.

On one memorable occasion, I was strolling around an IT department of an international company with the CTO (IT Director actually) chatting about this and that, when we popped over to look at a dev team and get a quick demo of their work. They were dealing with a legacy system of some antiquity that, nevertheless, worked well and just needed upgrading as the hardware and software got upgraded. One of the programmers happened to say something like 'Jeeze, I'd love to meet the prize idiot that wrote this damned code, and poke him in the eye!' at which the Boss swung around fiercely and said 'You're talking to him, D*** you!'. Oops! (It was true. He had served in many roles for the company's IT department over the years)

I'm critical of all code, my own above of all the rest. But when dealing with all code there are constructive ways to get the improvements accomplished. Mainly I do improvement by suggestion or example. Sometimes a link to an article or a reference to a coworkers or my own similar source for solving a particular problems helps.

Yes, I have criticized my boss's and his boss's code. But I had a complete solution to the problem wrapped up in bows and was extremely tactful about the situation. Most of the time they don't know or care about the improvements, just that the problem has been solved without fuss.

All very true.

But when I'm explaining to a manager why I need to take their system offline for a few days to rip its guts out and replace it completely, they want to know why. At that point, I certainly have to review the quality and reliability of the current code, and why incremental, non-interruptive changes aren't the best option in this case.

I seem to have two basic options:

1. "Trust me" Tell them I know what I'm doing and why, and expect them to leave it at that. That makes me come across as arrogant and insulting to them.

2. "The code I'm replacing stinks and here's why..." Explain in layman's terms why it's better to rip it out and rebuild it. That comes across as criticizing the code, because that's what it is.

Of course, there is a third option, which would be don't tell them at all, and just start ripping things out and rebuilding, and let the affected managers and co-workers figure out on their own why there aren't any updates going from system A to system B. That seems even worse. I don't consider that a real option, even, because it seems like a good way to lose a job fast.

Even on code that doesn't have to stop functioning for any length of time, and can be incrementally refactored in a way that doesn't interrupt current operations, I have to justify to management why I'm spending my time on that. The company pays me, and the managers charged with the usual fiduciary duties expect to have justification for my salary compared to my projects. They rightfully need to know what I'm working on, and why I pick certain tactical targets. Again, I'm left criticizing the code.

Constructive criticism for purposes of educating someone has a place. But not in this setting.

Phil, I usually agree with most of what you write. I will sometimes nitpick little bits here and there. Other times I'll play devil's advocate and disagree just to get some action going in the discussion for my own entertainment. (Yes, I really do think that way sometimes.) I do the same to Steve on his editorials. And don't even get me started on politics, where I'll disagree with people just to see what effect it creates. But in this particular case, of never criticizing code, I really do disagree. There are times when it is necessary to do so. Am I likely to offend someone? Possibly. Does that bother me? Yes. Do I consider it the unfortunate consequence of a necessary action? Yes. Surgically removing a tumor inevitably traumatizes healthy tissue and compromises immunology - but a good doctor still recommends and does it to save the patient's life. While taking necessary actions to minimize the trauma, and working dilligently towards less traumatic methodologies, of course.

Arguably, though, the quality of the code doesn't matter; it either fulfills its business requirement or it doesn't. If you need to justify downtime to rewrite portions, then why not focus on business benefit? Hardly difficult, then, to show a legacy system needs extensive modification without knocking the fact it's spent years doing what it was designed to do, and it's then irrelevant whether it was a collection of cludges or the epitome of elegance in its prime.

I try not to criticize code too much. I know that I'm still early in my career and my ways are definitely not always the best way.

I do criticize code that's badly formatted, though. Maybe it's because formatting was ingrained in my when I first started learning to write code in high school, but I find it very difficult to look at code that isn't consistently formatted. My coworkers laugh when they ask me to look at their code and I reformat it in front of them before I look at. (I've been working with them for almost two years now, I didn't do that when I first started)

I've seen a lot of code and if someone makes a choice that I would not have, I don't have a problem with that.

However, there is bad code out there by adolescent or inadequately trained programmers that deserves to be criticized. In many cases, this may require complete re-writes to handle changed requirements and the customers may not understand why it will take so long for such a "simple" change.

Sometimes the code was so horrendous that it was easier (quicker) to go back and get requirements and read hardware manuals than fixing the code. This has usually happened in software applications interfaced with custom hardware where the developer was an engineer or a shop floor supervisor (in one case) rather than a developer.

I'll accept code that goes through the mutations of evolution and the bandaid & tourniquet approach to solving problems - if those that have touched code have left comments.

I'm wary of rip&replace on squirrelly code because I can't be sure I've captured everything it does unless I understand every part of the as-is before implementing the to-be. The more squirrelly it is the more likely I won't have the luxury of time to fix. However:

I consider it a contribution to the mess if we become aware of bad code and do not comment on it. We were there for some reason. We figured out enough to get today's work accomplished. If we don't leave some bread crumbs for the next maintenance developer then we become part of the problem.

I've been telling new developers: "The code comments you leave today will save the poor maintenance programmer of tomorrow. You should care deeply about that person; it will likely be you."