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

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

• improve code that is likely to change
• increase 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 3

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 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

[This answer is a little broader than the question originally posed, since this is the redirect for so many other questions on code reviews.]

Here are some principles I find useful:

Criticize privately, praise publicly. Let someone know about a bug in their code one-on-one. If they did something brilliant or took on a task nobody wanted, praise them at a group meeting or in an email cc'd to the team.

Share your own mistakes. I share the story of my disastrous first code review (received) with students and junior colleagues. I also let students know that I caught their bug so quickly because I've made it before myself. In a code review, this might come out as: "I think you got the index variables wrong here. I always check that because of the time I got my indexes wrong and brought down a data center." [Yes, this is a true story.]

Remember to make positive comments. A brief "nice!" or "neat trick!" can make the day of a junior or insecure programmer.

Assume the other person is intelligent but sometimes careless. Don't say: "How do you expect the caller to get the return value if you don't actually return it?!" Say: "Looks like you forgot the return statement." Remember that you wrote awful code in your early days. As someone once said, "If you're not ashamed of your code from a year ago, you're not learning."

Save the sarcasm/ridicule for friends not at your workplace. If the code is epicly awful, joke about it elsewhere. (I find it convenient to be married to a fellow programmer.) For example, I would not share the following cartoons (or this one) with my colleagues.

Answer 8

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 9

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 10

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 11

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 12

There are two notable issues in the question, the tactfully part, and the deadline coming up part. These are distinct issues - the first is a question of communication and team-dynamics, the second is a question of planning and prioritization.

Tactfully. I assume you want to avoid brushed egos and negative pushback against the reviews. Some suggestions:

• Have a shared understanding on coding standards and design principles.
• Never criticize or review the developer, only the code. Avoid the word "you" or "your code", just talk about the code under review, detached from any developer.
• Put your pride into the quality of the completed code, such that review comments which help improve the end result are appreciated.
• Suggest improvements rather than demand. Always have a dialogue if you disagree. Try to understand the other point of view when you disagree.
• Have the reviews go both ways. I.e. have the person you reviewed review your code next. Don't have "one-way" reviews.

The second part is the prioritization. You have many suggestions for improvements, but since deadline is approaching there is only time to apply a few.

Well, first you want to avoid this happen in the first place! You do this by performing continuous, incremental reviews. Don't let a developer work for weeks on a feature and then review it all at the last moment. Secondly, code reviews and time to implement review suggestions should be part of the regular planning and estimates for any task. If there is not enough time to review properly, something have gone wrong in planning.

But lets assume something have gone wrong in the process, and you are now faced with a number of review comments, and you do not have time to implement them all. You have to prioritize. Then go for the changes which will be hardest and riskiest to change later if you postpone it.

Naming of identifiers in source code is incredibly important for readability and maintainability, but it is also pretty easy and low-risk to change in the future. Same with code formatting. So don't focus on that stuff. On the other hand, sanity of publicly exposed interfaces should be highest priority, since they are really hard to change in the future. Persistent data is hard to change - if you first start storing inconsistent or incomplete data in a database it is really hard to fix in the future.

Areas which are covered by unit-tests are low risk. You can always fix those later. Areas which is not, but which could be unit-tested are lower risk than areas which cannot be unit-tested.

Say you have a big chunk of code with no unit tests and all kinds of code quality issues including a hardcoded dependency on an external service. By instead injecting this dependency, you make the code chunk testable. This means you can in the future add tests and then work on fixing the rest of the issues. With the hardcoded dependency you cannot even add tests. So go for this fix first.

But please try to avoid ending up in this scenario in the first place!

Answer 13

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 14

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 15

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 16

Increase the quality of the code reviews.

Apart from the quality of the code under review, there is such thing as a quality of the code review itself:

• Are the proposed changes really an improvement over that is present?
• Or just a matter of opinion?
• Unless something very obvious, has the reviewer explained properly, why?
• How long did it take? (I have seen reviews lasting for months, with the developer under reviewing in charge for resolving all numerous merge conflicts).
• Tone?

It is much easier to accept a good quality code review than some mostly questionable ego grooming.