Agree that waiting until the very end of the development process for a code review is troublesome. It’s the most expensive time to request changes too. As you point out, asking for something completely different is going to put the author back at square one and frustrate a lot of people.
It’s best, I find, to get that kind of design-level feedback early and review it when needed as progress is made. No sense waiting until you’ve polished it to hear that you wasted your time building the wrong thing.
Another strategy to cope: small changes merged early and often. This requires a bit more work on the part of the author to keep the continuity of the plan moving in the right direction over several PRs. However, it has been demonstrated empirically that humans can’t review much more than a couple hundred lines of code per hour [0].
And lastly, as a reviewer, demand evidence as to why the change is correct. Don’t bother yourself with reading every line like you’re some compiler that’s better than your compiler. Instead validate the reasoning of the author and read the tests/evidence/proof. You need a fairly experienced team that are sophisticated enough to write good specifications, but even good unit tests go a long way. This way you only have to check the author’s argument.
If you’re using languages with manual memory management make sure you’re using the right testing strategy that will catch errors in your program. Write unit tests at the bare minimum. Property tests would be preferred. Fuzz tests, integration tests, etc on top. Whatever it takes: catching errors in programs is surprisingly difficult for humans regardless of experience or training. The JVM had a critical error in its binary search implementation that hid for nearly a decade, OpenSSL, etc.
Code review is not about reading every line and playing, “spot the error.” You’ll miss some. Your team mates will miss some. You need to think above the code to catch those. Time is precious and life is short. Spend review time effectively by making sure you understand the specifications and that your colleagues have done their homework.
I think that one issue here is that the early feedback is not code review and there is zero reason to try to force it into code review framework. Vague high level design-level feedback session can be organized and named as such.
Comments
Agree that waiting until the very end of the development process for a code review is troublesome. It’s the most expensive time to request changes too. As you point out, asking for something completely different is going to put the author back at square one and frustrate a lot of people.
It’s best, I find, to get that kind of design-level feedback early and review it when needed as progress is made. No sense waiting until you’ve polished it to hear that you wasted your time building the wrong thing.
Another strategy to cope: small changes merged early and often. This requires a bit more work on the part of the author to keep the continuity of the plan moving in the right direction over several PRs. However, it has been demonstrated empirically that humans can’t review much more than a couple hundred lines of code per hour [0].
And lastly, as a reviewer, demand evidence as to why the change is correct. Don’t bother yourself with reading every line like you’re some compiler that’s better than your compiler. Instead validate the reasoning of the author and read the tests/evidence/proof. You need a fairly experienced team that are sophisticated enough to write good specifications, but even good unit tests go a long way. This way you only have to check the author’s argument.
If you’re using languages with manual memory management make sure you’re using the right testing strategy that will catch errors in your program. Write unit tests at the bare minimum. Property tests would be preferred. Fuzz tests, integration tests, etc on top. Whatever it takes: catching errors in programs is surprisingly difficult for humans regardless of experience or training. The JVM had a critical error in its binary search implementation that hid for nearly a decade, OpenSSL, etc.
Code review is not about reading every line and playing, “spot the error.” You’ll miss some. Your team mates will miss some. You need to think above the code to catch those. Time is precious and life is short. Spend review time effectively by making sure you understand the specifications and that your colleagues have done their homework.
Happy reviewing.
[0] https://www.researchgate.net/profile/Ahmed-E-Hassan-2/public...
I think that one issue here is that the early feedback is not code review and there is zero reason to try to force it into code review framework. Vague high level design-level feedback session can be organized and named as such.
Definitely! Code review is a bad time to be making design crit.