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.

Is it important to point out the good parts of the code during a code review and the reasons why it is good? Positive feedback might be just as useful for the developer being reviewed and for the others that participate in the review.

We are doing reviews using an online tool, so developers can open reviews for their committed code and others can review their code within a given time period (e.g. 1 week). Others can comment on the code or other reviewer's comments.

Should there be a balance between positive and negative feedback?

share|improve this question
2  
Hey, if it passes, it's positive feedback. :) –  iKnowKungFoo Oct 11 '12 at 14:49
1  
To a large extent, I think it depends on the person whose code is being reviewed. If they will react negatively to only receiving criticism, then it is important to find a balance; otherwise, positive feedback is redundant since passing review is inherently positive. If they do something new and wonderful you can mention it, but incorporating it into your team's best practices would also be positive feedback. –  Matt Oct 11 '12 at 16:07
add comment

14 Answers

up vote 39 down vote accepted

Improve Quality and Morale Using Peer Code Reviews
http://www.slideshare.net/SmartBear_Software/improve-quality-and-morale-using-peer-code-reviews

Things Everyone Should Do: Code Review
http://scientopia.org/blogs/goodmath/2011/07/06/things-everyone-should-do-code-review/

Both of these articles state that one of the purposes of code review is to share knowledge about good development techniques, not just find errors.

So I'd say it's very important. Who wants to go to a meeting and only be criticized?

share|improve this answer
24  
Me! Me! sarcasm aside, I've actually grown very annoyed of code reviews where there's no criticism of my code; I know I didn't do things perfectly and so lacking criticism makes me feel like I'm wasting my time even asking for the review. So I agree that nothing but criticism is bad, but none is as well. –  Jimmy Hoffa Oct 11 '12 at 14:47
3  
I tend to agree with Jimmy Hoffa. In general (not just in code reviews), I find it very annoying to deal with people who try to make lots of positive feedback. Positive feedback should be useful: there is no need to pollute the review by saying things that the author of the code already knows. Personally, I prefer the attitude: "You're great and smart, but there are a few minor issues in your code." –  MainMa Oct 11 '12 at 18:30
6  
@MainMa: "Looks fine" works for me, if there are no problems found. For especially useful or well-written code: "This could be useful. Let's put it into our code sharing archive with some notes, or try to incorporate it into our daily coding practices." –  Robert Harvey Oct 11 '12 at 18:32
1  
We once had someone quit because of how horrible our code reviews used to be. We've since changed over to using code reviews as more of a workshop, with a little bit of code criticism for serious problems, but mostly for education. The guy that quit got into a screaming match with our manager during the review because of differences in opinion. People can get really defensive during reviews, so I highly recommend giving positive feedback to relieve tension and make the "you should change this" moments easier for the reviewee to deal with, if for no other reason than to occasionally stroke egos –  Brian Oct 11 '12 at 21:27
2  
@Brian I have to say, if someone gets into a screaming match over a little criticism they're likely a detractor from company culture and office morale altogether, I think you're better off. –  Jimmy Hoffa Oct 11 '12 at 21:36
show 4 more comments

When I do code reviews I tend to just have a running monologue, so as I'm making sense of what I'm reading there will be a lot of "Ok, I see what that does.. Good it connects to this and calls that, alright.. and that piece depends on both of those alright.".

I think in this way it's not "oo la la this is so great!", it could be perfectly trivial boring code, but hearing somebody else actually parse and show comprehension of what you wrote is a form of positive feedback in and of itself, the feedback being "This code makes sense", when I run into parts that I don't understand I ask for explanation and when I do understand it exclaim "Ah, I got it".

I think that simple transfer of comprehension is praise to another engineer because we all want our code to be understood by others, it gives a form of implicit validation.

