I've gotten pushback on best practises, especially from more junior developers or developers on a tight deadline.
I do C# and one common, yet trivial example is poor naming conventions.
As a reviewer we have certain tools we can use to encourage change:
* We can make comments low priority, so the advice is there, but it's skippable.
* We can make summary comments, that are opinionated but don't expect any immediate or direct resolution (great for architectural thoughts).
* We can include example code snippets in our comments. This reduces the burden for the developer to adopt a certain change. I've even gone to the extent of writing small programs to validate a refactor or prove an error exists.
* Sometimes it's not an error, but how that company wants X done. We take note of these for future reviewers.
Overall, this is a diplomacy game, the only power we have is soft power. I find it good practise, as I'm typically a direct kinda guy.
Results?
I've seen a lot of developers, after being introduced to new ways of working, implement that on later PR's. Sometimes immediately, sometimes after it arises a few times. Examples include improved naming, better code structure, usage of newer language features, more clarity in the code, or better SQL injection protection!
Thanks for the reply! It's nice to hear that a lot of devs are receptive to change and aren't treating the service hostilely. I like that there's categorization with expectations encoded in the category, I can see it helping a lot of things. A huge chunk of my reviews used to be in Code Collab, which... lacked certain desirable things. (But it got right a few others, so using it wasn't a totally bad tradeoff.) One of those I wanted was useful categories -- by default you just have review comments with the only alternative being a bright red Defect comment that would block closing the review until addressed -- but it was rather annoying to all parties and so few people used it. Different conventions arose to try and address the issue but they tended to be team specific. One of my own was prefacing those low priority things with "Nitpick: " or similar like in "Nit: add space between if and (". Another convention came from being able to make file-level comments that aren't targeted at a particular line, the reviewer might preemptively check off the file if they don't expect any action and it's just broader discussion/commentary, or leave that step undone until some sort of response (maybe just clarification) or possibly broad code changes has been done.
I've sometimes had trouble convincing some older programmers about using new (or even not so new but slightly 'advanced') language features like Java lambdas or Optional or generic types (juniors/interns were often aware of and happy to use the new stuff already), to the point that for specific individuals I'd just give up and focus my review efforts on other aspects. However if I ended up touching that code myself later on, or someone else did whom I was reviewing, I'd use those newer features/encourage the other person to use them if they weren't already, and if the original person ever came back to it they'd face an argument of local consistency against trying to change it back to using the old ways. It seems like the approach of longer-term encouraging and supporting a pocket of engineers pushing for better practices would actually work with this service given the ability to make notes for future reviewers. Another flaw in Code Collab was its atrocious search which made it hard to find prior reviews about a set of files.
I do C# and one common, yet trivial example is poor naming conventions.
As a reviewer we have certain tools we can use to encourage change: * We can make comments low priority, so the advice is there, but it's skippable. * We can make summary comments, that are opinionated but don't expect any immediate or direct resolution (great for architectural thoughts). * We can include example code snippets in our comments. This reduces the burden for the developer to adopt a certain change. I've even gone to the extent of writing small programs to validate a refactor or prove an error exists. * Sometimes it's not an error, but how that company wants X done. We take note of these for future reviewers.
Overall, this is a diplomacy game, the only power we have is soft power. I find it good practise, as I'm typically a direct kinda guy.
Results?
I've seen a lot of developers, after being introduced to new ways of working, implement that on later PR's. Sometimes immediately, sometimes after it arises a few times. Examples include improved naming, better code structure, usage of newer language features, more clarity in the code, or better SQL injection protection!