139

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.

Thomas Owens
  • 79,623
  • 18
  • 192
  • 283
Yony
  • 1,647
  • 3
  • 12
  • 14
  • 26
    Firstly, make sure your view is not subjective. Quite often developers write off other peoples code because they simply like another style or dialect. If it's not the case, try to offer improvements one at a time. – Coder Oct 11 '11 at 14:02
  • Since a lot of software development has to do with tradeoffs, and because I'm talking about code craftsmanship which is mainly design-based, many code review comments end up being subjective. That's why I asked "how do you go about *suggesting* improvements". Its certainly not my goal to dictate anything. – Yony Jul 05 '12 at 20:03
  • 5
    The question makes it sound as though the code review is happening *after* testing has been completed. If that's the case then you're either wasting testing time (any changes need to be re-tested) or making it much harder to get changes made as a result of the code review (why change code which already works?). – vaughandroid Feb 07 '13 at 10:01
  • 3
    @Baqueta - Why review code and waste multiple people's time when you have no idea if it works yet? – Dunk Feb 14 '13 at 22:41
  • 4
    @Baqueta There are obviously different kinds of tests. If they're to be useful, code reviews should happen after initial tests, like unit tests (so you know it works), and before final tests, like user acceptance tests (so that changes don't incur a pile of red tape). – Caleb Feb 15 '13 at 05:05
  • @Dunk: I didn't say to review before doing any testing; I said don't do it after you've *completed* testing. Yes, before you start reviewing you ought to know that the code compiles and is more or less working, but "contains little to no bugs" to me suggests a lot more than that. – vaughandroid Feb 15 '13 at 09:39
  • Thinking on my previous point further, perhaps code reviews ought to be done sooner for programmers who have been shown to frequently take the wrong tack (as the peer in this question). I'm not sure how you deal with that in terms of office politics though! – vaughandroid Feb 15 '13 at 09:41
  • @Baqueta:I worked at a place where the code review was done before writing/performing unit tests. What a waste of time. The people who liked it thought they were geniuses for finding errors that would have been easily caught with just basic tests, if people were allowed to test first. Sure they caught the brain-dead errors but missed the important ones, like does the function do what it needs to do in order to work with the system. As for reviewing code of problem developers sooner, that is what your leads are for. – Dunk Feb 15 '13 at 22:08
  • @Dunk Code reviews after code is checked in almost always gets de-prioritized and won't get fixed unless it's a correctness issue. – Ruan Mendes Apr 17 '13 at 00:11
  • @Juan:Yep, formal code reviews do get deprioritized. But by the time our code gets formally "locked-down", it already works for the project's needs, it works correctly, and most of it has been looked at and fiddled with by more than 1 developer because they had to integrate with it. I don't understand the comment "won't get fixed unless it's a correctness issue". Formatting/coding standard issues should be checkable and fixable by your tools. The code compiles without warnings is easily verifiable. There's not a lot left to pick on after that other than fixing incorrect code. – Dunk Apr 17 '13 at 23:12
  • @Dunk I guess you don't mind when people use known anti-patterns or messy symbol names in their code making it harder to understand or modify the code... As long as it works, it's good? I'm on the other side, that's why we code review code first. To ensure good practices are being followed. You mentioned a project being finished; however, a project is never finished, there's always more features being added and bugs being fixed, substandard code hinders that. If something is worth doing, it's worth doing well. I understand there may be time constraints, but that's another case. – Ruan Mendes Apr 17 '13 at 23:43
  • @Juan:I agree, if something is worth doing then it's worth doing well. I contend that "your well", which I spent most of my career doing, ends up in code and a product that is "far, less well" than how we are currently doing it. Messy symbol names(use tools), anti-patterns get fixed by someone who knows what they are doing. I never mentioned "project being finished"...I said "locked-down" meaning now we are at a point that we don't want people changing a code file unless it has a "Change Request" associated with it. Code gets locked down after it has been through a review – Dunk Apr 18 '13 at 22:05
  • http://c2.com/cgi/wiki?ConstantRefactoringIsaGoodThing – Ruan Mendes Apr 19 '13 at 20:16
  • "functional and contains little to no bugs" how would you know? If it's difficult to follow... there's probably the nub of an answer in there. – James Snell Dec 21 '15 at 15:40
  • meta discussion: [Is this question about code reviews on topic? If not, why?](http://meta.softwareengineering.stackexchange.com/q/8290/31260) – gnat Oct 28 '16 at 19:52
  • Something I've encountered with poorly written code as I've reviewed it, is that because the developer who wrote it had such a different approach to the problem, that it caused issues in the design. It was not possible to propose changes without ending up with a complete rewrite of the code and thus no changes were accepted. What I took away from that, and what I think comes across from your question, is that it can be very useful to discuss the design beforehand so that a lot of problems can be avoided. – Jonathan van de Veen Sep 28 '17 at 14:26
  • Hmmm... code has "little concern for future maintenance" yet at the same time "it's functional and contains little to no bugs"? Maintainability doesn't equal cleanliness. It could be very pragmatic to write code hastily (yet keep design robust) when its future is uncertain. If you have reasons to believe otherwise - just let them fail and demonstrate maintainability advantages of your clean coding. – KolA Aug 20 '19 at 08:39

16 Answers16

169

  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.

Caleb
  • 38,959
  • 8
  • 94
  • 152
  • 3
    Indeed, it comes down to 'selling' your concerns. As mentioned: point out benefits and added value. That's a tough job, also in my experience. – Wivani Oct 12 '11 at 09:51
  • 4
    Understanding your own motivation is not just selling. You need to understand why you like some techniques and not others, so you can know when your rules of thumb are valid and when they are not. Many, many issues arise from experienced programmers applying correct techniques in the wrong situations. – Jørgen Fogh Mar 01 '15 at 09:04
  • 2
    The corollary to point (6), interestingly seems to be that interface quality is more important than implementation quality – Bradley Thomas Dec 15 '16 at 17:53
  • I would go further with the last one: **Make sure that your proposed change adds *sufficient* value.** Doing it might mean not doing something more useful. – Deduplicator Jan 26 '19 at 23:05
16

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.
Declan McKenna
  • 476
  • 4
  • 12
Kramii
  • 14,029
  • 5
  • 44
  • 64
  • 5
    +1 for use of automated tools. Shockingly, it seems many shops don't care enough to see what the developers tool kit looks like. Just because you've got source control, an editor, and a compiler, does not make your toolkit complete. – Spencer Rathbun Oct 11 '11 at 14:19
  • 4
    @Spencer: I couldn't agree more. At the same time, I get frustrated by developers who don't use the tools they've got - through ignorance of the function or benefits or plain laziness. Most modern IDEs have a built-in code formatting function that takes just a couple of keystrokes - yet some devs don't use it. – Kramii Oct 11 '11 at 18:24
  • 2
    True. But I include that underneath the shop itself not caring. Your devs may not know about some functionality within the current toolset, especially if management never bothered to create standards. Secondly, many IDEs are very large, with a huge feature set. I've been using vim for a couple years now, and I still don't know all the different things I can do with it. If you dropped me into Visual Studio, I'd leave 90% of the fuctionality untouched until I had time to dig through it. Then I might not remember it. – Spencer Rathbun Oct 11 '11 at 18:32
14

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.

ChrisF
  • 38,878
  • 11
  • 125
  • 168
hotpaw2
  • 7,938
  • 4
  • 21
  • 47
9

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.
DVK
  • 3,576
  • 2
  • 24
  • 28
  • 2
    On #3, code reviews aren't just for teaching the code's author. A review can be a good way for less experienced developers to learn, and for experienced programmers who are new to the team to come up to speed on coding standards. Discussing problems as a group can also lead to insights about the product. – Caleb Oct 11 '11 at 13:13
  • 1
    @Caleb - good point. I didn't want to make TOO many points, so this one got edited out of the outline, but it's still a valid point. – DVK Oct 11 '11 at 14:13
  • #4 cross training developers on new features – Ruan Mendes Apr 17 '13 at 00:15
  • 1
    #5 - the prime purpose of code reviews is to ensure that the design document has been implemented (correctly) – Mawg says reinstate Monica Feb 09 '15 at 10:56
9

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.

Thanos Papathanasiou
  • 2,462
  • 1
  • 20
  • 22
8

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. :-)

