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­sion­s ­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.

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

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] https://youtu.be/uIwl01Nazdg
[2] I’ll mention the sort of checks that can be configured in the CI, in a future entry.