Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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.

share|improve this question
12  
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 5 '12 at 20:03
2  
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?). –  Baqueta Feb 7 '13 at 10:01
2  
@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
1  
@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 5:05
show 8 more comments

14 Answers 14

up vote 103 down vote accepted
  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.

share|improve this answer
2  
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 9:51
add comment

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.

share|improve this answer
add comment

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.
share|improve this answer
2  
+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
    
@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
add comment

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.
share|improve this answer
    
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 –  Juan Mendes Apr 17 '13 at 0:15
add comment

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.

share|improve this answer
add comment

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

share|improve this answer
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
    
Also known as YAGNI c2.com/cgi/wiki?YouArentGonnaNeedIt –  Juan Mendes Apr 17 '13 at 0:17
add comment

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!

share|improve this answer
add comment

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.

share|improve this answer
add comment

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

share|improve this answer
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
    
I wish I could learn #3, I get so frustrated with poor code that I have a hard time even trying to understand it... working on it constantly.... –  Juan Mendes Apr 17 '13 at 0:19
add comment

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

share|improve this answer
1  
I prefer the approach where you actually talk about things, before presenting your own solution. It's less like "oh look, i got something better than you". –  mri Feb 15 '13 at 8:44
    
As I said, it's possible to frame it as a learning experience for both developers. Also, having your own code ensures that you're doing the "put up" part of "put up or shut up" which is a situation that isn't fun to be in. I too like discussing solutions but discussion can only take you so far. –  omouse Feb 21 '13 at 2:01
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.