5 ms·

The other problem is that these self-imposed roadblocks are so engrained in the modern SDLC that developers literally cannot imagine a world where they do not e

by vrosas 3y ago

The other problem is that these self-imposed roadblocks are so engrained in the modern SDLC that developers literally cannot imagine a world where they do not exist. I got _reamed_ by some "senior" engineers for merging a small PR without an approval recently. And we're not some megacorp, we're a 12 person engineering startup! We can make our own rules! We don't even have any customers...

insanitybit 3y ago
Indeed, dogmatic adherence to arbitrary patterns is a huge problem in our field. People have strong beliefs, "X good" or "X bad", with almost no idea of what X even is, what the alternatives are, why X was something people did or did not like, etc.
- jacquesm 3y ago
  Another problem is people making decisions with potentially far reaching consequences well above their paygrade without authorization.
  - insanitybit 3y ago
    I don't think that problem is nearly as significant.
    - jacquesm 3y ago
      What you think is usually the limit of your experience, which effectively makes it anecdata. I've looked at enough companies and dealt with the aftermath of enough such instances that I beg to differ. It's possible that due to the nature of my business my experience is skewed in the other direction which makes that anecdata as well. But it is probably more important than you think (and less important than I think).
jacquesm 3y ago
Your 'senior' engineer is likely right: they are trying to get some kind of process going and you are actively sabotaging that. This could come back to haunt you later on when you by your lonesome decide to merge a 'small PR' with massive downtime as a result of not having your code reviewed. Ok, you say, I'm perfect. And I believe you. But now you have another problem: the other junior devs on your team who see vrosas commit and merge stuff by themselves will see you as their shining example. And as a result they by their lonesomes decide to merge 'small PR's with massive downtime as a result. If you got _reamed_ you got off lucky: in plenty of places you'd be out on the street. It may well be that you had it right but from context as given I hope this shows you some alternative perspective that might give you pause the next time you decide to throw out the rulebook, even in emergencies - especially in emergencies - these rules are there to keep you, your team and the company safe. In regulated industries you can multiply all of that by a factor of five or so.
- vrosas 3y ago
  I'm challenging my team to actually think about that process, why it's in place, how it's helping (or actively hurting!) us. Comparing ourselves to companies that have regulatory requirements (spoiler: we don't and likely won't for a long, long time) just furthers my point that no one really thinks about these things. They just cargo cult how everyone else does it.
  - jacquesm 3y ago
    You can challenge them without actually violating established process. I wasn't comparing you to companies that have regulatory requirements, I was merely saying that all of the above will factor in much, much stronger still in a regulated industry. But not being in a regulated industry doesn't mean there isn't a very good reason to have a code review in your process, assuming it is used effectively and not for nitpicking. Not having a code review step is usually a bad idea, unless everybody on your team is of absolutely amazing quality and they never make silly mistakes. I've yet to come across a team like that, but maybe you are the exception to the rule.
- insanitybit 3y ago
elzbardico 3y ago
Really man. I have almost two decades developing software and yet, I feel a lot more comfortable having all my code reviewed. If anything I get annoyed by junior developers in my team when they just rub-stamp my PRs because supposedly I am this super senior guy that can't err. Code Reviews are supposed to give you peace of mind, not being a hassle. During all this time, I've seen plenty of "small changes" having completely unexpected consequences, and sometimes all it would take to avoid would someone else seeing it from another perspective.
- vrosas 3y ago
  Not a man. I’m not convinced bad code would get merged more often if we didn’t require approvals. I am convinced we’d deliver code faster though, and that’s what I’m trying to optimize for. Your company and engineering problems are not the same as mine.
  - jeltz 3y ago
    Optimizing for delivering faster is usually the wrong thing to optimize for. Usually the issue is making sure the right things are delivered.
- foobiekr 3y ago
  At 30 years of coding professionally in great engineering-focused organizations, and that on top of nearly a lifetime of having coded for myself, I’ve concluded code reviews barely work. I agree with everything you say here, but honestly it’s quite ineffective. I wish we had found something better than unit tests (often misleading and fragile) and the ability of a qualified reviewer to maintain attention and context that a real review entails.
  - vvanders 3y ago
    IMO catching bugs is a nice side-effect of code reviews. The primary value I've seen across teams has been more on having shared team context across a codebase, if something goes bump in the night you've got a sense on how that part of the codebase works. It's also a great opportunity for other engineers to ask "why" and explain parts that aren't obvious or other context that's relevant. We'll find the occasional architectural mismatch(although we like to catch those much earlier in the design process) and certainly prevented bugs from shipping but if that's the primary focus I think a team is missing a lot of the value from regular code reviews.

