5 ms·

I agree with your criticism against pair programming. I also agree that it's helpful with an extra pair of eyes on anything you write, which is why I'm strongly

I agree with your criticism against pair programming. I also agree that it's helpful with an extra pair of eyes on anything you write, which is why I'm strongly in favour of code reviews.

harryf 5y ago
...although some people might argue that you could get the same value from a rubber duck ( https://en.wikipedia.org/wiki/Rubber_duck_debugging https://en.wikipedia.org/wiki/Rubber_duck_debugging )
- Agentlien 5y ago
  I want very different things from my rubber ducks and my code reviews. Rubber ducking is great for forcing yourself to articulate your train of thought, spelling out your assumptions, and proof-reading your own logic. Code reviews are great for catching errors you glaze over because you're too familiar with what you think you wrote to see what's actually written. It's also a great way to onboard people and enforce code standards and best practices. Even when you're in a more senior position code reviews can often teach you new things, when you interact with systems the reviewer knows better and they happen to know a better approach.
- hughrr 5y ago
  I do this. It works. I have a stuffed parrot though. And everyone thinks I’m nuts but I don’t care :)
MockObject 5y ago
I despise code reviews, because the criticism is shallow, and offered too late, after the work has been done. I want the review in real time, and at a greater depth of insight. This is what pairing provides.
- Agentlien 5y ago
  Shallow and too late criticism sounds like a problem with how the reviews are handled. At my previous job reviews were required before you were allowed to check in, meaning if it wasn't good enough you had to rework it. This meant junior engineers were quickly brought up to speed with code convention and best practices. If you didn't adhere to them your code wasn't approved. I also found it a great way to catch subtle bugs or difficult to follow logic. But it does require actually having and taking the time to carefully review things, rather than giving them a passing glance.
  - MockObject 5y ago
    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).
- anthonygd 5y ago
  I have mixed feelings about code reviews. It seems like sometimes they're an opportunity to show off so every review _will_ find something insignificant; sometimes code reviews are rubber stamps and just not useful. There's a balance though where you get concise thoughtful responses. It seems like there is something about pairing though that guarantees a focus on shallow insignificance. I don't like dismissing something that has smart proponents but I just can't find a way to make pair programming anything other than negative. The real time aspect rules out depth.
