A few ideas on code review

Af­ter watch­ing a talk about code re­view 1, from the last Eu­roPy­thon, I got some ideas that I’d like to sum­marise.

The first one is a point I made on my talk about clean code in Python, about run­ning style checks (PEP-8, for ex­am­ple), au­to­mat­i­cal­ly as part of the CI. This has sev­er­al ra­tio­nales, the first one be­ing that it’s the sort of thing ma­chines should do, and not hu­man­s. Hav­ing soft­ware en­gi­neers re­view­ing the tab­s, spac­ing, and such, is just a wast­ed ef­fort (not to men­tion, wast­ed mon­ey). As I men­tioned on the con­clu­sions dur­ing my talk, we should fo­cus on the read­abil­i­ty of the code in terms of the log­ic, by ask­ing our­selves things like “does this code make sense?“, rather than “is the space be­fore the com­ma cor­rec­t?“. Re­mem­ber, code is for hu­man­s, not ma­chi­nes, it’s a means of com­mu­ni­ca­tion with the de­vel­op­ment team.

By au­tomat­ing these checks in the CI build 2, we leave these de­tails out of the scope, to fo­cus on more im­por­tant things, be­cause in the end, the ul­ti­mate goal of the code re­view is ac­tu­al­ly to find de­fects on the code, and point them out for ear­ly cor­rec­tion.

In ad­di­tion, sep­a­rat­ing the style check­s, gives them a sense of “ob­jec­tive met­ric”, be­cause the au­thor has to ar­gue with an scrip­t, in­stead of an­oth­er de­vel­op­er. This al­so en­forces the style guide for the pro­jec­t, clos­er to the code.

Then fol­lowed lots of more tips aimed at mak­ing code re­view more ef­fec­tive. For ex­am­ple, that it’s im­por­tant to al­so high­light pos­i­tive things found whilst look­ing at the code, and (ar­guably the most im­por­tant one) that au­thors have to be re­cep­tive about feed­back when plac­ing a pull re­quest. This is per­haps the most dif­fi­cult point, be­cause most of the times we hear things like “would you merge this code?“, rather than “would you give me feed­back about this?“. Once again, the goal of code re­view is not to chal­lenge, but to find de­fects on the code, so when plac­ing a pull re­quest, it’s good for the au­thor to keep in mind that re­view­ers are ac­tu­al­ly help­ing him or her, to im­prove the qual­i­ty of the work. And it’s al­so a learn­ing ex­pe­ri­ence.

0% thank­ful­ly. Coders act like they’ve paint­ed a mas­ter­piece and tend to de­bate ev­ery piece of feed­back.

— Mark Litwintschik (@­mark­l­it82) June 1, 2016

An­oth­er im­por­tant point, is that for code re­views to be ef­fec­tive, pull re­quests have to be smal­l.

10 lines of code = 10 is­sues.

500 lines of code = “looks fine.”

Code re­views.

— I Am De­vlop­er (@i­amde­vlop­er) November 5, 2013

This in­creas­es not on­ly the qual­i­ty of the re­view, but al­so the like­li­hood of it be­ing merged.

Code re­view is a very ex­ten­sive top­ic, so there are a lot of more things that can be anal­ysed, but these are some of the main ideas that I con­sid­er to be of most im­por­tance.

1

http­s://y­outu.be/uI­wl01­Nazdg

2

I’ll men­tion the sort of checks that can be con­fig­ured in the CI, in a fu­ture en­try.