Code Reviews – How to Identify Positive Aspects

code-reviews

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.

Best Answer

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.

Related Topic