6 ms·

I don’t have your experience but I personally think some of this feedback can be warranted. > Can't refactor code because it changes too many files and too man

I don’t have your experience but I personally think some of this feedback can be warranted. > Can't refactor code because it changes too many files and too many lines. This really depends on the change. If you are just doing a mass rename like updating a function signature, fair enough but if you changing a lot of code it’s very hard to review it. Lots of cognitive load on the reviewer who might not have the same understanding of codebase as you. > Can't commit large chunks of well tested code that 'Does feature X', because... too many files and too many lines. Same as the above, reviewing is hard and more code means people get lazy and bored. Just because the code is tested doesn’t mean it’s correct, just means it passes tests. > Have to split everything down into a long sequence of consecutive pull requests that become a process nightmare in its own right This is planning issue, if you correctly size tickets you aren’t going to end up in messy situations as often. > The documentation comments gets nitpicked to death with mostly useless comments about not having periods at the ends of lines Having correctly written documentation is important. It can live a long time and if you don’t keep an eye on it can becomes a mess. Ideally you should review it before you submitting it to avoid these issues. > End up having to explain every little detail throughout the function as if I'm trying to produce a lecture, things like `/* loop until not valid */ while (!valid) {...` seemed to be what they wanted, but to me it made no sense what so ever to even have that comment I definitely agree with this one. Superfluous comments are a waste of time. Obviously this is just my option and you can take things too far but I do think that making code reviewable (by making it small) goes a long way. No one wants to review 1000s lines of code at once. It’s too much to process and people will do a worse job. Happy to hear your thoughts.

quesomaster9000 2y ago
I do object to the notion of something being a planning issue when you're talking about a days worth of work. Implement X, needs Y and Z, ok that was straightforward, also discovered U and V on the way and sorted that out, here's a pull request that neatly wraps it up. Which subsequently gets turned into a multi-week process, going back & forth almost every day, meaning I can't move on to the next thing, meanwhile I'm looking at the cumulative hourly wages of everybody involved and the cost is... shocking. Death by process IHMO.
- bspammer 2y ago
  > Implement X, needs Y and Z, ok that was straightforward, also discovered U and V on the way and sorted that out, here's a pull request that neatly wraps it up This sounds very difficult to review to be honest. At a minimum unrelated changes should be in their own pull request (U and V in your example).
  - pbh101 2y ago
    Agree. Another item here that is contextual: what is the cost of a bug? Does it cost millions, do we find that out immediately, or does it take months? Or does it not really matter, and when we’ll find the big it will be cheap? The OP joining a new company might not have the context that existing employees have about why we’re being cautious/clear about what we’re changing as opposed to smuggling in refactors in the same PR as a feature change. I’m going to be the guy that is asking for a refactor to be in a separate commit/PR from the feature and clearly marked. It doesn’t justify everything else he mentioned (especially the comments piece) but once you get used to this it doesn’t need to extend timelines.
  - shakna 2y ago
    How do you rework a core process, then? If you rework a major unit that touches just about everything... Sharding something like that can break the actual improvement it is trying to deliver. Like... Increase the performance of a central VM. You'll touch every part of the code, but probably also build a new compiler analysis system. The system is seperate to existing code, but useless without the core changes. Seperating the two can ruin the optimisation meant to be delivered, because the context is no longer front and center. Allowing more quibling to degrade the changes.
  - tacitusarc 2y ago
    I work as a tech lead, so I get a lot of leeway in setting process. For small PRs, we use the normal “leave comments, resolve comments” approach. For large PRs, we schedule 30m meetings, where the submitter can explain the changes and answer questions, and record any feedback. This ensures everyone is on the same page with the changes, gives folks a chance to rapidly gather feedback, and helps familiarize devs who do not work in that area with what is going on. If the meeting is insufficient to feel like everyone is on the same page and approves the changes, we schedule another one. These are some of the best meetings we have. They are targeted, educational, and ensure we don’t have long delays waiting for code to go in. Instead of requiring every PR to be small, which has a high cost, I recommend doing this for large/complex projects. One additional thing to note on small PRs: often, they require significant context, which could take hours or even days, to be built up repeatedly. Contrast that with being able to establish context, and then solve several large problems all at once. The latter is more efficient, so if it can be enabled without negative side effects, it is really valuable. I want my team to be productive, and I want to empower them to improve the codebase whenever they see an opportunity, even if it is not related to their immediate task.
lazyasciiart 2y ago
> This is planning issue, if you correctly size tickets you aren’t going to end up in messy situations as often. No, it’s “this refactor looks very different to the original code because the original code thought it was doing two different things and it’s only by stepping through it with real customer data that you realized with the right inputs (not documented) it could do a third thing (not documented) that had very important “side effects” and was a no-op in the original code flow. Yea, it touches a lot of files. Ok, yea, I can break it up step by step, and wait a few days between approval for each of them so that you never have to actually understand what just happened”.
- grey-area 2y ago
  The way I normally approach this is one big pr for context and then break it into lots of small ones for review.
  - F-W-M 2y ago
    A PR with self-contained smaller commits would be possible as well.
    - t-writescode 2y ago
      Yes, though it does depend on how good the commenting system is; and, for something like that, you're still probably going to want a meeting to walk people through such a huge change. And you'd better hope you're not squashing that monstrous thing when you're done.
  - jaredsohn 2y ago
    I've found processes like this to work better, too. Basically, the one big pr is like building a prototype to throw away. And the benefit is it has to get thrown away because the PR will never pass review.
- deleted 2y ago
  [deleted]
