3 ms·

Logically, any review criticism is too late, because the code was already written! Catching the issue before commitment is great, but the proper time for the fe

Logically, any review criticism is too late, because the code was already written! Catching the issue before commitment is great, but the proper time for the feedback was during the actual writing. Now the code must be sent back for rewriting, and time has been wasted.

kstenerud 5y ago
If the code requires such a substantial rewrite after review, that would be indicative of serious communication problems on the team. In 20-odd years of reviewing code, I can't think of many instances where a review led to a substantial rewrite.
- MockObject 5y ago
  I don't know what "substantial" means but, say, in a 3 day long ticket, surely all sorts of issues found in review can easily force another day or so of work.
  - kstenerud 5y ago
    Anything over half a day would be substantial, but really I'd consider any post code review fix that takes longer than an hour to be a red flag that we're not communicating effectively. As I alluded to earlier, I've never encountered these sorts of problems in the healthy companies I've worked at (but I HAVE encountered them at unhealthy companies).
Agentlien 5y ago
Catching problems before things are written is definitely better and that's why it's often a good idea to mention how you intend to approach problems in your daily sync. If someone has concerns, they can bring it up. But if that fails, it is definitely better to review and catch it after the code is written than to submit badly written code out of fear of wasted effort. In my experience the vast majority of tasks are easily addressed by a single person and most proficient programmers will usually produce perfectly adequate code which doesn't raise any concerns. Having reviews to catch the odd lapse in judgement or logical misstep doesn't usually add a lot of overhead (as opposed to allocating two programmers to every menial task) but saves a lot more time and effort in avoided bugs and issues than the cost of having to rewrite something every now and then. Additionally, any single submit should be kept to a manageable size so that they're easier to review and issues are caught early. The only times I can remember having insisted someone rewrite something non-trivial have been when they misunderstood some fundamental issue, did not communicate progress as they should have, and did not follow guidelines of solving a single task per submit. That type of behaviour is in itself a pattern which needs to be addressed.
- MockObject 5y ago
  > But if that fails, it is definitely better to review and catch it after the code is written than to submit badly written code out of fear of wasted effort. Yes, without a doubt. > Having reviews to catch the odd lapse in judgement or logical misstep doesn't usually add a lot of overhead (as opposed to allocating two programmers to every menial task) Having paired for years, and soloed for years, I have found that the downsides of allocation are overridden by the benefits of pairing, one of which is real-time code review.
  - Agentlien 5y ago
    I have done some pair programming. At my first job we sometimes did it when some problem proved especially tricky. Typically a tricky bug. In some cases it's invaluable, absolutely. I just don't think that's a large percentage of the code written on a daily basis. For most tasks pairing seems to be either 1) one person tapping out simple code which practically writes itself while the other lazily nods along. 2) One person solving something tricky which happens to be their area of expertise while the other stares blankly. Maybe at that point it would be useful to explain things in detail, but that can make development take much longer and if something like this requires rethinking halfway it can be quite taxing. In that case, I would prefer waiting until it's done and compiling the result as an annotated review. Which can be a great forum for asking questions.
    - MockObject 5y ago
      When I've seen it adopted, it was in the context of XP. The red-green-refactor cycle affords a decent flow for pairs, who may switch off between test writing and feature writing. I can see that without the other XP practices, pairing may be awkward.

kstenerud 5y ago

If the code requires such a substantial rewrite after review, that would be indicative of serious communication problems on the team. In 20-odd years of reviewing code, I can't think of many instances where a review led to a substantial rewrite.

MockObject 5y ago
I don't know what "substantial" means but, say, in a 3 day long ticket, surely all sorts of issues found in review can easily force another day or so of work.
- kstenerud 5y ago
  Anything over half a day would be substantial, but really I'd consider any post code review fix that takes longer than an hour to be a red flag that we're not communicating effectively. As I alluded to earlier, I've never encountered these sorts of problems in the healthy companies I've worked at (but I HAVE encountered them at unhealthy companies).

Agentlien 5y ago

Catching problems before things are written is definitely better and that's why it's often a good idea to mention how you intend to approach problems in your daily sync. If someone has concerns, they can bring it up. But if that fails, it is definitely better to review and catch it after the code is written than to submit badly written code out of fear of wasted effort. In my experience the vast majority of tasks are easily addressed by a single person and most proficient programmers will usually produce perfectly adequate code which doesn't raise any concerns. Having reviews to catch the odd lapse in judgement or logical misstep doesn't usually add a lot of overhead (as opposed to allocating two programmers to every menial task) but saves a lot more time and effort in avoided bugs and issues than the cost of having to rewrite something every now and then. Additionally, any single submit should be kept to a manageable size so that they're easier to review and issues are caught early. The only times I can remember having insisted someone rewrite something non-trivial have been when they misunderstood some fundamental issue, did not communicate progress as they should have, and did not follow guidelines of solving a single task per submit. That type of behaviour is in itself a pattern which needs to be addressed.

MockObject 5y ago
> But if that fails, it is definitely better to review and catch it after the code is written than to submit badly written code out of fear of wasted effort. Yes, without a doubt. > Having reviews to catch the odd lapse in judgement or logical misstep doesn't usually add a lot of overhead (as opposed to allocating two programmers to every menial task) Having paired for years, and soloed for years, I have found that the downsides of allocation are overridden by the benefits of pairing, one of which is real-time code review.
- Agentlien 5y ago
  I have done some pair programming. At my first job we sometimes did it when some problem proved especially tricky. Typically a tricky bug. In some cases it's invaluable, absolutely. I just don't think that's a large percentage of the code written on a daily basis. For most tasks pairing seems to be either 1) one person tapping out simple code which practically writes itself while the other lazily nods along. 2) One person solving something tricky which happens to be their area of expertise while the other stares blankly. Maybe at that point it would be useful to explain things in detail, but that can make development take much longer and if something like this requires rethinking halfway it can be quite taxing. In that case, I would prefer waiting until it's done and compiling the result as an annotated review. Which can be a great forum for asking questions.
  - MockObject 5y ago
    When I've seen it adopted, it was in the context of XP. The red-green-refactor cycle affords a decent flow for pairs, who may switch off between test writing and feature writing. I can see that without the other XP practices, pairing may be awkward.