4 ms·

The issue with code reviews is that most of them should be prototype reviews before you are even half way done, or design reviews before you even start. Or just

The issue with code reviews is that most of them should be prototype reviews before you are even half way done, or design reviews before you even start. Or just dialog around some code or problem, along the way. Looking for quality issues or obvious mistakes after the fact is very rarely useful. I still think it's a good idea to have them as a sign off (e.g. so you at least have 2 people knowing what's happened), but other than that I don't think they provide that much value, at least not for people who are many years into a project and code base. But still we do these after-the-fact code reviews because I don't want to bother my colleague N times in the process. So I churn away until I'm done. And then not to bother them twice, I also polish it. And THEN I ask for a review. The reviewer sees something polished and finished, and assumes that a) since it's polished I must have spent a lot of thought around whether it's the right thing, and obviously anything they can do in the review time which is never going to exceed 2% of the dev time, is rarely going to result in them second guessing that and b) since it's polished they know they'll probably step on some toes if they say "throw it out and do something completely different". They can see the effort. The relationship with a colleague is more important than even the health of the project. So they approve it. The problem of course then is: how do you get from the "too late code reviews that they are rubber stamps" into a process of dialog, half-way reviews and iteration? Other than pair/mob programming I don't know. We say we don't want more processes, but whenever there isn't a process for this iterative behavior, the result is people developing for too long on their own and producing results that are too far gone to be shot down.

lexapro 4y ago
>review time which is never going to exceed 2% of the dev time What, why?
- alkonaut 4y ago
  Because it’s seen as pure overhead, so is expected to take perhaps a few minutes for a large change (that’s my experience at least). The fact that it isn’t is of course not lost on anyone - but everyone accepts the status quo as preferable. That is: people don’t want to spend a lot of time doing code review because they don’t like it. So they rather spend time working on a worse code base.
kqr 4y ago
> I don't want to bother my colleague N times in the process. Think of it this way: the maintenance burden of your (lower-quality) work if you don't bother your colleagues throughout the process will be way more bothersome than checking in up front.
- alkonaut 4y ago
  Exactly. So how do you align everyone on that? The status quo is that people wait too long to make reviews (they are a formal process step at the very end). It's very hard to make it a formal step half way. At the very end, they are of limited value and seen as a chore. So they become rubber stamps. The quality of the code base suffers from this, making development slower. Everyone knows what the end goal is, yet "Code review finished code" is easy to adopt, but not very efficient unless the org is extremely good at what they are doing. "Pause for thought and collaboration half way" is much much harder to achieve, without taking it to the extreme (pair sessions enforced etc).
  - kqr 4y ago
    Train reviewers to look critically at the big picture before they dive into details. That's going to hurt at first: almost finished code is going to be rejected and have to be reworked heavily or deleted entirely. But after a while, people will learn how to avoid it: seek feedback at an earlier stage.
grey-area 4y ago
Talking to your colleges about design decisions and wip reviews plus small iterative prs helps a lot. I have no idea why people think review of final code should be the default, as you say it is rarely useful.
_0w8t 4y ago
As was pointed out in the article a late async review can ensure that somebody else understands what happens in the code. In a few cases this alone justifies such review even if it is too late to change the architecture. Plus besides problematic architecture there are a lot of bugs that are easy to fix but that that one does not when developing the code. It is much easier to discover those in polished code than in the first working prototype.
agentultra 4y ago
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/publication/276162927_An_empirical_study_of_the_impact_of_modern_code_review_practices_on_software_quality/links/578e893308ae81b4466ec98e/An-empirical-study-of-the-impact-of-modern-code-review-practices-on-software-quality.pdf https://www.researchgate.net/profile/Ahmed-E-Hassan-2/public...
- watwut 4y ago
  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.
CuriouslyC 4y ago
I encourage people to put up draft PRs, and to request reviews early. Draft PRs don't have to pass CI/CD or be complete, and they let you see what everyone is currently working on.
davemp 4y ago
A big contributor to this is humans bending over backwards to fit a creative and dynamic process to rigid tools like JIRA.
marcosdumay 4y ago
> We say we don't want more processes Hum... You mean you don't want more meetings? You can't have more or less processes, their amount is completely defined by the diversity of your work. If it's about meetings, yes, synchronicity is always a problem. But you have synchronicity on the review already, it can't be too bad to just move it into another time.
ozim 4y ago
How do you get into process of dialogue? Years of working together in a team on the same thing to gain trust.

lexapro 4y ago

>review time which is never going to exceed 2% of the dev time What, why?

alkonaut 4y ago
Because it’s seen as pure overhead, so is expected to take perhaps a few minutes for a large change (that’s my experience at least). The fact that it isn’t is of course not lost on anyone - but everyone accepts the status quo as preferable. That is: people don’t want to spend a lot of time doing code review because they don’t like it. So they rather spend time working on a worse code base.

kqr 4y ago

> I don't want to bother my colleague N times in the process. Think of it this way: the maintenance burden of your (lower-quality) work if you don't bother your colleagues throughout the process will be way more bothersome than checking in up front.

alkonaut 4y ago
Exactly. So how do you align everyone on that? The status quo is that people wait too long to make reviews (they are a formal process step at the very end). It's very hard to make it a formal step half way. At the very end, they are of limited value and seen as a chore. So they become rubber stamps. The quality of the code base suffers from this, making development slower. Everyone knows what the end goal is, yet "Code review finished code" is easy to adopt, but not very efficient unless the org is extremely good at what they are doing. "Pause for thought and collaboration half way" is much much harder to achieve, without taking it to the extreme (pair sessions enforced etc).
- kqr 4y ago
  Train reviewers to look critically at the big picture before they dive into details. That's going to hurt at first: almost finished code is going to be rejected and have to be reworked heavily or deleted entirely. But after a while, people will learn how to avoid it: seek feedback at an earlier stage.

grey-area 4y ago

Talking to your colleges about design decisions and wip reviews plus small iterative prs helps a lot. I have no idea why people think review of final code should be the default, as you say it is rarely useful.

_0w8t 4y ago

As was pointed out in the article a late async review can ensure that somebody else understands what happens in the code. In a few cases this alone justifies such review even if it is too late to change the architecture. Plus besides problematic architecture there are a lot of bugs that are easy to fix but that that one does not when developing the code. It is much easier to discover those in polished code than in the first working prototype.

agentultra 4y ago

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/publication/276162927_An_empirical_study_of_the_impact_of_modern_code_review_practices_on_software_quality/links/578e893308ae81b4466ec98e/An-empirical-study-of-the-impact-of-modern-code-review-practices-on-software-quality.pdf https://www.researchgate.net/profile/Ahmed-E-Hassan-2/public...

watwut 4y ago
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.

CuriouslyC 4y ago

I encourage people to put up draft PRs, and to request reviews early. Draft PRs don't have to pass CI/CD or be complete, and they let you see what everyone is currently working on.

davemp 4y ago

A big contributor to this is humans bending over backwards to fit a creative and dynamic process to rigid tools like JIRA.

marcosdumay 4y ago

> We say we don't want more processes Hum... You mean you don't want more meetings? You can't have more or less processes, their amount is completely defined by the diversity of your work. If it's about meetings, yes, synchronicity is always a problem. But you have synchronicity on the review already, it can't be too bad to just move it into another time.

ozim 4y ago

How do you get into process of dialogue? Years of working together in a team on the same thing to gain trust.