tags:

views:

2623

answers:

32

All programmers have their style of programming. But some of the styles are let’s say... let’s not say. So you have code review to try to impose certain rules for good design and good programming techniques.

But most of the programmers don’t like code review. They don’t like other people criticizing their work.

Who do they think they are to consider themselves better than me and tell me that this is bad design, this could be done in another way. It works right? What is the problem? This is something they might say.

So how do you make people accept code review without starting a war?

How can you convince them this is a good thing; that will only improve their programming skills and avoid a lot of work later to fix and patch a zillion times a thing that hey... "it works"?

People will tell you how to make code review (peer-programming, formal inspections etc) what to look for in a code review, studies have been made to show the number of defects that can be discovered before the software hits production etc. But how do you convince programmers to accept a code review?

+18  A: 

Well you can't really, if someone just flat-out refuses and you're not their boss.

But the main reason to adopt another style is ...

Consistency

I think, whatever crazy style there is, it must be consistent. So if a project is build in this style, it must typically stay in this style, unless there is an exceptionally compelling reason to change. So if their style doesn't match; it's a fact that it should be made to match. No question here.

There are other problems that can be noted that are just flat-out wrong. Security, general 'wrong-ness', etc. These are unquestionable and should be changed.

If someone refuses to acknowledge something as bad design, after you have shown them the correct way, and shown them all the risks and problems with what they do, I think you then need to decide for yourself how to act. But you would hope you are not working with people who are so unreasonable.

Noon Silk
+1 for great answer. I definitly agree with you. Its about consistency and learning about best practises. This is important to shape up your style and learning about nice new ideas.
bastianneu
+74  A: 

I've found that people who don't like code reviews will do their best to work around whatever you put in place.

The best way to make sure that the code you work with is code reviewed properly is to work somewhere that treats that as the normal way of coding, and that only hires developers who are likely to fit into that environment well.

If you can't change who you work with, you might have success if you first give your own code for review. Encourage them to find fault with it (dare I suggest adding in a few deliberate mistakes?) so that you can demonstrate that it's not meant to be a way of criticising the developer in question. That's the main problem, IMO: people take code reviews too personally.

Jon Skeet
+100 for 'people take code reviews too personally'
rahul
+1 for being a student of the human condition, I'll admit I'm guilty of the last sentence
Scott Vercuski
Well, I must disagree, a review is *always* personal, but the real trick is to realise there is no problem in being wrong. People stick to their views too much as they think it is weak to be wrong; I think it is weak to not be wise, admit mistakes, learn, and move on.
Noon Silk
@silky: It's not personal as in, "I'm not saying you're a bad person, I'm saying your code has problems." There's a big difference between attacking the code and attacking the person. If you go into a code review saying, "What were you thinking? Are you stupid or something?" then that's a problem :)
Jon Skeet
Indeed :) I'm sure we're in general agreement. Mainly I am saying I think the problem stems from false pride. Be proud of your ability to learn, not only your ability to know.
Noon Silk
The smartest people are the ones most able to accept finding out they are wrong. find out where you are wrong, then make it right. But self-conscious "slow" folks avoid anything that makes them feel wrong;it hurts too much. this is just an error in thinking. They dont' see that it causes them to be stupid. Fear of stupidity creates more stupidity.
Alex Baranosky
I used this technique for quite some time, always making sure one of my colleges reviews my code and than making corrections. Everyone *loved* giving the reviews and I was sure very soon they would want their code reviewed as well. I couldn't have been more wrong.
gkdm
+5  A: 

When I'm managing a project, if I say we are going to do code reviews, we are going to do code reviews. I will of course supply good reasons for doing them, but at the end of the day it is my responsibility and decision to implement them.

And if the programmers don't like it, they can find alternative employment. Most failing projects can usefully be improved by getting rid of half the developers anyway, and in my experience, it's the poorest developers that object to code reviews the most.

anon
+5  A: 

I've made good experiences with pair reviews. Two people are examining each other's source code while both are sitting in front of the same monitor. We make sure that it's not always the same people and that it's clear and agreed upon in advance what they both are looking for.

But still - doing code reviews requires open minded people who are willing to accept criticism.

