How can I tactfully suggest improvements to others' badly designed code during review?

Question

I'm a great believer in clean code and code craftsmanship, though I'm currently at a job where this isn't regarded as a top priority. I sometimes find myself in a situation where a peer's code is riddled with messy design and very little concern for future maintenance, though it's functional and contains little to no bugs.

How do you go about suggesting improvements in a code review when you believe there is so much that needs changing, and there's a deadline coming up? Keep in mind that suggesting the improvements be made after the deadline may mean they'll be de-prioritized altogether as new features and bug-fixes come in.

Answer 1

1. Double-check your motivation. If you think the code should be changed, you ought to be able to articulate some reason why you think it should be changed. And that reason should be more concrete than "I would have done it differently" or "it's ugly." If you can't point to some benefit that comes from your proposed change, then there's not much point in spending time (a.k.a. money) in changing it.

2. Every line of code in the project is a line that has to be maintained. Code should be as long as it needs to be to get the job done and be easily understood, and no longer. If you can shorten the code without sacrificing clarity, that's good. If you can do it while increasing clarity, that's much better.

3. Code is like concrete: it's more difficult to change after it's been sitting a while. Suggest your changes early if you can, so that the cost and risk of changes are both minimized.

4. Every change costs money. Rewriting code that works and is unlikely to need to be changed could be wasted effort. Focus your attention on the sections that are more subject to change or that are most important to the project.

5. Form follows function, and sometimes vice versa. If the code is messy, there's a stronger likelihood that it also contains bugs. Look for those bugs and criticize the flawed functionality rather than the aesthetic appeal of the code. Suggest improvements that make the code work better and make the operation of the code easier to verify.

6. Differentiate between design and implementation. An important class with a crappy interface can spread through a project like cancer. It will not only diminish the quality of the rest of the project, but also increase the difficulty of repairing the damage. On the other hand, a class with a well-designed interface but a lousy implementation shouldn't be a big deal. You can always re-implement the class for better performance or reliability. Or, if it works correctly and is fast enough, you can leave it alone and feel secure in the knowledge that its cruft is well encapsulated.

To summarize all the above points: Make sure that your proposed changes add value.

Answer 2

If the code functions without serious bugs, and a major deadline (as in effects P&L or corporate PR) is imminent, it's too late to suggest improvements that require major changes. Even improvements to code can create risks to project deployment. The time for improvements was earlier in the project, when there was more time to invest in the future robustness of the code base.

Answer 3

There is a sweet-spot for adding value through refactoring. Changes need to accomplish three things:

• improve code that are likely to change
• add the most clarity
• cost least effort

Considerations:

1. We know that clean code is less expensive to write and maintain, and is more fun to work on. Your job is to sell that idea to people in your company. Think like a salesman, not like an arrogant grouch (ie. not like me).
2. You can't win, you can only loose less.
3. Focus on adding real value - not just beauty. I like my code to look nice, but sometimes have to accept that inexpensive matters more.
4. A good way to find the sweet spot is to follow the Boy Scout Principle - when you work on an area of code, always leave it in better shape than you found it.
5. A small improvement is better than no improvement.
6. Make good use of automated tools. For example, tools that just clean up a bit of formatting can make a world of difference.
7. Sell other ideas that incidentally improve code clarity. For example, unit testing encourages decomposing big methods into smaller ones.

Answer 4

Code review serves 3 purposes:

1. Checking for bugs

2. Checking to see where the code could be improved

3. Teaching tool for whoever wrote the code.

Evaluating design/code quality are of course about #2 and #3.

As far as #2:

