5 ms·
At my workplace all code review comments must be classified as either "blocking", which is stuff like "this code will break the app for our users" or "this is i
by JoshuaDavid 5y ago
At my workplace all code review comments must be classified as either "blocking", which is stuff like "this code will break the app for our users" or "this is insecure", or "non-blocking", which is stuff like "this method is hard to read and should be broken up" or "I think this variable could be named better". Non-blocking stuff is (really, truly) optional to address.
IMO this system works pretty well as a balance between "we want to make sure nothing catastrophically bad hits production" and "we don't need perfect code, just good-enough code". It does require that you have a general understanding on the team that the purpose of the code is to deliver value to your users, and does not have to be perfect to be valuable.
We did also at one point try a system with more granular classification where every comment had to be prefixed with one of a predefined list of things like "nit", "security", "performance", "comment", or "kudos", which was fine but I don't think really adds all that much relative to just "blocking / non-blocking".
- bcrosby95 5y ago> "I think this variable could be named better" I had a friend describe to me how hurt he was because the code reviewer didn't like how he named things. Apparently he built a giant metaphor for his system. And he then wrote code in that metaphor. I don't recall the details. It involved fruit, trains, clippers, etc. He was writing a system to report on how disk space was used across their systems.
- aliasEli 5y agoNames are important for understanding code. They are most useful if everybody uses the same conventions. It sounds like your friend was trying to push personal conventions onto others.
- dwaltrip 5y agoDid he eventually realize that his naming system was confusing and could be cleaned up?
- stephane-klein 5y agohttps://conventionalcomments.org/ https://conventionalcomments.org/
- JoshuaDavid 5y agoYeah, it was that list of prefixes. Thanks.
- davedx 5y agoThis sounds good. It all depends on your engineering culture at the end of the day.