by vrosas 3y ago

insanitybit 3y ago

Indeed, dogmatic adherence to arbitrary patterns is a huge problem in our field. People have strong beliefs, "X good" or "X bad", with almost no idea of what X even is, what the alternatives are, why X was something people did or did not like, etc.

jacquesm 3y ago
Another problem is people making decisions with potentially far reaching consequences well above their paygrade without authorization.
- insanitybit 3y ago
  I don't think that problem is nearly as significant.
  - jacquesm 3y ago
    What you think is usually the limit of your experience, which effectively makes it anecdata. I've looked at enough companies and dealt with the aftermath of enough such instances that I beg to differ. It's possible that due to the nature of my business my experience is skewed in the other direction which makes that anecdata as well. But it is probably more important than you think (and less important than I think).

jacquesm 3y ago

Your 'senior' engineer is likely right: they are trying to get some kind of process going and you are actively sabotaging that. This could come back to haunt you later on when you by your lonesome decide to merge a 'small PR' with massive downtime as a result of not having your code reviewed. Ok, you say, I'm perfect. And I believe you. But now you have another problem: the other junior devs on your team who see vrosas commit and merge stuff by themselves will see you as their shining example. And as a result they by their lonesomes decide to merge 'small PR's with massive downtime as a result. If you got _reamed_ you got off lucky: in plenty of places you'd be out on the street. It may well be that you had it right but from context as given I hope this shows you some alternative perspective that might give you pause the next time you decide to throw out the rulebook, even in emergencies - especially in emergencies - these rules are there to keep you, your team and the company safe. In regulated industries you can multiply all of that by a factor of five or so.

vrosas 3y ago
I'm challenging my team to actually think about that process, why it's in place, how it's helping (or actively hurting!) us. Comparing ourselves to companies that have regulatory requirements (spoiler: we don't and likely won't for a long, long time) just furthers my point that no one really thinks about these things. They just cargo cult how everyone else does it.
- jacquesm 3y ago
  You can challenge them without actually violating established process. I wasn't comparing you to companies that have regulatory requirements, I was merely saying that all of the above will factor in much, much stronger still in a regulated industry. But not being in a regulated industry doesn't mean there isn't a very good reason to have a code review in your process, assuming it is used effectively and not for nitpicking. Not having a code review step is usually a bad idea, unless everybody on your team is of absolutely amazing quality and they never make silly mistakes. I've yet to come across a team like that, but maybe you are the exception to the rule.
insanitybit 3y ago

elzbardico 3y ago

Really man. I have almost two decades developing software and yet, I feel a lot more comfortable having all my code reviewed. If anything I get annoyed by junior developers in my team when they just rub-stamp my PRs because supposedly I am this super senior guy that can't err. Code Reviews are supposed to give you peace of mind, not being a hassle. During all this time, I've seen plenty of "small changes" having completely unexpected consequences, and sometimes all it would take to avoid would someone else seeing it from another perspective.

vrosas 3y ago
Not a man. I’m not convinced bad code would get merged more often if we didn’t require approvals. I am convinced we’d deliver code faster though, and that’s what I’m trying to optimize for. Your company and engineering problems are not the same as mine.
- jeltz 3y ago
  Optimizing for delivering faster is usually the wrong thing to optimize for. Usually the issue is making sure the right things are delivered.
foobiekr 3y ago
At 30 years of coding professionally in great engineering-focused organizations, and that on top of nearly a lifetime of having coded for myself, I’ve concluded code reviews barely work. I agree with everything you say here, but honestly it’s quite ineffective. I wish we had found something better than unit tests (often misleading and fragile) and the ability of a qualified reviewer to maintain attention and context that a real review entails.
- vvanders 3y ago
  IMO catching bugs is a nice side-effect of code reviews. The primary value I've seen across teams has been more on having shared team context across a codebase, if something goes bump in the night you've got a sense on how that part of the codebase works. It's also a great opportunity for other engineers to ask "why" and explain parts that aren't obvious or other context that's relevant. We'll find the occasional architectural mismatch(although we like to catch those much earlier in the design process) and certainly prevented bugs from shipping but if that's the primary focus I think a team is missing a lot of the value from regular code reviews.