Benedikt Eger
Alternatively it requires a development lead or a manager who won't let their employees push them around.
wlashell
A: 

That depends extremely on the programmer. I had loads of code reviews and some just ignore whatever you say - sometimes because they do not understand, but mostly because they simply ignore advice - especially when it's being pushed on them. It often looks like "they write bad code" (which they more often then less do - sigh) and they take that personally - even if it is not meant that way (positive ctiticism).

Interesting enough, the best way I found to get people to write cleaner code is to introduce automatic code inspection tools on check-in. So when their code doesn't meet certain standards (i.e. test-cases, comments, unsave code blocks), the commit is rejected. There are still some people who fall through the loops and write bogus, but it helped more than it hurt (in my experience).

Niko
Automatic code inspection tools are there to be coded around. There is no insight gained.
+5  A: 

"It works" is not enough. Code is not only written for machines to execute but also for people to read, adapt, and improve. If people can't find their way through code, they can't do all these tasks that are a mayor part of programming.

If Joe Programmer says: "Don't look at my code, I'm the only person who needs to understand it" I'd doubt this. Only in rare cases is code every touched by only one person. And even if this is the case: does Joe understand his own code next week, next month, next yeare? Probably not.

If somebody says "I'm the only person who needs to understand it" the code definitely needs a review ;-)
Benedikt Eger
If somebody says "I'm the only person who needs to understand it" then that somebody needs to get acquainted with the phone numbers of employment agencies.
Mike Woodhouse
Even if only one person *needs* to understand the code, most developers *should* be able to understand it. My favorite quote comes to mind: *"Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it." - Brian Kernighan*
Guffa
+4  A: 

Having a higher authority instate a review process should work. Even those who rebel against the idea at first should hopefully come around to see its benefits. If they don't... well, can't really help that.

If you're just a lone fighter among disinterested peers you'll have a much harder time. The key in both scenarios though is to make the reviewees understand that it's not a personal attack on their skills, but rather a team effort to squash everyones bugs more efficiently.

deceze
+6  A: 

The only reason I bought into the code review process was that a year or so ago the person that tried to impose them pulled the superiority card out and made us review each other's code for a given project. By the end of that project I could see the enormous benefit and I now try to instate code reviews on my projects.

James Hay
+2  A: 

We use ReviewBoard to do reviews. At our company, reviews are voluntary: if you're doing anything of interest (substantial additions or changes) we expect you to create a review-request to be done by the collegues in your team. The selected team members can then approve the review or request further changes.

If you have a need to force reviews because a programmer isn't playing along, one might require all commits to the SCM to come with a review ID that is checked to be marked as approved before it is accepted.

MathieuK
+3  A: 

The best way is for the decision to come from the programmers that make it.

I would suggest to use some people skills and start with a discussion on how to make sure that the group can work together, I would also talk about ways to make sure that the workplace skills of the programmers stay up to date.

