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.

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 its not A person who is failing or approving the checkin. They are mearly 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 dont like the rules have some meeting to change them.

How to find positive things in a code review?

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. Your job is to find problems and get them fixed before they affect customers.

A commenter writes:

"The developer who intimidates co-workers to the extent that they don't point out his/her errors should not be doing the coding."

A developer who intimidates their coworker 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.

It raises the bar to praise your fellow developers when they meet higher standards. So that means the main question is a good one, and worth addressing.

You can point out where the code meets various aspects of higher levels of coding. I would look for whether they follow best practices.

Language specific best practices

If the language supports docstrings, namespaces, OOP or functional features, you can call those out and congratulate the author on using them - where appropriate.

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?

Functional programming:

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

OOP

• encapsulation - provides a clean interface, and hides the details
• inheritance - code reused appropriately, perhaps mixins
• polymorphism - interfaces are defined, perhaps abstract base classes

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

Conclusion

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

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.

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.

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.

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.

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.

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.

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.

Your job is not to be nice. Your job is to find flaws in the code and point them out so that the overall code quality can be improved.

This being said, explain to your team the fact that a code review is just that, is not about the person nor about his skills, it's just about some flaws in the code (see egoless programming).

Also, make it clear what is the quality standard you're targeting for, such that people know what to expect (from my experience, if people are aware of the desired code quality they tend to write better code). You can even share your reviewer checklists such that people can do a quick pre-review when they finish their job.

When facing the "we always did it that way" arguments, explain that even if you know that it's a common practice in the company/team, it is better to do things other ways because (insert good arguments). You can also add explanations in the code reviews on why the things they've done are not as good as you're proposed solution.

But most importantly, make them understand it's not about them. Be a friendly person when interacting to them, so that they see that you don't have a grudge against them and you're just helping them write better code.

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.

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.

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.

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.