How to find positive things in a code review?

Question

After some serious quality problems in the last year, my company has recently introduced code reviews. The code review process was quickly introduced, without guidelines or any kind of checklist.

Another developer and I where chosen to review all changes made to the systems, before they are merged into the trunk.

We were also chosen as "Technical Lead". This means we are responsible for code quality, but we don't have any authority to implement changes in the process, reassign developers, or hold back projects.

Technically we can deny the merge, giving it back to development. In reality this ends almost always with our boss demanding that it be shipped on time.

Our manager is an MBA who is mostly concerned with creating a schedule of upcoming projects. While he is trying, he has almost no idea what our software does from a business point of view, and is struggling to understand even the most basic customer demands without explanation from a developer.

Currently development is done in development branches in SVN, after the developer thinks he is ready, he reassigns the ticket in our ticketing system to our manager. The manager then assigns it to us.

The code reviews have lead to some tensions within our team. Especially some of the older members question the changes (I.e. "We always did it like this" or "Why should the method have a sensible name, I know what it does?").

After the first few weeks my colleague started to let things slide, to not cause trouble with the co-workers (she told me herself, that after a bug report was filed by a customer, that she knew of the bug, but feared that the developer would be mad at her for pointing it out).

I, on the other hand, am now known for being an ass for pointing out problems with the committed code.

I don't think that my standards are too high.

My checklist at the moment is:

• The code will compile.
• There is at least one way the code will work.
• The code will work with most normal cases.
• The code will work with most edge cases.
• The code will throw reasonable exception if inserted data is not valid.

But I fully accept the responsibility of the way I give feedback. I'm already giving actionable points explaining why something should be changed, sometimes even just asking why something was implemented in a specific way. When I think it is bad, I point out that I would have developed it in another way.

What I'm lacking is the ability to find something to point out as "good". I read that one should try to sandwich bad news in good news.

But I'm having a hard time to find something that is good. "Hey this time you actually committed everything you did" is more condescending than nice or helpful.

Example Code Review

Hey Joe,

I have some questions about your changes in the Library\ACME\ExtractOrderMail Class.

I didn't understand why you marked "TempFilesToDelete" as static? At the moment a second call to "GetMails" would throw an exception, because you add Files to it but never remove them, after you deleted them. I know that the the function is just called once per run, but in the future this might change. Could you just make it an instance variable, then we could have multiple objects in parallel.

