8 ms·

I go back and forth on this. On the one hand, I really like constant deep feedback. I really like the consistency benefits of having another person say “that’s

I go back and forth on this. On the one hand, I really like constant deep feedback. I really like the consistency benefits of having another person say “that’s too much, I find that unreadable.” On the other, I have now been at a lot of places where it was very hard to get my code reviewed, latencies of days and sometimes weeks if folks are in a particularly heinous crunchtime... And then when it does get reviewed, the stuff that I worked really hard on to get the right speed or to properly centralize cache invalidation... Suddenly someone is like “I would have done it from this other approach” and you have no idea whether it's tractable or not. While I have never been at a place that did this, I have in my head the idea that the code should be an unfolding collective conversation, kind of like when folks are all collaborating on a shared Google Doc, I see that you are editing this section and I throw in a quick comment “don't forget to add XYZ” and then jump to a different part that I won't be stepping on their toes with. So the basic idea would be to get everybody to merge to `main` like 2 or 3 times a day if possible. In that case code review really is just “make sure this doesn't break the build or break prod, everything is behind a feature toggle, if I don't like the design I will comment on the code near the design or sketch a proof of concept showing that my approach is superior in clarity or speed or whatever”... Nobody ever takes me up on this culture shift it seems.

yarekt 3y ago
This is called Continuous Integration, and its a shame that the term got nicked to now mean "that thing that builds our PRs somewhere". The idea was that everyone pushes to main all the time, which basically reduces integration time to 0, as everyone is doing it every couple of minutes on big teams. After a while you learn how to not step on people's toes (Introduce new classes incrementally, use docblocs documenting class' intent, rather than just current functionality) I too find that its basically impossible to suggest switching to this workflow, given the weight of all our existing tools. They are so easy to setup, and most come for free (Azure Devops/Pipelines thing) that going off the track is just unthinkable.
- eru 3y ago
  Everyone committing to the main branch all the time sounds like a nightmare to me, to be honest. I would probably seriously consider quitting if this was forced on me. (Or rather, I would probably just run on my private git copy, and only pull every once in a while, and ignore that the main branch always changes.) When / how do you do code review in your suggested workflow?
  - gizzlon 3y ago
    You can have branches, but the ""rule"" is that no branch should live for more than, say, a day. This trades the integration complexity and problems for some new.. challenges :)
    - pmontra 3y ago
      Do people start working on something new past 3 PM or do they start discussing what to do the next day?
      - bobthepanda 3y ago
        My team uses this pattern and generally speaking it looks something like this * everybody is good at breaking down code into changes small enough review quickly but not small enough to become tedious and trivial * people work on CRs whenever they have time. They do generally not post them after people have started going offline, and wait til next morning as a courtesy, since there is no difference between publishing for review at 5pm vs 9am the next day One side effect of this workflow is that because the pieces are more manageable, less uninterrupted “flow” time is required to complete a bunch of small things than to make one really big change. And others digest the smaller changes easier and knowledge spreads more effectively. And with the time its easy to say “don’t make people review outside working hours.”
    - mannykannot 3y ago
      They are not new challenges, they are the same old challenge that led to version control procedures and systems in the first place: your incomplete or broken code is interfering with my attempts to complete or fix mine!
bernds74 3y ago
"I would have done it from this other approach". I've seen that, and it's not good when you get the feeling of "code review is when someone who hasn't thought about your problem tells you how you should have solved it". People sometimes feel they have to add value as a reviewer, and casually discarding other people's work is the way to do it. Fortunately it's not something I have to deal with at my current job.
- dijksterhuis 3y ago
  I’ve made comments that start like that and it’s usually down to 1) obvious code smell, here’s an example using your existing code refactored and the reasons why it is a better fit here 2) you’ve done something I didn’t think of and it’s clearly better than the way I was thinking of it. Here’s why it’s better. Kudos! Helps that I’m lead/principal in a small biz with like 4 people max writing code. So I kind of know what everyone is working on / what they’re touching with their changes. Mileage will definitely vary in bigger teams / businesses. 100% helps that my performance isn’t tied to number of PRs reviewed etc.
