4 ms·

This is the part that I don't understand, >Even if each round of review takes 2 days and there are 2-3 such iterations, it is easy for the author to stay block

by omouse 11y ago

This is the part that I don't understand, >Even if each round of review takes 2 days and there are 2-3 such iterations, it is easy for the author to stay blocked for good part of a week leading to wasted time. How can the author of the code be blocked while waiting for a code review? Can't they use that time to review someone else's code or to work on something else in a separate branch or, heaven forbid, learn or train themselves? I mean we never seem to find time to train developers but the time between writing code and having it pass review could be used for education not just sitting around and waiting. >For code reviews to work well, each change should be reviewed by the people who have enough context on that change. It's even better if these code reviewers are responsible for “maintaining” the code they are reviewing so that they are incentivized to make the right long-term trade-offs This makes sense at first but don't you also want more people looking at the code who don't have complete context so that the context must be explained to them and then they can learn more about the code base? Isn't reducing the bus factor also a priority?

awinder 11y ago
I love this comment. I've heard this actually voiced by developers having trouble coping with code reviews -- "I'm blocked, I can't work on anything else". Think you're right on with the idea of using it for training / research / pet project time.
- omouse 11y ago
  I've actually gotten into trouble because I spent more than 5 minutes doing a code review of a pull request. I had to code review for 3 junior devs and one senior; the junior devs all knew little Python so only I could review their code while the other senior dev was busy. It took hours to wade through and double-check their code. I always suggested to them to look up some Python libraries or to work on puzzles to practice or to review the code as well or talk to each other about the next piece of work. There's so many other things to do than to keep coding.
  - gknoy 11y ago
    That sounds less like "code review", and more like "Mentoring". If you find a way to position it that way, you can bring that up with your boss(es) before your next performance review. I've found mentoring other developers (whether it's explaining "Here's the facets of how we test All The Things", or "Here's how git branching works") to be profoundly fulfilling. I love explaining things. The best part is, fostering the idea of teaching each other means that others can mentor YOU as well on things, and you (and they) don't need to feel threatened.
chadaustin 11y ago
The problem is not that the time waiting on a code review can't be used in some other way. It's that the constant interruptions seriously hurt straight-line productivity. "OK, done with this minor patch. Now, uhhh, while I wait for code review, I guess I'll go review this other thing... And then start a new patch.... Code review is done now? Better context-switch back." I'm not strictly opposed to pre-commit reviews, but I think it's incorrect to be dismissive of their cost.
- mkozlows 11y ago
  I agree with this to some extent, but it can be ameliorated a lot with smaller and more frequent PRs. If your PR is enormous and represents a week's worth of work and you end up having to respond to 20 comments, it's going to be awful. If your PR is small and represents hours' worth of work and you have to respond to 1-2 comments, it's a lot less obnoxious. (And you'll have more natural opportunities for context-switching, so responding to comments won't be so distracting.)
  - deepGem 11y ago
    I can't agree more to this approach. Smaller changes are easy to review and manaage. We developers need a push to make this happen though.
    - MaulingMonkey 11y ago
      This also needs proper VCS support - by which I mean DVCSes that let you pile commits up to the same files one after another sanely, or reviews after commit rather than before (better have a good branching setup!) My last job was perforce (no unsubmitted commit stacking) with everyone developing directly against a single branch. In theory, code reviews were supposed to happen before hand. In practice, I submitted small changes from the hip, and complained when my coworkers gave me a week long spitball of unrelated features in a single mega-commit. Not that I wasn't guilty of that myself. Bleh.
  - yummyfajitas 11y ago
specialk 11y ago
I completely agree that how can you get blocked waiting for a code review? Some concurrency is wonderful for avoiding being blocked. In work I currently have 3 pull requests open all touching different sections of the product. The 3 PRs are all at different stages of the review process. For example, I'll be closing one first thing tomorrow and deploying it out (don't like deploying in the evenings - its bad luck). Another I just opened as I left work today, so I doubt anyone's reviewed it yet. I'm sure Quora has a big enough codebase and not enough developers (like most companies) that they always have a list of things a mile long that they would like to do or should do, be that new features or just refactoring. 2-3 days isn't long for a review and iteration cycle in itself. I have one PR that is currently open for 5 working days at this stage. We decided that it didn't quite fit the bill on the first draft . It is blocking progress on two other tasks in our backlog but we also have 14 things on our backlog of goals for this week. The team is working around it while we finish it off or create issues for things we think have fallen outside the scope of the initial job. One thing we've noticed alot on my team is that we try and make our pull requests for review as small as possible. We all have a rough guide of about max 500 lines. Sometimes we do go over but its a rough guideline. Smaller PRs are just easier to review for everyone involved. Reviewers don't have walls of code to break through or need to keep tons of context in their head. Post-commit reviews (or post-merge into master or whatever you process is) is an interesting idea. I'd be curious to see what my team think of the idea tomorrow for a small sub set of our codebase.
superuser2 11y ago
>Can't they use that time to review someone else's code or to work on something else in a separate branch or, heaven forbid, learn or train themselves? Yes, but context-switches are expensive, particularly when you have several reviews in progress across several projects.