- 2y ago
callc 2y ago
> This is planning issue, if you correctly size tickets you aren’t going to end up in messy situations as often. I think the underlying issue is what is an appropriate “unit of work”. Parent commenter may want to ship a complete/entire feature in one MR. Ticketing obsessed people will have some other metric. Merge process may be broken in this aspect. I would rather explain to reviewer to bring them up to speed on the changes to make their cognitive load easier
- gjadi 2y ago
  This. The solution to long and multiple reviews to MR is single pair review session where most of the big picture aspects can be addressed immediately and verbally discussed and challenged. IMHO it is the same as chat. If talking about an issue over mail or chat takes more than 3-5 messages, trigger a call to solve it face to face.
8note 2y ago
code reviews that are too small, i think are worse than ones that are too big, and let through more bugs. 10 different reviewers can each look at a 100 lin change out of the 1000 line total change, but each miss how the changes work together. theyre all lying by approving, since they dont have the right context to approve

quesomaster9000 2y ago

I do object to the notion of something being a planning issue when you're talking about a days worth of work. Implement X, needs Y and Z, ok that was straightforward, also discovered U and V on the way and sorted that out, here's a pull request that neatly wraps it up. Which subsequently gets turned into a multi-week process, going back & forth almost every day, meaning I can't move on to the next thing, meanwhile I'm looking at the cumulative hourly wages of everybody involved and the cost is... shocking. Death by process IHMO.

bspammer 2y ago
> Implement X, needs Y and Z, ok that was straightforward, also discovered U and V on the way and sorted that out, here's a pull request that neatly wraps it up This sounds very difficult to review to be honest. At a minimum unrelated changes should be in their own pull request (U and V in your example).
- pbh101 2y ago
  Agree. Another item here that is contextual: what is the cost of a bug? Does it cost millions, do we find that out immediately, or does it take months? Or does it not really matter, and when we’ll find the big it will be cheap? The OP joining a new company might not have the context that existing employees have about why we’re being cautious/clear about what we’re changing as opposed to smuggling in refactors in the same PR as a feature change. I’m going to be the guy that is asking for a refactor to be in a separate commit/PR from the feature and clearly marked. It doesn’t justify everything else he mentioned (especially the comments piece) but once you get used to this it doesn’t need to extend timelines.
- shakna 2y ago
  How do you rework a core process, then? If you rework a major unit that touches just about everything... Sharding something like that can break the actual improvement it is trying to deliver. Like... Increase the performance of a central VM. You'll touch every part of the code, but probably also build a new compiler analysis system. The system is seperate to existing code, but useless without the core changes. Seperating the two can ruin the optimisation meant to be delivered, because the context is no longer front and center. Allowing more quibling to degrade the changes.
- tacitusarc 2y ago
  I work as a tech lead, so I get a lot of leeway in setting process. For small PRs, we use the normal “leave comments, resolve comments” approach. For large PRs, we schedule 30m meetings, where the submitter can explain the changes and answer questions, and record any feedback. This ensures everyone is on the same page with the changes, gives folks a chance to rapidly gather feedback, and helps familiarize devs who do not work in that area with what is going on. If the meeting is insufficient to feel like everyone is on the same page and approves the changes, we schedule another one. These are some of the best meetings we have. They are targeted, educational, and ensure we don’t have long delays waiting for code to go in. Instead of requiring every PR to be small, which has a high cost, I recommend doing this for large/complex projects. One additional thing to note on small PRs: often, they require significant context, which could take hours or even days, to be built up repeatedly. Contrast that with being able to establish context, and then solve several large problems all at once. The latter is more efficient, so if it can be enabled without negative side effects, it is really valuable. I want my team to be productive, and I want to empower them to improve the codebase whenever they see an opportunity, even if it is not related to their immediate task.

lazyasciiart 2y ago

> This is planning issue, if you correctly size tickets you aren’t going to end up in messy situations as often. No, it’s “this refactor looks very different to the original code because the original code thought it was doing two different things and it’s only by stepping through it with real customer data that you realized with the right inputs (not documented) it could do a third thing (not documented) that had very important “side effects” and was a no-op in the original code flow. Yea, it touches a lot of files. Ok, yea, I can break it up step by step, and wait a few days between approval for each of them so that you never have to actually understand what just happened”.

grey-area 2y ago
The way I normally approach this is one big pr for context and then break it into lots of small ones for review.
- F-W-M 2y ago
  A PR with self-contained smaller commits would be possible as well.
  - t-writescode 2y ago
    Yes, though it does depend on how good the commenting system is; and, for something like that, you're still probably going to want a meeting to walk people through such a huge change. And you'd better hope you're not squashing that monstrous thing when you're done.
- jaredsohn 2y ago
  I've found processes like this to work better, too. Basically, the one big pr is like building a prototype to throw away. And the benefit is it has to get thrown away because the PR will never pass review.
deleted 2y ago
[deleted]
2y ago

callc 2y ago

> This is planning issue, if you correctly size tickets you aren’t going to end up in messy situations as often. I think the underlying issue is what is an appropriate “unit of work”. Parent commenter may want to ship a complete/entire feature in one MR. Ticketing obsessed people will have some other metric. Merge process may be broken in this aspect. I would rather explain to reviewer to bring them up to speed on the changes to make their cognitive load easier

gjadi 2y ago
This. The solution to long and multiple reviews to MR is single pair review session where most of the big picture aspects can be addressed immediately and verbally discussed and challenged. IMHO it is the same as chat. If talking about an issue over mail or chat takes more than 3-5 messages, trigger a call to solve it face to face.

8note 2y ago

code reviews that are too small, i think are worse than ones that are too big, and let through more bugs. 10 different reviewers can each look at a 100 lin change out of the 1000 line total change, but each miss how the changes work together. theyre all lying by approving, since they dont have the right context to approve