- pmezard 3y ago
  Another way to frame it is "code review is not the right place to validate design decisions". This type of comment is the hardest to make for me as I know it means redoing a large part of the work from scratch. It would generally be avoided by having quick design reviews before starting to code something difficult or involving sweeping changes. Just highlight how you are going to do it in a few sentences. On the other hand, letting these kind of things go through usually means you will have to deal with the outcome later, and it will be more painful. Maybe one way to decide about making this call or not is if you find the submitted version bad enough you are ready to implement the alternative yourself, immediately. And it happens. With junior devs, or because people are new to the codebase.
  - Zababa 3y ago
    > Another way to frame it is "code review is not the right place to validate design decisions". That's a good way to put it. I wonder if draft PRs/MRs could be used for that? Make a draft PR/MR with the design, do a design review, then implement the code and finally the code review.
koolba 3y ago
> While I have never been at a place that did this, I have in my head the idea that the code should be an unfolding collective conversation, kind of like when folks are all collaborating on a shared Google Doc, I see that you are editing this section and I throw in a quick comment “don't forget to add XYZ” and then jump to a different part that I won't be stepping on their toes with. You just discovered pair programming. It works astonishingly well. The second pair of eyes not only catches errors and envisions expanded use cases, it also prevents you from shirking off to HN. The biggest problem is you need two people, preferably sitting right next to each other with just one person “driving”.
- williamcotton 3y ago
  With the right pair it is unbelievable how much productivity can be unleashed. I concur, it's much easier to stay focused on the task at hand with someone actively working on the same thing. The reason why this didn't catch on is that it's almost like a Mick and Keith sort of relationship. You can't just take any two musicians and throw them together and get the Rolling Stones and the same thing applies to pair programming. Take a look at Bell Labs during the birth of UNIX for an entire SWARM of interdependent engineers. There's more than just a small element of good fortune! It seems those people were almost meant for one another.
  - MaxBarraclough 3y ago
    > You can't just take any two musicians and throw them together and get the Rolling Stones and the same thing applies to pair programming. I don't see that effective teamwork should necessarily depend on compatible personalities. I think the better comparison would be to surgery teams, or pilot crews, rather than art.
- samus 3y ago
  It works well if the tasks are short and well-described. Long, complicated tasks descend into odysseys where one person zones out or gets completely lost as the other person just ends up treating them as a clumsy proxy for the IDE. Furthermore, it rarely works online.
jaggederest 3y ago
In this kind of context, I ask people what log level they'd like their review at. If you just want to get the code out the door, by all means, "error" or "warn" might be the right review depth, when you're confident in your code and don't want to be derailed with philosophy. If you're exploring a new concept and want all the ideas and brainstorming you can get in your feedback, "debug" log level is appropriate. Once that idea has moved down the pipe, you may be down to "info" or "warn" depending on how much conversation has happened around the PR.
- rad_gruchalski 3y ago
  > In this kind of context, I ask people what log level they'd like their review at. It's a code review, not a QA session.
  - jhardy54 3y ago
    Isn’t QA the whole point of code review?
    - rad_gruchalski 3y ago
      Code review is part of the QA but code review is code review. Tell me everything what you see, don't selectively skip things.
- plugin-baby 3y ago
  This is a really useful analogy, thank you for sharing it.
michaelrpeskin 3y ago
I'm a principal so I rarely write code now. I probably do 6 PRs a year, basically when its directly related to my expertise and it makes more sense for me to do it. But when I do, I really try to guide the reviewer into my thought process. In your "centralize cache invalidation", I would have in the PR (and probably as comments in the code too) "I moved the cache invalidation to this function to centralizes the logic, it avoids problems X, Y, and Z which we've seen before". I find that giving a good narrative gets the reviewer thinking like you or at least tells them why you chose one path and not another so that they don't waste time with a "why didn't you do it this way" question. I keep trying to train my team do to that, but they're so focused on completing tickets they don't want to take the extra time to explain their "whys" and thought processes.
xorcist 3y ago
> Suddenly someone is like “I would have done it from this other approach” and you have no idea whether it's tractable I find myself on the other side of this, all the time. Suddenly someone comes up with some work that they have done without involving anyone else. I know this codebase, and I know moving that cache invalidation will make the codebase harder to understand, and also runs counter to the multi month effort you had to move all cache handling to the shared library which every other codebase in the product uses. I try to be very careful about it, "have you considered using this other approach instead?" usually means unless you do this there will be the need for an even bigger rewrite in the future, but nobody can really take comfort in that. The answer is that post facto code review is really unsuited for collaborative development. You start some work, you better involve other people straight away. Bounce ideas with a partner or two that really knows the codebase so you both understand what should be done and why. Unless you agree what it is your suggested change should achieve, there can be no useful code review! When you are very a tiny team this mostly follows naturally, but always gets lost when the team grows and are split in several, and when individual developers starts to lose track of the codebase as a whole.
- abcdrrr 3y ago
  [dead]

