A few ideas on code review

After wa­tching a ta­lk about co­de re­view [1], from the last Eu­ro­P­y­tho­n, I go­t ­so­me ideas that I’d like to su­m­ma­ri­se.

The first one is a point I ma­de on my ta­lk about clean co­de in Py­thon, about run­ning sty­le che­cks (PE­P-8, fo­r e­xam­ple), au­to­ma­ti­ca­lly as part of the CI. This has se­ve­ral ra­tio­na­le­s, the ­first one being that it’s the sort of thing ma­chi­nes should do, and not hu­man­s. Ha­ving so­ftwa­re en­gi­neers re­viewing the ta­bs, spa­cin­g, and su­ch, is just a was­ted effort (not to men­tio­n, was­ted mo­ne­y). As I men­tio­ned on the con­clu­sion­s ­du­ring my ta­lk, we should fo­cus on the rea­da­bi­li­ty of the co­de in ter­ms of the ­lo­gi­c, by asking our­sel­ves things like “does this co­de make sen­se?“, ra­the­r ­than “is the spa­ce be­fo­re the co­m­ma co­rrec­t?“. Re­mem­be­r, co­de is for hu­man­s, ­not ma­chi­nes, it’s a means of co­m­mu­ni­ca­tion wi­th the de­ve­lo­p­ment tea­m.

By au­to­ma­ting the­se che­cks in the CI build [2], we lea­ve the­se de­tails out of ­the sco­pe, to fo­cus on mo­re im­por­tant things, be­cau­se in the en­d, the ul­ti­ma­te ­goal of the co­de re­view is ac­tua­lly to find de­fec­ts on the co­de, and poin­t ­them out for ear­ly co­rrec­tio­n.

In addi­tio­n, se­pa­ra­ting the sty­le che­cks, gi­ves them a sen­se of “ob­jec­ti­ve ­me­tri­c”, be­cau­se the au­thor has to ar­gue wi­th an scrip­t, ins­tead of ano­the­r ­de­ve­lo­pe­r. This al­so en­for­ces the sty­le gui­de for the pro­jec­t, clo­ser to the ­co­de.

Then fo­llo­wed lo­ts of mo­re tips ai­med at making co­de re­view mo­re effec­ti­ve. ­For exam­ple, that it’s im­por­tant to al­so hi­gh­li­ght po­si­ti­ve things found whils­t ­looking at the co­de, and (ar­gua­bly the most im­por­tant one) that au­thors ha­ve to­ ­be re­cep­ti­ve about fee­dba­ck when pla­cing a pu­ll re­ques­t. This is perhaps the ­most di­ffi­cult poin­t, be­cau­se most of the ti­mes we hear things like “would you ­mer­ge this co­de?“, ra­ther than “would you gi­ve me fee­dba­ck about this?“. On­ce agai­n, the goal of co­de re­view is not to cha­llen­ge, but to find de­fec­ts on ­the co­de, so when pla­cing a pu­ll re­ques­t, it’s good for the au­thor to keep in ­mind that re­viewers are ac­tua­lly hel­ping him or he­r, to im­pro­ve the qua­li­ty of ­the wo­rk. And it’s al­so a lear­ning ex­pe­rien­ce.

Ano­ther im­por­tant poin­t, is that for co­de re­views to be effec­ti­ve, pu­ll ­re­ques­ts ha­ve to be sma­ll.

This in­crea­ses not on­ly the qua­li­ty of the re­view, but al­so the like­li­hood of it being mer­ged.

Co­de re­view is a ve­ry ex­ten­si­ve to­pi­c, so the­re are a lot of mo­re things tha­t ­can be ana­l­ys­e­d, but the­se are so­me of the main ideas that I con­si­der to be of ­most im­por­tan­ce.

[1] https://youtu.be/uIwl01Nazdg
[2] I’ll mention the sort of checks that can be configured in the CI, in a future entry.