(no title)
ampersandy | 3 years ago
If your coworkers are sending changes for review that don't explain why the changes are being made (and therefore provide the context), reject it until they write a proper summary/test plan that does explain it. You should review the "metadata" of changes before you ever review the code itself and if done well, you'll probably be able to guess what a lot of the incoming changes are before reviewing the code. This makes it easier to spot code that deviates from the stated purpose of the changes and can help identify faults in both understanding of the overall problem or the system being built, especially in more junior engineers.
If your coworkers are taking to long to review changes, work with your team to incentivize expedient code reviews, or talk to the people that aren't reviewing enough code to help everyone else get more done. You should also utilize a stack that lets you continue progressing with your work even when waiting for reviews (like stacked diffs).
plorkyeran|3 years ago
rekahrv|3 years ago
That's a great approach. It sounds logical, but I certainly don't do it consistently. :-)
watwut|3 years ago
ajorgensen|3 years ago
If you think about a pull request as a story you have who, what, where, when, and why. Most tools handle the first 4 but the why is critically important to anyone, including yourself, who needs to understand the nuance of the change that can’t easily be expressed by the code itself without a novel size comment block.
I find that a lot of times people forget the “why” when submitting a change set and simply restate the “what”. At the time you make the change it’s all fresh in your mind and you trick yourself into believing you’ll remember all the details but in reality that’s no my experience.
onion2k|3 years ago
arp242|3 years ago
Everything you say that's not written down will be lost in the aether within weeks, months at best. I'm a huge fan of async communication because everything will be written down.
If there's a discussion "why did you go with A and not B", in an (async) code review then anyone can come back to it a year later and read up on why it was done in this particular way. I've found this useful on a number of occasions.
It certainly beats "Alice, you changed this last year, but why is it done like that? Wouldn't X be much more convenient because Y? It's hard to add Z now.", "ehm, I discussed it with Bob, but he since left the company. I don't quite remember, I think it was A? Or maybe B? Or was it C?"
MajimasEyepatch|3 years ago