That said, if you see parts of the code that are good or positive characteristics (even boring trivial code can be good if it's the minimal form of itself) I definitely tend to state those characteristics, again I don't attribute them as "Wow great!" so much as "I see this is a minimal implementation" or "Ok, this complex algorithm has lots of comments", focus on the attributes of the code not so much it's inherent goodness or badness.

Any time you attribute "goodness" or "badness" to code in a code review to avoid making the engineer feel looked down upon or held on a pedestal don't say something is good or bad, but rather talk through the cause and effect of their code.

"Ok this part makes sense, ah there's a magic number here, the meaning of that value might not be well understood by the next engineer to touch this"

"I see you've got a DI container here ok so you'll have loose coupling with that repository"

"Ah there's a static dictionary here, if multiple threads are touching that dictionary we could run into some race conditions"

Notice, I'm not saying anything's good or bad, but whether the engineer should change it or not is going to be understood by the engineer whose code is being reviewed. Obviously you have to end the code review with a yay or nay, but accumulating these statements over the course of it will soften the nay's as explanation has already been made in the form of cause and effect statements when you tell them "I'd like those magic numbers fixed before checking this in".

share|improve this answer
3  
+1 for "simple transfer of comprehension is praise to another engineer... it gives a form of implicit validation" –  Roy Tinker Oct 11 '12 at 20:35
    
I want to +1 this twice. One for the same reason as @RoyTinker, and one for "Don't say something is good or bad, but rather talk through the cause and effect". Both very good points! –  Ben Lee Oct 15 '12 at 20:26
add comment

If I saw something in a code review that I really liked and was above and beyond "good-enough" code, I'd give positive feedback.

In general, I think that if someone writes a piece code that actually makes you say "Wow, this is really nice!" then yes, positive feedback is important - it makes it known to the author that what they did was enjoyed by others, and they should try to do that again. It has to be more than just following guidelines and standard practices though. Giving out praise because someone indented nicely or added boilerplate comments could set the bar rather low.

share|improve this answer
add comment

This isn't so much a programming question as it is a general management and human-interactions question. Positive feedback in code reviews is exactly as important as positive feedback in any sort of review.

Whether or not this is required (and the extent to which it is required) is a function of the disposition and emotional makeup of the person you're talking to. Some people respond to correction much more effectively when it is coupled with praise. Others see praise as insincere when delivered with correction.

The general formula is sometimes called a "Feedback Sandwich": Good stuff first, bad stuff second, good stuff last. The idea is to keep the overall tone positive while at the same time making sure that the negative feedback is received. This can help prevent stress when anticipating a review, and help prevent self-absorbed brooding afterward. Both are very important with respect to productivity and quality. This isn't just touchy-feely emotional hogwash; It's human behavior 101.

Again, you have to know the person you're working with and understand what they respond to. Dealing with people is what management is about, and good managers know how to make people respond.

share|improve this answer
add comment

It's more important if you're doing side-by-side or team reviews. In a written review, no news is good news. The goal is to get code into production. When it's your code, you should feel good about yourself.

Code review should be used as a source of information to help with mentoring and managing of the team. There are plenty of opportunities to give positive feedback without cluttering up the code review database. Examples can be pulled out to share with others.

There is a lot more to reviewing the developer other than their code. Hijacking code review time can be counter-productive to getting an app out the door. Set time that is specific to helping the developer outside of code reviews, but that doesn't mean you should exclude code review feedback.

share|improve this answer
add comment

I think positive feedback is very important and it's mainly from a personal, realpolitik dynamic. We all sit and write code for hours, days, weeks, months, and most of us take pride in what we do. Code reviews are a chance to showcase that.

If you go to a code review and the best outcome you can hope for is "no comment" (i.e. there is no balance of positive feedback), the meeting could easily be titled in outlook "Find Out How Badly People Think You Suck". Consequently, developers will start to be annoyed by or even dread code reviews, and that's clearly a detriment to the team. Developers will "forget" to get their code reviewed or will develop learned helplessness and simply ask their constant critics what to do about every little thing to avoid getting blasted in these meetings.

It's all well and good to say that, theoretically, it's most logical to fix the bad and to ask everyone to leave emotions at the door, but it's precisely attitudes like that one that are responsible for the rep developers get as being interpersonally tone deaf. Theories aside, we're humans and humans like to get a pat on the back from time to time, even a nominal one. That stuff matters.

share|improve this answer
add comment

The workflow that I liked most with code reviews was this:

  1. Dev submits patch on mailing list/online tool.
  2. Everyone who needs to care looks at the patch, suggests improvements.
  3. Dev goes back to #1
  4. If there's no improvement needed, people say "Good work, please commit." <- POSITIVE FEEDBACK. All code that is accepted is good. The less people have to tell you to change things, the better you're doing.
  5. Dev commits, moves on to the next item.

Usually what would happen is that new devs would get a lot more 'correctional' feedback as they got familiar with the codebase.

The benefits of this approach are:

  1. Everyone knows what everyone is doing. There's no knowledge monopolizing or mystery commits.
  2. Everyone learns from other's feedback. This is important. If feedback only happens between 2 people in a face-to-face conversation while pairing, then someone on the other side of the room does not benefit the same way as they would if it happened on the mailing list.
  3. Other devs can usually spot some bugs before they're in the version control.
share|improve this answer
add comment

The only way that I can think of where providing positive feedback about code could backfire on you is if you're not careful to avoid the "backhand compliment." Most people are familiar with this... it's signified by phrases like, "Great job, but..."

If everyone comes to the meeting with the attitude that this is not a personal review of the programmer, but an effort to improve coding practice for the quality of the entire system, then all feedback is "good" feedback. Feedback that highlights ways to improve coding practice becomes just as important as feedback highlighting a useful new method for handling a problem.

At the very least, if one doesn't go to that length, it should be stressed that striving to do a "good feedback, bad feedback, good feedback, bad feedback" cycle within the review process is only going to come across with the same backhand compliment feeling. Don't try to force good feedback, try to reinforce good effort, and shore up holes in knowledge.

Phrases that I've learned the most from, over the years:

  • "That's an interesting approach. What happens if we have to accommodate [some other use case]?"
  • "Nice try! Did you know we already have a method for doing that? Maybe we should do some benchmarking to see which approach is more efficient."
share|improve this answer
add comment

I can't agree with this at all. What's the difference between Good Development Techniques and so-called Ninja Coders who can write awesome but inexplicable-to-mere-mortals code? Software Development is now (IMO) a Lowest Common Denominator discipline where flair and cunning are shunned in favour of maintainability and ease-of-understanding. It's just too risky.

I can't think of a time I've ever seen code in a review that would make me go 'Oh that's cool'. I can only assume that if I did encounter code like this it'd fall into the Cool-Yet-Unacceptable camp.

You'd also have issues with people who get no positive feedback perhaps trying too hard and making an eventual mess "Trust me, it works!".

Code reviews are there to spread code quality responsibility among the team, i.e. an individual developer cannot be blamed if a serious problem arises later. Use it to find issues, use it to get explanations from the original developer of weird stuff in case you ever end up having to maintain it. Personally, I'm more interested in receiving negative feedback. Customers don't care about the coolness of your code, only that it does what they want.

Leave the backslapping to the pub.

share|improve this answer
1  
"Customers don't care about the coolness of your code, only that it does what they want." Customers also don't care about code readability. They might care about how long it takes to fix a bug, add a feature, or change some behavior, but they certainly don't care about code readability per se. –  Michael Kjörling Oct 12 '12 at 11:00
add comment

Mantra is simple: If one wants quality Code (which is less in reality) then proper review methods must be practiced. Having said that, positive feedback helps a developer/programmer to think and come up with ideas/solutions/fixes. Don't be too harsh, but be firm on the point. Q&A Managers should be aware of good methodologies and practices so that he/she could guide the team (or a member) in the right direction. This results in quality. Period.

share|improve this answer
add comment

Not nearly as important as honest feedback. I work for a large financial corporation, and our customers don't care if the programmer is trying hard or is a good guy, or usually write good code! They require software that works.

share|improve this answer
    
My experience is that programmers that try hard, are 'good guys' and are happy with a supportive team tend to write software that works. –  c_maker Oct 11 '12 at 17:31
    
Chicken and egg I guess. But the question was about code review... which I just don't think is the time to stroke ego... –  aserwin Oct 11 '12 at 17:40
    
Code review isn't the time to determine whether or not the user-visible portions of the software work according to spec. –  Michael Kjörling Oct 12 '12 at 11:25
add comment

It has mattered to me. I don't want fluff comments or positivity for the sake of positivity. If all the code I wrote is crappy, you tell me why and let's correct it and learn. But if I do something right, it's nice to hear it once and awhile. I don't need positive reinforcement for everything I did that was "correct", but even if it's a "let's improve X, Y, and Z, but the rest looks good" it matters.

share|improve this answer
add comment

I think it is important to be completely objective. Attempting to boost morale by making positive comments is a waste of time to my mind.

This can mean that code reviews are unduly critical - but then isn't that the point. We should also be critical of ourselves. I find that the assumption that the code that I have written is probably complete rubbish and definitely could be improved drives me to improve my code and my skill level.

If you get no comments then you can consider you have done a good job.

share|improve this answer
add comment

When the code is for a competition or submitted for a job interview (in other words, code that's written and cannot be rewritten), then positive comments are a must. In fact, you should ensure that there is positive feedback (where possible!) as well as negative. That way, the coder knows where his strengths and weaknesses lie, and can compensate.

However, you seem to be talking in a workplace environment, where the code can be rewritten. In which case, you're trying to get bugs the hell out of your system. So, in that situation, only negative bugs are of any worth.

If you feel uncomfortable about that, have a weekly code review meeting, where everyone can discuss both good code and bad code.

EDIT: Though I will say that, if something impresses you enough, there's nothing stopping you from expressing your praise in person. The tracker, however, seems to be only for production code review.

share|improve this answer
5  
"So, in that situation, only negative bugs are of any worth." - I can't agree with that at all. If someone comes up with a great way of fixing a bug/implementing a feature, it's absolutely not worthless. And keeping motivation up is important. If you only highlight failure, you're going to have issues. –  Mat Oct 11 '12 at 14:26
    
Poor word choice on my part. While the good stuff isn't worthless (having written enough dross in my time), the bugs are, most likely, what the tracker was set up for. The OP can, if he so chooses, leave positive comments. But, personally, I'd leave that for face-to-face conversations, as it prevents the tracker from being clogged up. Also, I'm very annoyed I can't vote up comments. :) –  KBKarma Oct 11 '12 at 14:33
1  
@KBKarma - if you feel that your original answer wasn't phrased as well as it could have been, please go back in and edit your answer to properly reflect your thoughts. Look for the edit button underneath your answer. –  GlenH7 Oct 11 '12 at 18:51
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.