by omouse 11y ago

awinder 11y ago

I love this comment. I've heard this actually voiced by developers having trouble coping with code reviews -- "I'm blocked, I can't work on anything else". Think you're right on with the idea of using it for training / research / pet project time.

omouse 11y ago
I've actually gotten into trouble because I spent more than 5 minutes doing a code review of a pull request. I had to code review for 3 junior devs and one senior; the junior devs all knew little Python so only I could review their code while the other senior dev was busy. It took hours to wade through and double-check their code. I always suggested to them to look up some Python libraries or to work on puzzles to practice or to review the code as well or talk to each other about the next piece of work. There's so many other things to do than to keep coding.
- gknoy 11y ago
  That sounds less like "code review", and more like "Mentoring". If you find a way to position it that way, you can bring that up with your boss(es) before your next performance review. I've found mentoring other developers (whether it's explaining "Here's the facets of how we test All The Things", or "Here's how git branching works") to be profoundly fulfilling. I love explaining things. The best part is, fostering the idea of teaching each other means that others can mentor YOU as well on things, and you (and they) don't need to feel threatened.

chadaustin 11y ago

The problem is not that the time waiting on a code review can't be used in some other way. It's that the constant interruptions seriously hurt straight-line productivity. "OK, done with this minor patch. Now, uhhh, while I wait for code review, I guess I'll go review this other thing... And then start a new patch.... Code review is done now? Better context-switch back." I'm not strictly opposed to pre-commit reviews, but I think it's incorrect to be dismissive of their cost.

mkozlows 11y ago
I agree with this to some extent, but it can be ameliorated a lot with smaller and more frequent PRs. If your PR is enormous and represents a week's worth of work and you end up having to respond to 20 comments, it's going to be awful. If your PR is small and represents hours' worth of work and you have to respond to 1-2 comments, it's a lot less obnoxious. (And you'll have more natural opportunities for context-switching, so responding to comments won't be so distracting.)
- deepGem 11y ago
  I can't agree more to this approach. Smaller changes are easy to review and manaage. We developers need a push to make this happen though.
  - MaulingMonkey 11y ago
    This also needs proper VCS support - by which I mean DVCSes that let you pile commits up to the same files one after another sanely, or reviews after commit rather than before (better have a good branching setup!) My last job was perforce (no unsubmitted commit stacking) with everyone developing directly against a single branch. In theory, code reviews were supposed to happen before hand. In practice, I submitted small changes from the hip, and complained when my coworkers gave me a week long spitball of unrelated features in a single mega-commit. Not that I wasn't guilty of that myself. Bleh.
- yummyfajitas 11y ago

specialk 11y ago

I completely agree that how can you get blocked waiting for a code review? Some concurrency is wonderful for avoiding being blocked. In work I currently have 3 pull requests open all touching different sections of the product. The 3 PRs are all at different stages of the review process. For example, I'll be closing one first thing tomorrow and deploying it out (don't like deploying in the evenings - its bad luck). Another I just opened as I left work today, so I doubt anyone's reviewed it yet. I'm sure Quora has a big enough codebase and not enough developers (like most companies) that they always have a list of things a mile long that they would like to do or should do, be that new features or just refactoring. 2-3 days isn't long for a review and iteration cycle in itself. I have one PR that is currently open for 5 working days at this stage. We decided that it didn't quite fit the bill on the first draft . It is blocking progress on two other tasks in our backlog but we also have 14 things on our backlog of goals for this week. The team is working around it while we finish it off or create issues for things we think have fallen outside the scope of the initial job. One thing we've noticed alot on my team is that we try and make our pull requests for review as small as possible. We all have a rough guide of about max 500 lines. Sometimes we do go over but its a rough guideline. Smaller PRs are just easier to review for everyone involved. Reviewers don't have walls of code to break through or need to keep tons of context in their head. Post-commit reviews (or post-merge into master or whatever you process is) is an interesting idea. I'd be curious to see what my team think of the idea tomorrow for a small sub set of our codebase.

superuser2 11y ago

>Can't they use that time to review someone else's code or to work on something else in a separate branch or, heaven forbid, learn or train themselves? Yes, but context-switches are expensive, particularly when you have several reviews in progress across several projects.