- rmk 5y ago
  Code reviews sometimes seem like a micro-management tool: they often boil down to quibbling over things that are quite superficial, while simultaneously ignoring or missing big problems that lurk under the surface. Without some sort of automated linting and code style enforcement, this is what happens in code reviews. They serve as a reminder to the developer that his work is not trusted, and someone else must keep an eye on him at all times. This is very bothersome to people who have been around the block a few times, and are already capable of producing robust, clean product (this is often a pitfall of having someone who's just out of college reviewing code from an experienced dev). Another problem I've observed in code reviews is that they turn into impromptu redesigning sessions. Most devs are quite opinionated, and code reviews often become an avenue to engage in bikeshedding that can actually harm morale, not to mention velocity and product quality. The trouble with pairing is that it doesn't remedy these problems effectively, while simultaneously reducing the number of hands doing work. If two people just don't get along, pairing them is not going to magically solve the problem. And I guarantee that there will always be people who simply do not see eye to eye on many important things in most sizable teams.
  - randomswede 5y ago
    Yeah, code reviews (like any other process) can be done wrong. The way to propose a change, in a "good way" (I am assuming here that no one has unilateral capability of merging to master, and things thus have to pass through review), to my mind, involve doing a few things. <ul> <li> keep the change small ("small" here is a very fungible quality, but "probably less than n * 1000 lines") <li> does NOT mix "fix bug" / "add feature" / "refactor" (arguably, there are cases where feature-adding and bug fixing are instrinsically linked, let those through) <li> has a change description detailing the why of the change (the "what" is the change itself) </ul> As a reviewer, your task is not to critique the design (that is why everyone agreed on a design doc up front, no?). It is OK to review the structure of the code, though. Hopefully, you have a "run this through a linter / style-checker" integrated into your pipeline, so that should be pretty much already done. But, if there are a few things left over, it is OK to point those out. It is OK to point out that things that should be (but aren't) tested should have tests. If you can spot an edge case that isn't (or doesn't seem to be) tested, ask for that to be tested, also. People need to be permitted to take the time to do a review. With these things in place, a review should be pretty quick (at the pace of up to a few lines per second), and most of the time frictionless. But, it does require some discipline, both from the writer and the reviewer.

I agree with your criticism against pair programming. I also agree that it's helpful with an extra pair of eyes on anything you write, which is why I'm strongly in favour of code reviews.

harryf 5y ago

...although some people might argue that you could get the same value from a rubber duck ( https://en.wikipedia.org/wiki/Rubber_duck_debugging https://en.wikipedia.org/wiki/Rubber_duck_debugging )

Agentlien 5y ago
I want very different things from my rubber ducks and my code reviews. Rubber ducking is great for forcing yourself to articulate your train of thought, spelling out your assumptions, and proof-reading your own logic. Code reviews are great for catching errors you glaze over because you're too familiar with what you think you wrote to see what's actually written. It's also a great way to onboard people and enforce code standards and best practices. Even when you're in a more senior position code reviews can often teach you new things, when you interact with systems the reviewer knows better and they happen to know a better approach.
hughrr 5y ago
I do this. It works. I have a stuffed parrot though. And everyone thinks I’m nuts but I don’t care :)

MockObject 5y ago

I despise code reviews, because the criticism is shallow, and offered too late, after the work has been done. I want the review in real time, and at a greater depth of insight. This is what pairing provides.

Agentlien 5y ago
Shallow and too late criticism sounds like a problem with how the reviews are handled. At my previous job reviews were required before you were allowed to check in, meaning if it wasn't good enough you had to rework it. This meant junior engineers were quickly brought up to speed with code convention and best practices. If you didn't adhere to them your code wasn't approved. I also found it a great way to catch subtle bugs or difficult to follow logic. But it does require actually having and taking the time to carefully review things, rather than giving them a passing glance.
- MockObject 5y ago
  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).
anthonygd 5y ago
I have mixed feelings about code reviews. It seems like sometimes they're an opportunity to show off so every review _will_ find something insignificant; sometimes code reviews are rubber stamps and just not useful. There's a balance though where you get concise thoughtful responses. It seems like there is something about pairing though that guarantees a focus on shallow insignificance. I don't like dismissing something that has smart proponents but I just can't find a way to make pair programming anything other than negative. The real time aspect rules out depth.
rmk 5y ago
Code reviews sometimes seem like a micro-management tool: they often boil down to quibbling over things that are quite superficial, while simultaneously ignoring or missing big problems that lurk under the surface. Without some sort of automated linting and code style enforcement, this is what happens in code reviews. They serve as a reminder to the developer that his work is not trusted, and someone else must keep an eye on him at all times. This is very bothersome to people who have been around the block a few times, and are already capable of producing robust, clean product (this is often a pitfall of having someone who's just out of college reviewing code from an experienced dev). Another problem I've observed in code reviews is that they turn into impromptu redesigning sessions. Most devs are quite opinionated, and code reviews often become an avenue to engage in bikeshedding that can actually harm morale, not to mention velocity and product quality. The trouble with pairing is that it doesn't remedy these problems effectively, while simultaneously reducing the number of hands doing work. If two people just don't get along, pairing them is not going to magically solve the problem. And I guarantee that there will always be people who simply do not see eye to eye on many important things in most sizable teams.
- randomswede 5y ago
  Yeah, code reviews (like any other process) can be done wrong. The way to propose a change, in a "good way" (I am assuming here that no one has unilateral capability of merging to master, and things thus have to pass through review), to my mind, involve doing a few things. <ul> <li> keep the change small ("small" here is a very fungible quality, but "probably less than n * 1000 lines") <li> does NOT mix "fix bug" / "add feature" / "refactor" (arguably, there are cases where feature-adding and bug fixing are instrinsically linked, let those through) <li> has a change description detailing the why of the change (the "what" is the change itself) </ul> As a reviewer, your task is not to critique the design (that is why everyone agreed on a design doc up front, no?). It is OK to review the structure of the code, though. Hopefully, you have a "run this through a linter / style-checker" integrated into your pipeline, so that should be pretty much already done. But, if there are a few things left over, it is OK to point those out. It is OK to point out that things that should be (but aren't) tested should have tests. If you can spot an edge case that isn't (or doesn't seem to be) tested, ask for that to be tested, also. People need to be permitted to take the time to do a review. With these things in place, a review should be pretty quick (at the pace of up to a few lines per second), and most of the time frictionless. But, it does require some discipline, both from the writer and the reviewer.