One way forward is of course to do code reviews, but there is also other avenues that might be successful, for example reading and discussing a book about how to program. I recommend "Clean Code: A Handbook of Agile Software Craftsmanship" ( http://www.amazon.com/Clean-Code-Handbook-Software-Craftsmanship/dp/0132350882 ).

Another thing is letting each programmer present her/his projects in front of the group when they have finished doing something. That will naturally make sure that the ugliest things don't get written, and everyone gets to feel proud that they have accomplished something.

Alexander Kjäll
+16  A: 

Simple: assign the important projects to programmers who do accept code reviews. That is a clear message: this project is too important to leave to amateurs.

The programmers who won't cooperate can do maintenance work. Your company will have plenty of that, if code reviews weren't widely done. Make it clear that this is a dead-end street. These products will decrease in importance, be dropped, or be replaced by newer products that were code reviewed.

Make it clear when hiring that code reviews are the norm in your company. Assign the new hires that do accept code reviews to the new projects.

Overall, this will tell your developers that the company is adapting code reviews. They can choose: go along or get out. There is no future in any company for people fighting the company.

MSalters
Developers who avoid code/design reviews tend to be awful at maintenance also.
sal
I don't think this is a clear message at all. People who don't accept code reviews also don't self-review enough to realize that they've been sidelined and that it's a result of their own actions. At most they'll realize they have been sidelined but they won't realize why, and they will simply feel persecuted. They might leave the company in this case, which might be a good thing, but if that's the goal it seems like it would be quicker to just fire them outright.
Imagist
SO is a global site, firing people is a lot harder outside the USA. Assigning less competent employees to less critical projects OTOH is widely accepted. As for employees not realizing why they're reassigned, it's mgmt's responsibility to clearly communicate. This is no exception. Developers should not need a "self-review" to find out why mgmt does what it does.
MSalters
+2  A: 

I never had a chance to be involved in code review, but I think it is surely an excellent practice.
For code review to be well accepted, you could have some training organised on how to make "positive criticism".

And, by the way, such a training could also be an opportunity to remind people the company/project development principles and do some team building.

iDevlop
+4  A: 

In almost every commercial establishment (and most non-commercial ones, come to that) programmers do not own the code they write, their employer does.

In almost every commercial establishment (and most non-commercial ones, come to that) most programmers seem to think they do own the code they write.

This causes an awful lot of problems with acceptance of code review as a design improvement technique...

Ways to reduce resistance might include:

  • introducing pair-programming: no code gets written unless two programmers are looking at it. Instant code review in real-time. Make sure the pairs switch round regularly.
  • find a way to "anonymize" code reviews, so that it can't be personal
  • review in small chunks, so that no-one is faced with long lists of improvements
  • don't allow any code to be checked-in until it has been reviewed (and action points implemented and reviewed)
  • introduce a discretionary component to pay, dependent on acceptance of code reviews

I'm not recommending any (well, maybe the first) of the above specifically.

What matters is to get a real understanding of what the real objections to reviews are, not what you're hearing on the surface. Maybe apply 5 Whys to get deeper?

Mike Woodhouse
+13  A: 

I am programmer and I enjoy code reviews. The key to good code reviews in my experience is making code better.

When I do a code review with a colleague and he/she points out potential improvements or lurking bugs, I am happy. The code improves and we probably both get a little smarter. Why would I not like that?

Unfortunately some people see code reviews as a way to stress the fact that your code must have 4 space indentations instead of 3 space indentation or some other non-vital detail.

Make sure people who do code reviews can and will give useful information. If the only input code reviews provide is something which can easily be accomplished by using a tool, then code reviews are a waste of time.

Make sure code reviews are not a waste of time, and most developers will enjoy them.

Brian Rasmussen
"The code improves and we probably both get a little smarter". I totally agree. I have learned a lot of things from code reviews. If my code was reviewed I could see what I did wrong, learn from that and tried hard to avoid it in the future. But I also have learned a lot from performing code reviews, because I saw better ways of doing (my) things. My experience increased by “stealing” skills from the others (I can’t imagine how many books I should have read or how time should I've been practicing to gain the same skills that I have learned from others better than me).
dpb
+1 the most important argument: a good way to learn from each other!
Emile Vrijdags
+5  A: 

I don't like code review when I am a developer.

But I really want to code review if I am a leader. We know why.

However, some developers will be sure to don't like code review, just like me.

How about tring anonymous code review?

We can write example code pieces or tips after code review to figure out what is better way.

A simple example:

original piece:

s=objGotFromSomewhere.doSomething()

better piece:

if(objGotFromSomewhere!=null){
      s=objGotFromSomewhere.doSomething()
  }

memo:

In some project , we got ... issue, caused by .. reason

tips:

always check ...

We can put this into wiki or somewhere , then talk about it in the weekly team meeting. Tech Leader will do it, and encourage senior developers and every team member to do it too.

I think, anonymous is important, that means no one knows who generated the bad code.

People may don't like code review, but will like to know what is better way.

chiesa
Anonymous reviews only work in large teams where most programmers don't know each other. If you're part of a small team (5 programmers) and you have to review team code, then it's often easy to check the style and thus detect who wrote it.
Workshop Alex
I disagree about the "anonymous is important" bit. Part of the whole point of code reviews is acknowledging that everyone makes mistakes, so nobody should have a problem if they catch someone making a mistake. On the other hand, if someone is making a lot of mistakes and they are making it into the code base, that person is a liability. Wouldn't it be better to identify people who are liabilities rather than allow them to hide behind anonymity?
Imagist
"it's often easy to check the style and thus detect who wrote it". Yes, I agree. But because the name is not figured out, the developer will accept it easier.
chiesa
+2  A: 

If you are in charge of the project from the start or are the lead developer, you should be able to conduct code reviews by instituting them as part of your project methodology. Make sure the reviews are performed by leading them yourself, if necessary. If you are in charge and some people won't follow your lead, then you need to either persuade them to get with the program or replace them with people who will.

If you are not in charge, then you have to persuade the team members that it is a benefit to them to perform code reviews. Start by persuading the other team members to review your code. Document your design and interfaces and walk them through your code. Then write up a post review summary and action items for yourself and distribute it to your team members. If there is a benefit from this, it will become quickly apparent to the team.

If there is resistance, you need to find out what people's objections are. They might be valid. For example, the project might be small enough and the entire team might be experienced enough that everyone on the project effectively peer reviews all the code in the project even without formal reviews.

I find code reviews most useful on very large projects where it is not possible for one person to really grok all the code, where there are inexperienced developers who need the feedback in order to develop their coding and analytical skills or where there are non-coding members of the team who must understand the design and the code. I find reviews essential or even mandatory when there are legal consequences or worse if the code fails (e.g. aircraft flight control systems). I find them redundant when working with seasoned programmers, where everyone on the team produces code, where the project is small and the team members read and critique the code all the time anyway.

Jeff Leonard
+3  A: 

Personally, I think a programmer who dislikes a code review is just a very bad programmer. If you write good code, you have nothing to fear from a review. You might actually be praised about your style. But if you make a habit of writing bad code then I can imagine why someone would dislike a review. But having someone else point out the errors in your style is just useful to improve your own experience! You should learn from a review, not shy away from it. It's an enormous boost to code quality.

When you do code reviews, make clear that you're doing this to improve code quality, not to punish the bad developers. Make sure your programmers will consider these reviews as educational, not as a way to measure their performance.

I've had plenty of code reviews and I've specialized in complex algorithms thus my code tends to look a bit complex for others. Fellow programmers do consider me to be very good at what I do and just love to review my code, because of it's educational value. And of course, occasionally they do find some small errors in my code but in general these code reviews are improving the quality of work of the person who reviews the code!

Workshop Alex
What if the person "leading" the code review is a complete moron? Been there. Some (many?) people get ahead by smooching up to their incompetent boss.
xcramps
A code review is always something that will lead to discussions but a person who reviews code should not be the one who also changes it! I've been there too, where a moron didn't like my work and wanted me to rewrite it according to his extremely bad design. I just refused to make those changes and told him he's free to make those changes, but he'd also be responsible for all the errors afterwards! He backed oof quite quickly. (And yes, I also told him I thought he was a moron to make such stupid suggestions.) (He was later kicked out for incompetence...)
Workshop Alex
+2  A: 

Don't always assume the problem is at their end. Maybe you're doing it wrong.

Find out why they don't like it and instead of automatically assuming that they are a 'bad programmer' firstly consider if they may have a point and that you may be a bad reviewer of code, or simply communicating badly. It's possible.

If your code review / style rules are not subjective, arbitrary or insane, then it will be easy to explain the rationale for them to any competent, professional programmer. If you can't, then maybe you need to rethink either what you are doing or how you are presenting it.

Otherwise, if the problem is with just one or two individuals, it is likely more of a deeper HR problem with those people and nothing to do with code review specifically, the code review thing is just a symptom of that bigger problem.

frankodwyer
A: 

You need to accept that you can't "make" people accept code review.

You can make a compiler do what you want, but with humans, the best you can aim for is:

"How can I be more influential when reviewing another person's code?"

...and that's another question entirely.

Leon Bambrick
OK, so how can you be more influential when reviewing another person's code?
dpb
@Leon: Are you saying that "making people accept things" is impossible? If so, how do universities "make" students accept attending tests?
yairchu
+1 You can make people do certain things by making their lives difficult if they don't, but you can't control what people think (at least with current technology).
Imagist
A: 

How do you make people accept your code review?

You can't, so you don't. And you're not responsible for anyone else's work until you're asked to take responsibility for it.

You could approach your 'review' so it's more like 'feedback.' It's not you being the boss looking for things someone else did wrong, it's two professionals discussing different approaches to the same problem.

IMO, any two programmers will come up with different ways to solve a problem, and they can both be right.

Ask the reviewee, 'What do you see as the advantage of taking this approach?' Also provide the disadvantages you see and the advantages of your alternative approach. Ask for acknowledgement of the DA's you've pointed out- 'do you agree this could be a problem with the code you have?'

I think most programming conflicts come from value differences and guessing what might happen in the future. People can spend hours arguing over something completely indeterminate. Focus on real problems now, less on (but not avoiding) things which could happen later.

Beth
@Beth: How do you know he is not responsible for someone else's work?
yairchu
A: 

One more option: We hold periodic "reflections" on our process with all the programmers on a project. Part of this practice is to do a "root cause analysis" on the bugs that have surfaced recently. What caused this? Was this a regression? Was this a lack of IE6 testing? Would asking Joe about this helped? The goal being to figure out why we are having those bugs and what we can change to prevent them.

If code reviews are an appropriate tool, this will come out during the discussion. And it will be the programmers themselves who introduce it, and so you will get much more buy-in from the beginning. We agreed that all code needs to be reviewed before it's checked in. Developers generally ask for it, but if they forget, they sheepishly admit it... they know they aren't just defying "management", but going against the team's agreement.

(Or if it's not an appropriate tool, people get a chance to explain why.)

ndp
+1  A: 

I think the biggest problem with code reviews are that some people can't do them right. For example putting final in front of class/method/parameter/anything doesn't matter and should not be in a code review. Another example is using the wrong loop since only foreach is ok. Require to comment everything. String +/StringBuilder/StringBuffer optimizations. If I got a code review with any of those things I would think that guy is an idiot and if I was not forced to change the code I would not.

Also sometimes code review tools are just wrong, for example default PMD (Java) likes to say that class has too many methods, so what I did is to create abstract parent class and push some methods there. I could have split the class into 2 classes, but I think the API will be easier to use with all those method in one class (it's a small lib and my design decision). Some people will always use just the default, so it should not be crappy.

martin
A: 

In my experience, pair programming has far more benefits than just reviews. Not only does it find more bugs, but it encourages more creative solutions, knowledge sharing, team building, open communication, and allows a team to continuously refine a natural programming style.

Every time, I've been on a team that tried reviews, it gets confrontational, and then fizzles out when a tough deadline looms.

It can be hard for a seasoned developer to adjust to the idea of reviews but when it clicks the momentum really picks up. Try it for a couple weeks and see if it works for you.

Eric Nicholson
A: 

Try to sell them on the learning aspect. Almost every programmer I've met enjoys learning, and I personally have learned something at every code review (whether I was the reviewer or reviewee).

Once you start the reviews, however, it's your job to ensure they are worthwhile. Don't get bogged down in petty formatting arguments (take those offline). Use static analysis tools (like FindBugs for Java), search for TODOs, review every compiler warning, check documentation for completeness, etc.

The more valuable insight found in the review, the more successful the reviews will be.

Brad Cupit
A: 

Code reviews are an essential part of preventing a major disaster early on. A lot of the major development shops REQUIRE code reviews on their projects.

To make it easier on people to accept there should be guidelines in place for the reviewer to follow. If the individual feels like they're being singled out or that the reviewer could have a grudge they may be reluctant to go through a review. If ahead of time everyone is informed of the guidelines and what the reviewer will be looking for, you may have better reception. No one likes subjectivity when it reflects on their career or performance.

Speaking from experience, I've worked with a number of developers on high profile government contracts who were anti-code review. Where did that get us? Way behind schedule and way over budget. Developers with something to hide are going to be resistant to people going through their work as they're well aware of their short comings.

It's been my experience that those who aren't willing or are resistant to reviews are also the same people who aren't willing to learn and adapt their style of thinking to the problem at hand which can be a slippery slope for a project when the person is in a decision making role.

Something else to think about; if the budget and company can make it available have someone who is tasked solely with code reviews or even bring someone over from another project to do the review. If there is no prior relationship, it may be easier on both parties.

If all else fails and you're concerned the persons attitude may be creating problems for the project overall, document it, and escalate the issue to a superior in a respectful manner. Going after someone or pointing out their short comings is going to look bad for you and may draw attention to your own work.

Doomspork
A: 

First off, as with all problems, start solving this one yourself. Make sure you are doing everything to make code reviews as productive and non-confrontational as possible.

  1. Don't use code reviews to enforce best practices when you can use automation of some kind. Have standard libraries at hand to accomplish most common tasks: encryption, database access, etc. Use AOP and policy injection to take care of standardized logging, auditing, security constraints, etc. Rather than confronting developers and browbeating them into doing it the "right" way, just make it easier to do. We're lazy, so we'll just take the path of least resistance.
  2. Don't review for variable names, standardized comment blocks and other nitpicky details. Unless the variable name is difficult to understand, it's fine. "Consistency" is just an excuse to exercise authority and everyone can see through it.
  3. Don't have the grouchiest guy in your group do the reviews. Or, take care that you're not having people with personality conflicts reviewing each other's code.
  4. Have snacks and drinks at each code review. Everyone likes snacks and drinks.
Chris McCall
+1  A: 

Review with questions, not commands

I think a fundamental problem with your review process is your idea that you need to force anything down their throats. Instead, ask questions like "Why did you decide to go with X approach instead of Y?"

This will make them think about their own coding process, instead of forcing them to accept your coding process. And hey, you might learn something too.

Sean
+2  A: 

The 1-3-2 principle applies here (I speak 1st person, then 3rd person, then 2nd -- in that order).

First priority is to practice what I preach, meaning I am first concerned to say "I" need code reviews (and doing them).

Then only if the atmosphere isn't self-adapting to code reviews, I start to say "people" need code reviews (and list the good reasons).

Only after that, if all else fails (and I'm in authority :) ), I lay down the law and say "you" need code reviews. This is the part that people have correctly enumerated on, but I twitch only when the 3rd person mentality is not last. If I fail to speak 1rst person fist and don't practice what I preach, not only am I an "ineffective" persuader, but I'm just that much more a jerk.

ps. Notice how I said all of that in first person :)

Thr4wn
That's a pretty interesting method.
Jeff Davis
This is not getting enough upvotes. Try switching to 3rd person. :)
Jeremy Stein
+2  A: 

To offer up another perspective, I have sometimes discovered that code reviews drift into things that do not make sense to even discuss. Like I had this guy reviewing my code saying that I don't have the correct punctuation (a stop) in one of the comments I added to the code. While it is subjective whether such a thing is important (if you are writing inline documentation that will get pulled into released docs for example, it might be important), in my case it was clearly not.

I think code reviews are mandatory, I have almost always been helped by code review comments, and it sometimes takes a bit of time for a new team member to understand why they are important. But it is also important that objectivity is maintained, every suggestion should have an objective reason, and should not be made because of subjective preferences of a reviewer.

So to answer the question - it is good to mentor new team members on the importance and purpose of code review, at a stage where they are likely to listen. And with experience, they will learn that it is not personal. Also it is important to have the right people be reviewers, not people who impose their subjective opinions on others.

alps123
A: 

I have a different situation. Since I'm not a very experienced developer, I would like to do code review for my code before every release. But most of the time, code review just doesn't fit into our tight schedule. Or, my company doesn't think it's important enough to make the case. Same for test-driven development. If I really have questions, I'll grab a senior developer to look at my code. Not ideal but doing so does help me improving my coding skill.

logoin
A: 

Make sure it's easy to discover new code. Preferably, notify developers when code changes - don't count on developers to look for changed code. In a reasonably sized team automatically sending SVN diffs on commit is a great way of achieving this. Together with the Colored diffs addon for Thunderbird code-reviews are a breeze. I read most of the code committed (we are six developers on my team). Not all developers will necessary read commits by doing this, but it will become really convenient for those who are motivated to do so.

larsm
A: 

If they won't do code reviews, then simple... just lay them off. Clearly they aren't too concerned with improving anyway. There's no need to keep dead weight around.

Alex Baranosky