... (Some other points that don't work)

Minor points:

• Why does "GetErrorMailBody" take an Exception as Parameter? Did I miss something? You are not throwing the exception, you just pass it along and call "ToString". Why is that?
• SaveAndSend Isn't a good name for the Method. This Method sends error mails if the processing of a mail went wrong. Could you rename it to "SendErrorMail" or something similar?
• Please don't just comment old code, delete it outright. We still have it in subversion.

Answer 1

How to find positive things in a code review?

After some serious quality problems in the last year, my company has recently introduced code reviews.

Great, you have a real opportunity to create value for your firm.

After the first few weeks my colleague started to let things slide, to not cause trouble with the co-workers (she told me herself, that after a bugreport was filed by a customer, that she knew of the bug, but feared that the developer would be mad at her for pointing it out).

Your coworker should not be doing code review if she can't handle telling developers what's wrong with their code. It's your job to find problems and get them fixed before they affect customers.

Likewise, a developer who intimidates coworkers is asking to be fired. I've felt intimidated after a code-review - I told my boss, and it was handled. Also, I like my job, so I kept up the feedback, positive and negative. As a reviewer, that's on me, not anyone else.

I, on the other hand, am now known for being an ass for pointing out problems with the committed code.

Well, that's unfortunate, you say you're being tactful. You can find more to praise, if you have more to look for.

Critique the code, not the author

You give an example:

I have some questions about your changes in

Avoid using the words "you" and "your", say, "the" changes instead.

Did I miss something? [...] Why is that?

Don't add rhetorical flourishes to your critiques. Don't make jokes, either. There's a rule I've heard, "If it makes you feel good to say, don't say it, it's no good."

Maybe you're buffing your own ego at someone else's expense. Keep it to just the facts.

Raise the bar by giving positive feedback

It raises the bar to praise your fellow developers when they meet higher standards. So that means the question,

How to find positive things in a code review?

is a good one, and worth addressing.

You can point out where the code meets ideals of higher level coding practices.

Look for them to follow best practices, and to keep raising the bar. After the easier ideals become expected of everyone, you'll want to stop praising these and look for even better coding practices for praise.

Language specific best practices

If the language supports documentation in code, namespaces, object-oriented or functional programming features, you can call those out and congratulate the author on using them where appropriate. These matters usually fall under style-guides:

• Does it meet in-house language style guide standards?
• Does it meet the most authoritative style guide for the language (which is probably more strict than in-house - and thus still compliant with the in-house style)?

Generic best practices

You could find points to praise on generic coding principles, under various paradigms. For example, do they have good unittests? Do the unittests cover most of the code?

Look for:

• unit tests that test only the subject functionality - mocking expensive functionality that is not intended to be tested.
• high levels of code coverage, with complete testing of APIs and semantically public functionality.
• acceptance tests and smoke tests that test end-to-end functionality, including functionality that is mocked for unit tests.
• good naming, canonical data points so code is DRY (Don't Repeat Yourself), no magic strings or numbers.
• variable naming so well done that comments are largely redundant.
• cleanups, objective improvements (without tradeoffs), and appropriate refactorings that reduce lines of code and technical debt without making the code completely foreign to the original writers.

Functional Programming

If the language is functional, or supports the functional paradigm, look for these ideals:

• avoiding globals and global state
• using closures and partial functions
• small functions with readable, correct, and descriptive names
• single exit points, minimizing number of arguments

Object Oriented Programming (OOP)

If the language supports OOP, you can praise the appropriate usage of these features:

• encapsulation - provides a cleanly defined and small public interface, and hides the details.
• inheritance - code reused appropriately, perhaps through mixins.
• polymorphism - interfaces are defined, perhaps abstract base classes, functions written to support parametric polymorphism.

under OOP, there are also SOLID principles (maybe some redundancy to OOP features):

• single responsibility - each object has one stakeholder/owner
• open/closed - not modifying the interface of established objects
• Liskov substitution - subclasses can be substituted for instances of parents
• interface segregation - interfaces provided by composition, perhaps mixins
• dependency inversion - interfaces defined - polymorphism...

Unix programming principles:

Unix principles are modularity, clarity, composition, separation, simplicity, parsimony, transparency, robustness, representation, least surprise, silence, repair, economy, generation, optimization, diversity, and extensibility.

In general, these principles can be applied under many paradigms.

Your criteria

These are far too trivial - I would feel condescended to if praised for this:

• The code will compile.
• There is at least one way the code will work.
• The code will work with most normal cases.

On the other hand, these are fairly high praise, considering what you seem to be dealing with, and I wouldn't hesitate to praise developers for doing this:

• The code will work with most edge cases.
• The code will throw reasonable exception if inserted data is not valid.

Writing down rules for passing code review?

That's a great idea in theory, however, while I wouldn't usually reject code for bad naming, I've seen naming so bad that I would reject the code with instructions to fix it. You need to be able to reject the code for any reason.

The only rule I can think of for rejecting code is there's nothing so egregious that I would keep it out of production. A really bad name is something that I would be willing to keep out of production - but you can't make that a rule.

Conclusion

You can praise best practices being followed under multiple paradigms, and probably under all of them, if the language supports them.

Answer 2

Don't bother picking something good unless its a solid concise example and is directly related to the focused issue.

I won't sugar coat it - from the sounds of it you are dealing with at least one person who is insecure with their abilities and is handling being challenged about their work in an immature way. They are also likely bad at their job - a good developer should always be willing to self reflect, take constructive criticism, and be open to changing their ways.

Now that that's out in the air lets talk about you. Regardless of if you think you are being reasonable you need to be extra careful with people like this to get the ball rolling. I've found the best way to deal with these people is to be very careful with how you word things.

Make sure that you are blaming the code and not the author. Focus on the one issue at hand rather than the poo-mountain that is your code base, which they may have had a significant hand in creating and would be seen as a further personal attack. Choose your battles initially, focus on critical issues that manifest themselves to your users so that you aren't launching a volley of criticisms at the person leading them to dismiss everything you are saying.

Body language and tone are important if you are talking to them in person, be clear with what you are saying and make sure you aren't talking down to them or dismissing their technical abilities. They'll most likely be on the defensive right off the bat so you need to settle their worries instead of confirming them. You need to be conscious about this conversation without being too obvious so that they subconsciously think you are on their side, and hopefully they accept that they need to make the changes that were brought to attention.

If this doesn't work you can start getting a bit more aggressive. If the product is runnable from a conference room, bring it up on the projector during the code review and show the bug first hand, its better if a manager is right there so the person can't back down. This isn't to shame them but to force them to accept that the problem is real for the user and that it needs to be fixed, instead of just a gripe you have with their code.

Eventually if you aren't getting anywhere, are tired of treating the person like a kindergarten student, and management is completely unaware of the problem, and it either reflects poorly on your performance as a code reviewer or you care about the well being of your company and/or product, you need to start talking to your boss about their behavior. Be as specific and impersonal as possible - make a business case that productivity and quality are suffering.

Answer 3

Code reviews can be toxic, time wasting, will to live-sapping nerd wars. Just look at the divergence of opinion on things like clean code vs comments, naming conventions, unit and integration testing, check in strategies, RESTfulness, etc., etc.

The only way to ensure you avoid this is to write down the rules for passing a code review.

Then it's not a person who is failing or approving the checkin. They are merely checking that the rules have been followed.

And because they are written down in advance, when you write your code you can follow the rules and not have to explain your reasoning or have arguments later.

If you don't like the rules, have a meeting to change them.

Answer 4

I would not sugar coat your feedback because it may be considered as patronising.

In my opinion, the best practice is to always focus on the code and never on the author.

It's a code review, not a developer review, so:

• "This while loop may never finish", not "Your loop may never finish"
• "A test case is needed for scenario X", not "You didn't write the test to cover this scenario"

It is also very important to apply the same rule while talking about the review with others:

• "Anne, what do you think about this code?", not "Anne, what do you think about Jon's code?"

Code review is not the time for a performance review - it should be done separately.

Answer 5

I'm surprised it hasn't been mentioned in any answer so far, and perhaps my experience with code reviews is the unusual one, but:

Why are you reviewing the entire merge request in a single message?

My experience with code reviews is via GitLab; I always imagined that other code review tools would work similarly.

When I am giving a code review, I comment on specific, individual lines of the diff. This is extremely unlikely to be received as personal criticism, because it is commentary on the code—and is actually displayed as commentary on the code, shown directly below the code it is a commentary on.

I can also comment on the entire merge request, and often do. But specific points can be pointed out on specific lines of the diff. (Also, when a new commit is added such that the diff changes, the comments on the "outdated diff" are hidden by default.)

With this workflow, code reviews become much more modular and less cohesive. A line from a code review can simply say,

Nice approach, wrapping this in a dedicated function!

Or it might say,

This object name doesn't really match the purpose of the object; maybe we could use a name like 'XYZ' instead?

Or if there are major problems with a section, I might write:

I see that this code works (and I see why it works) but it requires focused study to understand it. Could you please refactor it into separate functions so it will be easier to maintain in the future?

(Example: Function ABC is actually doing three things here: bazzing the foo, barring the boz and fripping the zorf. Those could all be separate functions.)

After writing all of the above, I can make a summary comment on the entire merge request, something like:

This is a great new feature and it will be really useful once merged. Can you please clean up the function naming, and handle the refactoring mentioned in the individual comments I've made, and then let me know to look at it again? :)

---

Even if the merge request is a complete dog's breakfast, the individual comments can each be simple. There will just be more of them. Then the summary comment might say:

I'm sorry, but this code isn't really up to snuff. There are a great many edge cases (as detailed in individual comments) that will be handled incorrectly and give a bad user experience, or even data corruption in one case. (See comment on commit 438a95fb734.) Even some normal use cases will result in extremely bad application performance (specifics noted in individual comments on the diff for somefile.c).

To be production ready this feature will need a full rewrite with more attention given to what assumptions are safe to make at every point in the flow. (Hint: No assumptions are safe unless they've been checked for.)

I'm closing the merge request pending a full rewrite.

---

Summary: Review the technical aspects of the code as comments on individual lines of code. Then summarize those comments in an overall comment on the merge request. Don't get personal—just deal in the facts, and in your opinion about the code, not about the coder. And base your opinion on facts, accurate observation, and understanding.

Answer 6

I am really surprised nobody picked that up, but there is something wrong with the sample review posted.

There is simply no reason you should address to Joe directly. That Joe fixes his failures is none of your business. That someone does, is your business. Your concern is code quality. So, instead of writing requests/orders/demands (that, if I were Joe, I could simply refuse because you are not legitimate for this), stay at your role and write a simple anonymous todo list.

Trying to be fair in giving good and bad points is not only impossible but completely out of your role.

Here are example of reformulation with content taken from your review:

• Before we pull changes in the Library\ACME\ExtractOrderMail Class we need to fix a few issues.
• Unless I missed something, "TempFilesToDelete" shouldn't be static.
• In the future, we may call the function more than once per run, this is why we need (what has to be done here).
• I need to understand why does "GetErrorMailBody" take an Exception as Parameter. (and, I'm being borderline here, because by now, you should already have a conclusion)
• SaveAndSend should be renamed to better fit its behavior, like for example to "SendErrorMail"
• Commented out code should be deleted for readability purpose. We use subversion for eventual rollbacks.

If you formulate the review like this, no matter how much the reader hates you personally, all he can see here is notes on improvements that someone has to carry on later on. Who ? When ? Nobody cares. What ? Why ? THAT you should say.

Now, you will address the very reason code reviews are rising tension by taking human factor out of the equation.

Answer 7

The whole point of code review is to find problems. If there is any bug, the best thing you can do is write a test case that is failing.

Your team(manager) should communicate that producing bugs is part of the game, but finding and fixing them will save everybody's job.

It might be helpful to have regular meetings (either team or pair) and go through a couple of the issues. The original programmer didn't introduce problems intentionally, and sometimes he might think it was good enough (and sometimes it even might be). But having that second pair of eyes gives a completely fresh view, and he might learn greatly from looking at the problems.

It really is a cultural thing, and it needs lots of trust and communication. And time to work with the results.

Answer 8

I think the positive thing to do would be to get some consensus on coding standards before doing reviews. Others tend to buy in more to something when they have input.

For something like naming conventions, ask others if this is important. Most developers will agree especially among their peers. Who wants to be the person who doesn't want to agree with something so prevalent in the programming world? When you start the process by picking someone's code and pointing out the flaw, they get very defensive. When standards are established, there will be disagreement on the interpretation or what is considered "good enough," but you're better off than you are now.

Another part of this process is determining how code reviews will handle objections. This cannot become endless debate. At some point, someone has to make the decision. Maybe there can be a third-party to be the judge or the reviewer gets all the power. Stuff has to get done should be the goal.

The final piece of this is to let everyone know that Code Reviews were not your idea, they're going to stay, so everyone should make an effort to make the best of it. If everyone feels powerless, maybe they can be allowed to review your code?

Hopefully, some measurable result for management is to limit bugs, customer complaints, delays etc. Otherwise, someone just heard the buzzword "code review" and figured if they just add it to your process, miracles will occur without a lot of time, energy and effort put into this.

Answer 9

This may be harsh, but don't worry about giving good feedback if there's nothing good to measure.

However, in the future, as your developers start to improve their code, that's when you'll want to give them good feedback. You'll want to point out the improvements in the code, and you'll also want to point out the benefits to the team as a whole. When you start seeing improvement, they will as well, and things should slowly start to come around.

Another thing; there may be a defensive air because they feel as though they don't have a say. Why not let them review each other's code? Ask them specific questions and try to get them involved. It shouldn't be you versus them; it should be a team effort.

1. What would you change about this code if you had the time?
2. How would you improve this area of the codebase?

Ask that now, and ask that six months from now. There's a learning experience in here.

The main point - don't give praise in terms of code where it isn't warranted, but recognize effort and definitely recognize improvement. Try to make reviews a team exercise instead of an adversarial one.

Answer 10

Quality without Tension

You’ve asked how you can find positive things to say about code, but your real question is how you can avoid “tensions within [your] team” while addressing “serious quality problems.”

The old trick of sandwiching “bad news in good news” may backfire. Developers are likely to see it for what it is: a contrivance.

Your Organizations Top-Down Troubles

Your bosses tasked you with ensuring quality. You came up with a list of criteria for code quality. Now, you want ideas for positive reinforcement to provide to your team.

Why ask us what you need to do make your team happy? Have you considered asking your team?

It sounds like you are doing your best to be nice. The problem is not how you are delivering your message. The problem is the communication has been one-way.

Building a Culture of Quality

A culture of quality has to be nurtured to grow from the bottom up.

Let your boss know you are concerned quality is not improving fast enough, and you want to try applying some advice from The Harvard Business Review.

Meet with your team. Model the behaviors you want to see in your team: humility, respect, and a commitment to improve.

Say something like: “As you know, [coworker] and I were tasked with ensuring code quality, to prevent the sort of production issues we’ve suffered recently. I personally don’t think I’ve solved this problem. I think my biggest mistake was not involving all of you more at the beginning. [coworker] and I are still accountable to management for code quality, but going forward, we are all in the quality effort together.”

Get ideas from the team about code quality. (A whiteboard would help.) Make sure your requirements get in there by the end. Offer your insights in respectful ways, and ask questions when appropriate. Be willing to be surprised about what your team feels is important. Be flexible, without compromising business needs.

(If you have some old code you’re embarrassed about, you might trot it out, encouraging everyone to be candid. At the end, let folks know you wrote it. Model the mature reaction you are hoping for when others receive construction criticism.)

Collaborative Code Reviews

I haven’t worked at a place like you describe, where a few senior programmers review all the code and make corrections. No wonder folks respond like you are a teacher making red marks on their papers.

If you can sell the idea to managment, start doing peer code reviews. This is best done in small groups, including you or the other accountable developer. Ensure everyone is treated with respect.

An important goal of peer reviewing code is to ensure the code can be understood by all members of the team. Consider your example of unclear function names: hearing from a more junior developer that they find the function name confusing can be easier to accept than another “correction” from the senior dev.

Quality is a Journey

One other thing to do is to expunge any notion that a code review is a pass/fail sort of thing. Everyone should expect to make some edits after a code review. (And your job is to ensure they happen.)

But if that doesn’t work...

Let’s assume you make some strides establishing a culture of quality. You still might not get everyone on board. If someone is still not with the quality program, now the problem is they are not fitting in with the team, rather than there being a problem between the two of you. If they need to be dismissed from the team, the rest of the team will better understand the reasons.

Answer 11

Sorry for yet another long answer, but I don't think the others have fully addressed the human element of this issue.

sometimes even just asking why something was implemented in a specific way. When I think it is bad, I point out that I would have developed it in another way.

The problem with "Why did you implement it this way?" is that you put the developer immediately on the defensive. The question itself can imply all sorts of things depending on context: Are you too stupid to think of a better solution? Is this the best you can do? Are you trying to ruin this project? Do you even care about the quality of your work? etc. Asking 'why' the code was developed a certain way is only going to be confrontational, and that subverts any pedagogical intent your comment might have had.

Similarly "I would have done this differently..." is also confrontational, because the immediate thought the developer will have is "Well I did it this way... You got a problem with it?" And again, it's more confrontational than it needs to be and turns the discussion away from improving the code.

Example:

Instead of asking "Why did you choose not to use the constant variable for this value?", simply state "This hard-coded value should be replaced with the constant XYZ in file Constants.h" Asking the question suggests that the developer actively chose not to use the already-defined constant, but it's entirely possible that they didn't even know it existed. With a large enough code base, there's going to be a lot of things each developer doesn't know. This is simply a good learning opportunity for that developer to get acquainted with the project's constants.

There's a fine line to walk with code reviews: you don't need to sugar coat everything you say, you don't need to sandwich bad news with good news, and you don't need to soften the blow. It could be the culture I'm in, but my coworkers and I never compliment each other in code reviews - the parts of the code we develop that are defect-free are enough of a compliment. What you need to do is identify the defect you see, and perhaps give a reason why (the why is less useful when it's obvious/simple).

Good:

• "The name of this variable needs to be changed to match our coding standard."

• "The 'b' in this function name needs to be capitalized in order to be PascalCase."

• "This function's code isn't indented properly."

• "This code is a duplicate of code found in ABC::XYZ(), and should use that function instead."

• "You should use try-with-resources so that things are guaranteed to be closed properly in this function, even if errors occur." [I only added a link here so non-java users would know what try-with-resources means]

• "This function needs to be refactored in order to meet our n-path complexity standards."

Bad:

• "I think you could improve this code by changing the variable name to match our standard"

• "Perhaps it would be better to use try-with-resource to properly close things in this function"

• "It might be desirable to take another look at all the conditions in this function. Its N-path complexity is over our standard's maximum allowed complexity."

• "Why are the indents here 2 spaces instead of our standard's 4?"

• "Why did you write a function that breaks our n-path complexity standard?"

In the bad statements the italicized parts are things that people commonly use when they want to soften the blow, but all it really does in a code review is make you sound uncertain of yourself. If you, the reviewer, aren't certain on how to improve the code, then why should anyone listen to you?

The 'good' statements are blunt, but they don't accuse the developer, they don't attack the developer, they're not confrontational, and they don't question why the defect exists. It exists; here's the fix. They're certainly not as confrontational as that last 'why' question.

Answer 12

I would respectfully disagree with the method of code review you've described. Two staffers going off into a 'closed room' and coming out with criticism institutionalizes the very kind of adversarial notion that good code reviews should be avoiding.

Making code review a positive experience to the extent possible is essential to making it successful. Step one is to remove the adversarial notion of review. Make it a weekly or bi-weekly group event; ensure everyone knows they are welcome to participate. Order in pizza or sandwiches or whatever. Don't even call it a 'code review' if that stirs up negative connotations. Find something to celebrate, to encourage, to share - even if it's nothing more than progress through the current sprint or iteration. Take turns assigning leadership over the review.

Make the process an endeavor to serve the product and the people. If they are done constructively, positively, wherein good techniques or patterns are shared and encouraged just as poor ones are discouraged - everyone benefits. Everyone gets coached in public. If you have a problem programmer who isn't "getting it," they should be addressed privately and separately - never in front of the broader group. That's separate from code review.

If you're having trouble finding something "good" to bring up, that to me begs a question: If progress is being made in the project, and people are working, that progress in and of itself is something to celebrate. If you're truly finding nothing good, that implies everything that's been done since the last review is either bad or no better than neutral. Is that really the case?

As for the details, common standards are essential. Give everyone a stake in what's being done. And allow for newer, better techniques to be integrated into your code base. The failure to do that guarantees old habits are never excised under the guise of "we've always done it that way."

The point in all this is that code reviews are given a bit of a bad rap because they are poorly implemented, used as hammers to belittle less experienced or less skilled programmers, and that is of service to no one. Managers who use them in this way aren't likely to be very effective managers, either. Good code reviews wherein everyone is a participant serves everyone - the product, the employees, and the company.

Apologies for the length of the post, but having been in a group where code review was largely ignored, it led to a legacy of unmaintainable code and only a narrow range of developers able even if allowed to make changes to a years-old codebase. It was a path I'd opt not to traverse again.

Answer 13

The problem that you see is: Developers are unhappy that their code is criticised. But that's not the problem. The problem is that their code is no good (assuming obviously that your code reviews are good).

If the developers don't like their code attracting criticism, then the solution is easy: Write better code. You say you had serious problems with code quality; that means better code is needed.

"Why should the method have a sensible name, I know what it does?" is something that I find particularly disturbing. He knows what it does, or at least he says so, but I don't. Any method should not just have a sensible name, it should have a name that makes it immediately clear to the reader of the code what it does. You might want to go to Apple's website and look for a WWDC video about the changes from Swift 2 to Swift 3 - a huge number of changes made just to make everything more readable. Maybe that kind of video could convince your developers that developers who are a lot smarter than them find intuitive method names to be very, very important.

Another disturbing item was your colleague who said "she told me herself, that after a bugreport was filed by a customer, that she knew of the bug, but feared that the developer would be mad at her for pointing it out". There is the possibility that there is some misunderstanding, but if a developer gets mad if you point out a bug to them, that is bad.

Answer 14

The paradox of good code is that it doesn't stand out at all, and looks like it was very straightforward and easy to write. I really like the pool player example from this answer. In code reviews, that makes it really easy to gloss over, unless it happens to be a refactor from bad code.

I make a point to look for it. If I'm reviewing code and I've gone through a file that was so easy to read that it seems deceptively easy to write, I just throw in a quick "I like how the methods here are short and clean" or whatever is appropriate.

Also, you should lead by example. You should insist that your code be reviewed too, and you should model how you want your team to behave when receiving correction. Most importantly, you should sincerely thank people for the feedback. This makes more difference than any unilateral feedback you can give.

Answer 15

It sounds like the real problem is that it is only two people who are doing all the code reviews, of which it is only you who take them seriously, which has ended you up in an unfortunate situation with a lot of responsibility and a lot of pressure from the other team members.

A better way would be to distribute the responsibility of doing the code reviews over the team, and have everyone take part in doing them, for example by letting the developers choose whom to review their code. This is something that your code review tool should support.

Answer 16

I've seen this happen first-hand and want to caution you away from the emotional intelligence challenged answers - they can kill entire teams. Unless you want to recruit, train and normalize new developers every year, creating a positive rapport with your peers is essential. After all, isn't the whole point of doing these reviews to improve code quality and foster a culture where the code quality is higher in the first place? I would argue that it is indeed part of your job to provide positive reinforcement as a means of integrating this code review system into the team culture.

Anyways, sounds like you need to review your Code Review system. Right now, from the sounds of it everything in your review process is, or can be interpreted as, subjective rather than objective. It is easy to get hurt feelings when it feels like someone is just picking apart your code because they don't like it, rather than them having a reason that they can cite when something doesn't fit the guidelines. This way, its easy to track and 'celebrate' positive improvements in code quality (with respect to your review system) in whatever way is appropriate for your office culture.

The Technical Leads need to get together outside of a review session and put out a Coding Guidelines/Code Review Checklist and it needs to be adhered to and referred to during the review. This should be a living document which can be updated and refined as the process develops. These Tech Lead meetings should also be when the 'Way we've always done things' vs. 'New and improved ways of doing things' discussion should happen, without the developer being reviewed feeling like the disagreement is a result of their code. Once the initial guideline has been more or less smoothed out, another way to positively reinforce the developers is to ask for their feedback and then actually take action on it and evolve the process as a team, this does wonders to bring them up to speed to begin to review code for onboards so you're not stuck doing reviews until you retire, too.

Answer 17

Premises...

Programmers are problem-solvers. We are conditioned to start debugging when presented with a problem or error.

Code is an outline-format medium. Switching to a paragraph-format narrative for feedback introduces a disconnect that must be translated. Inevitably something gets lost or misunderstood. Unavoidably, the reviewer is not speaking in the programmer's language.

Computer-generated error messages are seldom questioned and never taken as a personal affront.

Code-review items are human-generated error messages. They are intended to catch the edge-cases that programmers and automated tools miss as well as the inevitable side-effects on other programs and data.

Conclusions...

Thus, the more code-review items can be incorporated into automated tools, the better they will be received.

The caveat is that, to remain unquestioned, such tools must be continuously updated, usually daily or weekly, to be in compliance with every change to procedures, standards and practices. When a manager or developer team decides to make a change, tools must be changed to enforce that. Much of the code-reviewer's job then becomes maintaining the rules in the tools.

Example Code Review Feedback...

A rewrite of the given example incorporating these techniques:

• Subject:

  • Library\ACME\ExtractOrderMail Class.

• Principle issue...

  • TempFilesToDelete is static
    • Subsequent calls to GetMails throw an exception since files are added to it but never removed after deletion. Though only one call now, performance could be improved in the future with some parallelism.
    • TempFilesToDelete as an instance variable would allow multiple objects to be used in parallel.
• Secondary issues...
  • GetErrorMailBody has an exception parameter
    • Since it does not throw an exception itself, and just passes this to ToString, is it necessary?
  • SaveAndSend name
    • Email may or may not be used to report this in the future and this code contains the general logic for storing a persistent copy and reporting any errors. A more generic name would allow such anticipated changes without change to dependant methods. One possibility is StoreAndReport.
  • Commenting out old code
    • Leaving a line commented and marked OBSOLETE can be very helpful in debugging, but a "wall of comments" can also obscure errors in adjacent code. We still have it in Subversion. Perhaps just a comment referencing exactly where in Subversion?

Answer 18

If you don't have anything nice to say about the existing code (which sounds understandable), try to involve the developer in improving it.

In a patch for a functional change or bugfix, it's not helpful to have other changes (renaming, refactoring, whatever), bundled into the same patch. It should make the change that fixes the bug or adds the feature, nothing else.

Where renaming, refactoring and other changes are indicated, do them in a separate patch, before or after.

• This is pretty ugly, but I guess it's consistent with all our other nasty code. Let's clean that up later (in a patch with, ideally, no functional changes).

  It also helps if you join in refactoring, and let them review your code. It gives you a chance to say why you think something is good, rather than bad, and also to show an example of taking constructive criticism well.

If you need to add unit tests to a new feature, fine, they go in the main patch. But if you're fixing a bug, the tests should go in a prior patch and not pass so you can demonstrate the fix actually fixed the bug.

Ideally the plan (for how you can test a fix, or what to refactor and when) should be discussed before the work is done, so they haven't sunk a load of effort in something before finding out you have a problem with it.

If you find code doesn't cope with inputs you think it should, it may also be a mismatch in what the developer sees as reasonable input. Agree that in advance too, if possible. At least have some discussion about what it should be able to cope with and how nastily it can be allowed to fail.

Answer 19

I think it's a mistake to assume there is a technical or easy solution to: "my co-workers are upset when I judge their work by my standards, and have some power to enforce it".

Remember code reviews aren't just a discussion of what's good or bad. They are implicitly a "who's yard stick are we using", a "who makes the decision" and hence -most insidiously- a rank.

If you don't address those points, doing code reviews in a way that doesn't have the problems you encounter are going to be hard. There is some good advice here about how to do it, but you've set yourself a difficult task.

If you are right, without any wiggle room, pointing it out is an attack: someone messed up. If not: your another MBA who doesn't get it. Either way, your going to be the bad guy.

However, if the what the review is looking and why, is clear and agreed upon, you have a decent chance of not being the bad guy. You aren't the gatekeeper, just a proofreader. Getting this agreement is a hard problem to solve[1]. I'd like to give you advice, but I haven't got the hang of it myself yet...

[1] It's not solved just because people have said "ok", or stopped fighting about it. None want to be the guy to say X just isn't practical for our {intelligence, experience, man-power, deadlines, etc} but that doesn't mean when it actually comes to some specific instance of doing X...

Answer 20

Code reviews should be mutual. Everybody criticized everybody. Let the motto be "Don't get mad, get even." Everybody makes mistakes and once people have told a hot shot how to fix their code, they will understand that this just the normal procedure and not an attack on them.

Seeing other people's code is educational. Understanding the code to the point where you can point out its weak spot and having to explain that weakness can teach you a lot!

There is little room for praise in a code review, since the whole point is to find flaws. However, you can heap praise on fixes. Both timeliness and neatness can be praised.