• Make it VERY clear what the benefits are from proposed changes vs costs to fix. As any business decision, this should be about cost/benefit analysis.

  E.g. "X approach to design would significantly reduce the likelyhood of bug Y from occuring when doing change Z, and we know this piece of code undergoes changes of type Z every 2 weeks. The cost of handling production outage from bug Y + finding the bug + fixing and releasing the fix + opportunity cost from not delivering the next set of featires is $A; whereas the cost of cleaning up the code now and opportunity cost (e.g. price of shipping late or with less features) is $B. Now, evaluate - or rather have your team leader/manager - evaluate $A vs $B and decide.

  • This will help the smart team leads to effectively manage this. E.g. they will make a rational decision using FULL information

  • This will (especially if you word this well) raise YOUR status - e.g. you are someone intelligent enough to see the benefits of better design, AND smart enough not to religiously demand it without weighing business considerations.

  • AND, in the likely event that bug Z happens, you gain all that much more leverage on the next set of suggestions.

As far as #3:

• VERY clearly delineate "must fix" bugs/issues from "This is a best practice and really ought to be fixed IF we can spare resources - see attached pro/con analysis" design improvements (attache the stuff described for #2 above) vs "These are general guidelines that I think would help you improve your code robustness so you can more easily maintain the code" optional changes. Please note the wording - it's not about "making your code like what I want" - it's "if you do this, YOU gain benefits a, b, c". The tone and approach matters.

Answer 5

Pick your battles, if a deadline is coming up then do nothing. The next time someone else is reviewing or maintaining code and they keep having trouble with it then approach them with the idea that as a team you should be more focused on code quality when reviewing code so that you won't have so much trouble later.

They should see the value before doing the extra work.

Answer 6

I always start my comment with "I would", which signals that mine is simply one of many views.

I also always include a reason.

"I would extract this block into a method because of readability."

I comment on everything; big and small. Sometimes I make more than a hundred comments on a change, in which case I also recommend pair-programming, and offer myself as wing-man.

I'm trying to establish a common language for reasons; readability, DRY, SRP, etc.

I've also created a presentation on Clean Code and Refactoring explaining why and showing how, which I held to my colleagues. I've held it three times so far, and a consultancy we're using asked me to hold it again for them.

But some people won't listen anyway. Then I'm left with pulling rank. I'm the design lead. Code quality is my responsibility. This change will not get through on my watch in its current state.

Please note that I'm more than willing to back down on any comment I make; for technical reasons, deadlines, prototypes, etc. I've still got lots to learn about coding too, and will always listen to reason.

Oh, and I recently offered to buy lunch to the first one on my team who submitted a non-trivial change on which I didn't have any comment. (Hey, you have to have fun too. :-)

Answer 7

Code reviews aren't always aimed at making improvements.

A review near the end of a project as seems to be the case here is just so that everyone knows where to start looking when bugs come in (or for a better designed project what may be available for later reuse). Whatever the result of the review, there simply isn't time to change anything.

To actually make changes you need to be discussing the code and design much earlier in the project - code is a lot easier to change when it only exists as talk about possible approaches.

Answer 8

I sometimes find myself in a situation where a peer's code is riddled with messy design and very little concern for future maintenance, though it's functional and contains little to no bugs.

This code is done. At a certain point, redesigns become too costly to justify. If the code is already functional with little to no bugs, then this will be an impossible sell. Suggest a few ways to clean this up in the future and move on. If/when the code breaks in the future, reassess the value of a redesign then. It may never break, which would be great. Either way, you are at the point where it makes sense to gamble that it will not break, since the cost will be the same now or later: drawn-out, terrible redesign.

What you need to do in the future is have tighter development iterations. If you had been able to review this code before all the work of ironing out bugs had been invested, it would have made sense to suggest a redesign. Towards the end, it never makes sense to do major refactoring unless the code is written in a fundamentally unmaintainable way and you know for certain that the code will need to be changed soon after release.

Given the choice between the two options (refactor or no refactor), think about which sounds like the smarter sell:

Hey boss, we were on schedule and had everything working, but now we are going to rebuild a lot of stuff so that we can add feature X in the future.

or

Hey boss, we are ready to release. If we ever have to add on feature X, it might take us a couple extra days.

If you said either one, your boss would likely say:

Who said anything about feature X?

The bottom line is that sometimes a bit of technical debt makes sense, if you were not able to correct certain flaws back when it was cheap (early iterations). Having quality code design has diminishing returns as you get closer to a completed feature and the deadline.

Answer 9

When a spoonful of sugar helps the medicine go down, and what's wrong can be expressed succinctly - there aren't 20 things wrong - I'll lead in with a form that suggests that I have no stake, no ego invested in what I want to be heard. Usually it's something like:

I wonder if it would be better to...

or

Does it make any sense to...

If the reasons are fairly obvious, I don't state them. This gives other people a chance to assume some intellectual ownership of the suggestion, as in:

"Yes, that's a good idea, because < your obvious reason here >."

If the improvement is fairly obvious, but not so much as to make me look an idiot for not thinking of it, and the reason to do it reflects a value shared with the listener, then sometime I don't even suggest it, instead:

I wonder if there's a way to... < shared value statement here >

This is only for dealing with really touchy people - with most of my peers, I just let 'em have it!

Answer 10

Code review must be integrated with the culture and development cycle to work. It is not likely that scheduling a big code review at the end of the the development of feature X will work. First of all, making the changes will be harder and someone is likely to feel embarrassed - creating resistance to the activity.

You should have early and frequent commits, coupled with reviews at the commit level. With code analysis tools in place, most reviews will be quick. Automated code analysis/review tools such as FindBugs and PMD will help you get a big class of design errors out of the door. They will not however, help you fish out architectural level issues so you must have a solid design in place and judge the overall system against that design.

Answer 11

In the event of cripplingly bad design, your focus should be on maximising the encapsulation. That way, it becomes easier to replace the individual classes/files/subroutines with better designed classes.

Focus on ensuring that the public interfaces of the components are well designed, and that the internal workings are carefully concealed. Also, Data Storage wrappers are essential. (Large amounts of stored data can be very hard to change, so if you get "implementation bleed" into other areas of the system, you're in trouble).

Once you've got the barriers between the components up, focus on the components most likely to cause major issues.

Repeat until deadline or until system is "perfect".

Answer 12

Instead of direct upfront criticism of someone's code , its always better to be cosntructive in our comments during code review.

One way that I follow is

1. it will be optimal if we do it this way.
2. Writting it in this way will make it run faster.
3. Your code will be much more readable if you do "this" "this" and "this"

Such comments will be taken seriously even though your deadlines are coming up. And probably will be implemented in the next development cycle.

Answer 13

Your question is "How to code review badly designed code?":

The answer IMO is simple. Talk about the DESIGN of the code and how the design is flawed or doesn't meet requirements. If you point out a flawed design or "doesn't meet requirement" then the developer will be forced to change his code because it doesn't do what it needs to do.

If the code is "functionally sufficient" and/or "meets spec" and/or "meets requirements":

If you are a peer to this developer, you do not have any direct power that would let you "tell him" to make changes.

There are a couple of options left to you:

1. You must use your own personal "influence" (a form of "power" that is indirect) and/or your ability to be "persuasive"
2. get involved with your organizations "code process" group and start making "code maintenance" a higher priority.
3. Bite the bullet and learn how to read crappy code faster/more-fluently so you don't get hung up (it sounds like you keep getting hung up or slowed down when you encounter crappy code) on crappy code.
   • This will also make you a stronger programmer.
   • And it will let you correct the crappy code when you are working on crappy code.
   • And this will also let you work on more projects because many projects have crappy code that is functional... but lots of crappy code.
4. Lead by example. Make your code better... but don't try to be a perfectionist.
   • Because then you will become "the slow guy who can't meet deadlines, is always criticizing, and thinks he is better than everyone else".

I find there is no silver bullet. You have to use all three and you have to be creative in your usage of all three.

Answer 14

Treat it like a free/open source project and submit patches that fix things and then have a discussion as to why your patch is an improvement over the old one. In some cases, there's a reasonable explanation as to why the current code is crap (out of time/money). In any case, the developer may want to improve things but they don't know how or they haven't seen another approach.

Don't approach this as "your code is bad, my code is better", frame it as "is my code acceptable?"