yarekt 3y ago

This is called Continuous Integration, and its a shame that the term got nicked to now mean "that thing that builds our PRs somewhere". The idea was that everyone pushes to main all the time, which basically reduces integration time to 0, as everyone is doing it every couple of minutes on big teams. After a while you learn how to not step on people's toes (Introduce new classes incrementally, use docblocs documenting class' intent, rather than just current functionality) I too find that its basically impossible to suggest switching to this workflow, given the weight of all our existing tools. They are so easy to setup, and most come for free (Azure Devops/Pipelines thing) that going off the track is just unthinkable.

eru 3y ago
Everyone committing to the main branch all the time sounds like a nightmare to me, to be honest. I would probably seriously consider quitting if this was forced on me. (Or rather, I would probably just run on my private git copy, and only pull every once in a while, and ignore that the main branch always changes.) When / how do you do code review in your suggested workflow?
- gizzlon 3y ago
  You can have branches, but the ""rule"" is that no branch should live for more than, say, a day. This trades the integration complexity and problems for some new.. challenges :)
  - pmontra 3y ago
    Do people start working on something new past 3 PM or do they start discussing what to do the next day?
    - bobthepanda 3y ago
      My team uses this pattern and generally speaking it looks something like this * everybody is good at breaking down code into changes small enough review quickly but not small enough to become tedious and trivial * people work on CRs whenever they have time. They do generally not post them after people have started going offline, and wait til next morning as a courtesy, since there is no difference between publishing for review at 5pm vs 9am the next day One side effect of this workflow is that because the pieces are more manageable, less uninterrupted “flow” time is required to complete a bunch of small things than to make one really big change. And others digest the smaller changes easier and knowledge spreads more effectively. And with the time its easy to say “don’t make people review outside working hours.”
  - mannykannot 3y ago
    They are not new challenges, they are the same old challenge that led to version control procedures and systems in the first place: your incomplete or broken code is interfering with my attempts to complete or fix mine!

bernds74 3y ago

"I would have done it from this other approach". I've seen that, and it's not good when you get the feeling of "code review is when someone who hasn't thought about your problem tells you how you should have solved it". People sometimes feel they have to add value as a reviewer, and casually discarding other people's work is the way to do it. Fortunately it's not something I have to deal with at my current job.

dijksterhuis 3y ago
I’ve made comments that start like that and it’s usually down to 1) obvious code smell, here’s an example using your existing code refactored and the reasons why it is a better fit here 2) you’ve done something I didn’t think of and it’s clearly better than the way I was thinking of it. Here’s why it’s better. Kudos! Helps that I’m lead/principal in a small biz with like 4 people max writing code. So I kind of know what everyone is working on / what they’re touching with their changes. Mileage will definitely vary in bigger teams / businesses. 100% helps that my performance isn’t tied to number of PRs reviewed etc.
pmezard 3y ago
Another way to frame it is "code review is not the right place to validate design decisions". This type of comment is the hardest to make for me as I know it means redoing a large part of the work from scratch. It would generally be avoided by having quick design reviews before starting to code something difficult or involving sweeping changes. Just highlight how you are going to do it in a few sentences. On the other hand, letting these kind of things go through usually means you will have to deal with the outcome later, and it will be more painful. Maybe one way to decide about making this call or not is if you find the submitted version bad enough you are ready to implement the alternative yourself, immediately. And it happens. With junior devs, or because people are new to the codebase.
- Zababa 3y ago
  > Another way to frame it is "code review is not the right place to validate design decisions". That's a good way to put it. I wonder if draft PRs/MRs could be used for that? Make a draft PR/MR with the design, do a design review, then implement the code and finally the code review.