Roger C S Wernersson
  • 756
  • 5
  • 12
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.

enter image description here WTFs/minute

Ellen Spertus
  • 728
  • 1
  • 6
  • 13
5

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.

Trevor Boyd Smith
  • 234
  • 1
  • 8
5

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.

Morgan Herlocker
  • 12,722
  • 8
  • 47
  • 78
  • Also known as YAGNI http://c2.com/cgi/wiki?YouArentGonnaNeedIt – Ruan Mendes Apr 17 '13 at 00:17
  • How's about: "Hey boss, you know that feature X you want, well, we need a couple of days before we can start working on it.". He would like that either. YAGNI is not a excuse to create messy code or leave code messy. A little technical debt is not a big problem, but debts must repaid, an the sooner you repay it, the cheaper it will be. – Thorsal Mar 02 '15 at 14:58
5

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!

Ed Staub
  • 221
  • 1
  • 6
  • 1
    It's rare that I'd say "I wonder if it would be better to...". I'd only say that if I wasn't sure - in which case the author is free to think if it would be better or not and is free to change or not. I usually say "I would do X". (Sometimes I'll say "I would have done X, but your approach is better"). Which means I think X is better, but you can disagree. Or I'll say "this doesn't work" or "this is dangerous" and you better change it. Sometimes I'm told "this doesn't work", and usually I look at the code, I'll say "Oh shit", and then I fix it. – gnasher729 Oct 28 '16 at 21:31
3

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.

Tom Clarkson
  • 1,342
  • 10
  • 8
1

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

deworde
  • 1,892
  • 14
  • 21
1

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.

Jay D
  • 129
  • 4
1

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!

JacquesB
  • 57,310
  • 21
  • 127
  • 176
0

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.

kgambrah
  • 9
  • 1
0

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.

h22
  • 905
  • 1
  • 5
  • 15