Selfelected

My talk about how to make code review

The other year I held a talk about how to make code reviews that are of value.
As zip.

Förvänta dig inte att din Pull request går igenom

ola fjelddahl
göteborgskontoret

Förvänta dig inte att din Pull request går igenom

PRAISE: Nice refactoring!

ola fjelddahl
göteborgskontoret

Förvänta dig inte att din Pull request går igenom

PRAISE: Nice refactoring!
SUGGESTION: Split line.

ola fjelddahl
göteborgskontoret

Förvänta dig inte att din Pull request går igenom

PRAISE: Nice refactoring
SUGGESTION: Isn’t it an Article?, not a Device.
CHECK: Have you investigated all uses of the enum?

ola fjelddahl
göteborgskontoret

Förvänta dig inte att din Pull request går igenom

PRAISE: Nice refactoring!
SUGGESTION: Split line.
CHECK: Do you use the enum the correct way? Are all cases considered?
EXPLAIN: How do this work?

ola fjelddahl
göteborgskontoret

Förvänta dig inte att din Pull request går igenom

PRAISE: Nice refactoring!
SUGGESTION: Split line.
CHECK: Do you use the enum the correct way? Are all cases considered?
EXPLAIN: How do this work?
FIX: Check for null.

ola fjelddahl
göteborgskontoret

Förvänta dig inte att din Pull request går igenom

PRAISE: Nice refactoring!
SUGGESTION: Split line.
CHECK: Do you use the enum the correct way? Are all cases considered?
EXPLAIN: How do this work?
FIX: Check for null.
QUESTION: Is this a HasA or a IsA

ola fjelddahl
göteborgskontoret

Code reviewing comments

I am presently using TFS’s code reviewing tool and IMHO it lacks visualisation of which comments are important and which are not.

So I came up with the idea of prefixing my comments.

PRAISE: Good refactoring, I like how you remove the negation.

FIX: A null check is missing.

SUGGESTION: Split line.

CHECK: Is it the right enum used here?

EXPLAIN: Is this comparison really correct? or seems to overflow to me.

If there are many FIX but only one or a few are critical I prefix the critical once with RED, like RED FIX.
This gives the opportunity to “while you already are updating the module, do these things too”.

When code reviewing do not forget to give appraisal too

Update:

This is what I wrote at Patricia Aas’ show at Dotnetrocks:

I have learned that some don’t like code review because it is a critique of the code written. When failing a code review the task is halted for a rewrite and everyone loses.

I love it because to me it is a discussion about code and the system *we* are building *together*.
The code involved is a talking piece.

I prepend my comments with “PRAISE:”, “FIX:”, “SUGGESTION:”, “EXPLAIN:” and “CHECK:”.

FIX is the only one failing a code review.
Depending on the amount of, and severity, EXPLAIN and CHECK might give a red flag too.
The rest are for discussion; to learn and spread knowledge.

PRAISE is important. Only giving negative critique wears a relation. It might also be a part of the reason some don’t like code reviews. It can be hard to recognise good code as, as with many things, it steps out of the way so one does not reflect upon it. Good naming of a method, remember to remove old usings, good test data, …

SUGGESTION is how *I* would like to write the code. Change if you like, don’t if you don’t. I’d be happy to talk about it but there is no need to involve me in a decision.

EXPLAIN on the other hand requires my input. I don’t understand what you are doing – please explain it to me. If it is easy to explain then I should probably have understood; but if you have a hard time explaining it, then there is a good chance the code is too complex. (I have found a bug this way.)

CHECK is more serious. I am not sure you are doing the right thing and would like for you (or anyone else, including me) to double check it.

Typically there will be quite some SUGGESTION followed by a healthy amount of CHECK and EXPLAIN.

Update:

I have used this method for some projects now and only received positive or neutral feedback.

In one project one colleague took every SUGGESTION and implemented them while another ignored anything that wasn’t FIX. Both were skilled and made good choices. We talked a lot about code.

In another project my colleague liked to start with only looking at FIX to get a tally and then read through the rest. S/he is/was junior and we’ve had good discussions about code.

Update:

Someone else has come to the same conclusion: https://conventionalcomments.org/#labels

When code reviewing – do not forget to give appraisal too

Albeit code reviewing is for the greater good and we all adhere to the idea of common ownage of code we are still critisising someone else’s labour.

Psychologically we cannot neglect that.

Ergo: remember to tell about the good solutions and code you find.

Unfortunately TFS does not have a (good) solution for differentiating between comments and praise and critisism.