koolba 3y ago

> While I have never been at a place that did this, I have in my head the idea that the code should be an unfolding collective conversation, kind of like when folks are all collaborating on a shared Google Doc, I see that you are editing this section and I throw in a quick comment “don't forget to add XYZ” and then jump to a different part that I won't be stepping on their toes with. You just discovered pair programming. It works astonishingly well. The second pair of eyes not only catches errors and envisions expanded use cases, it also prevents you from shirking off to HN. The biggest problem is you need two people, preferably sitting right next to each other with just one person “driving”.

williamcotton 3y ago
With the right pair it is unbelievable how much productivity can be unleashed. I concur, it's much easier to stay focused on the task at hand with someone actively working on the same thing. The reason why this didn't catch on is that it's almost like a Mick and Keith sort of relationship. You can't just take any two musicians and throw them together and get the Rolling Stones and the same thing applies to pair programming. Take a look at Bell Labs during the birth of UNIX for an entire SWARM of interdependent engineers. There's more than just a small element of good fortune! It seems those people were almost meant for one another.
- MaxBarraclough 3y ago
  > You can't just take any two musicians and throw them together and get the Rolling Stones and the same thing applies to pair programming. I don't see that effective teamwork should necessarily depend on compatible personalities. I think the better comparison would be to surgery teams, or pilot crews, rather than art.
samus 3y ago
It works well if the tasks are short and well-described. Long, complicated tasks descend into odysseys where one person zones out or gets completely lost as the other person just ends up treating them as a clumsy proxy for the IDE. Furthermore, it rarely works online.

jaggederest 3y ago

In this kind of context, I ask people what log level they'd like their review at. If you just want to get the code out the door, by all means, "error" or "warn" might be the right review depth, when you're confident in your code and don't want to be derailed with philosophy. If you're exploring a new concept and want all the ideas and brainstorming you can get in your feedback, "debug" log level is appropriate. Once that idea has moved down the pipe, you may be down to "info" or "warn" depending on how much conversation has happened around the PR.

rad_gruchalski 3y ago
> In this kind of context, I ask people what log level they'd like their review at. It's a code review, not a QA session.
- jhardy54 3y ago
  Isn’t QA the whole point of code review?
  - rad_gruchalski 3y ago
    Code review is part of the QA but code review is code review. Tell me everything what you see, don't selectively skip things.
plugin-baby 3y ago
This is a really useful analogy, thank you for sharing it.

michaelrpeskin 3y ago

I'm a principal so I rarely write code now. I probably do 6 PRs a year, basically when its directly related to my expertise and it makes more sense for me to do it. But when I do, I really try to guide the reviewer into my thought process. In your "centralize cache invalidation", I would have in the PR (and probably as comments in the code too) "I moved the cache invalidation to this function to centralizes the logic, it avoids problems X, Y, and Z which we've seen before". I find that giving a good narrative gets the reviewer thinking like you or at least tells them why you chose one path and not another so that they don't waste time with a "why didn't you do it this way" question. I keep trying to train my team do to that, but they're so focused on completing tickets they don't want to take the extra time to explain their "whys" and thought processes.

xorcist 3y ago

> Suddenly someone is like “I would have done it from this other approach” and you have no idea whether it's tractable I find myself on the other side of this, all the time. Suddenly someone comes up with some work that they have done without involving anyone else. I know this codebase, and I know moving that cache invalidation will make the codebase harder to understand, and also runs counter to the multi month effort you had to move all cache handling to the shared library which every other codebase in the product uses. I try to be very careful about it, "have you considered using this other approach instead?" usually means unless you do this there will be the need for an even bigger rewrite in the future, but nobody can really take comfort in that. The answer is that post facto code review is really unsuited for collaborative development. You start some work, you better involve other people straight away. Bounce ideas with a partner or two that really knows the codebase so you both understand what should be done and why. Unless you agree what it is your suggested change should achieve, there can be no useful code review! When you are very a tiny team this mostly follows naturally, but always gets lost when the team grows and are split in several, and when individual developers starts to lose track of the codebase as a whole.

abcdrrr 3y ago
[dead]