5 ms·

> A PR is waaaaay too late to bring up No. That’s literally the point of a review.

by thiht 3y ago

withinboredom 3y ago
No, it's not and there is a ton of literature (even entire books) written on why it isn't the time to bring this up during a review. For starters, it results in a waste of time for literally everyone involved: 1. The person writing has to rewrite it (probably). 2. The reviewer could have sat down with the person before a single line of code was written. 3. Anyone else reviewing just wasted their time because it will be rewritten. If you see code badly designed, walk over to, or call the developer and say "hey, can we discuss the design of the code." The saying "measure twice, cut once" doesn't just apply to wood. Design before you ever write any code.
- DandyDev 3y ago
  The alternative is a big design upfront?
  - withinboredom 3y ago
    Generally, it goes something like this imaginary Slack convo: Me: I think we are getting events from SQS out of order and that’s why we are seeing some weird synchronization issues. What do you think about sending events to the regular queue and a FIFO queue at the same time and comparing them? Team: How would that work? Me: Since we are using a single threaded consumer here, I think we can simply override the transport class and instead of pulling from one queue per transport instance, we set up a feature flag allowing us to pull from two, compare the events and if they are different, alerting us in the logs. Team: Sounds good! You don’t need a full design spec, just agreement from the team on the approach. There won’t be any surprises in the review.
    - thiht 3y ago
      > There won’t be any surprises in the review. There can always be surprises. Sometimes when you see something implemented you can get a « click » as to why another solution would be better. It happens to me regularly both on the receiving and giving end of this and it’s rarely a big deal. Most of the time it happened to me, I could reuse the functional tests I had written and parts of the code anyway. Reworking on something so that it’s better is never a waste of time, and it’s always better to do it before it reaches Prod.
      - withinboredom 3y ago
        Making a suggestion on how to do something better is quite a bit different than saying “this isn’t up to standard. Do not pass go. Do not collect $200” We all see these suggestions. That’s ok, and wanted. But to say “ah, no I think it should be the other way.” [reject] That’s what I was talking about here.
- johnnyanmac 3y ago
  1. That's the point. It's not up to standard 2. They could but they usually don't. It's fine to sit down and make sure they understand some esoteric structure within a system I owj I'm not going to sit down with someone else to make sure they use for each loops unless it's a very new junior (i wouldn't block that in most code reviews anyway unless the code was legitimately painful to read, but every company is different). 3. IME there may be multiple reviewers but one primary reviewer. Half the time the non-primary ones either focus on "is this code going to break your work flow?" or are simply there for bookkeeping and isn't the approval you're looking for (i.e. A lead on a review that already talked about the plans but doesn't have the knowledge to review that specific module). Anymore than 1 primary reviewer probably needed a smaller PR if possible.
  - withinboredom 3y ago
    Anyone who has ever come along in a code review and told me my code “isn’t up to standard” will inevitably start a very long game of attrition with me. Here’s the thing: there’s no such thing as “standard” software architecture (until someone writes a new kind of software architecture called “standard”). I’ve seen and used everything from MVC to DDD to TDD to MVVC to whatever React is to reactive to event-oriented to actors to whatever you can imagine. There simply isn’t a “standard” way to do anything in this industry. If you hired me, you hired me for these different perspectives on problem solving, and that goes for all of us. A code review simply isn’t the place to enforce your unique perspectives on someone else. Sometimes, there is simply a better way to do things. These might even be new ideas in the code base. If it looks like it is, hopefully the author will bring it up with the team beforehand. If the reviewer changes their mind in the code review… well, that’s not the authors problem. The reviewer should rewrite it and the author review it. Actually, that’s exactly what I suggest if someone ever tries that with me (usually when I’m the new guy). Please submit your own PR showing me how it’s done. I’ll move on to another ticket. Usually, about half way through their implementation, they’ll see why I did it the way I did it and approve my PR. It only takes one time before they start asking “why” instead of assuming they know everything; every line of code exists for a reason, after all. In one case, one specific dude (bless his heart) kept blocking PRs after we discussed everything before hand. I had to get HR involved, and other engineers. Dude just didn’t like being wrong and would throw tantrums when he was. He eventually got fired after people realized they could do what I was doing to stop the nits he said was a blocker. Nobody cared if a variable should be renamed from “SameFactory” to “EqualFactory”. Ah, but code quality! It’s so subjective. High code quality, to me, is easy to maintain code. Is the intention clear, do the comments reflect reality, is it easy to read, but more importantly, easy to change? Will changing a line in the module break 15 other sibling modules? God I hope not. Dependencies should be obvious. And, no, I’m not writing an interface if there is exactly one implementation. That’s ridiculous. For some people, they want layers. More layers of abstraction than a layered cake on a wedding day. To them, that’s good code quality! Some people think high code quality is beautiful code. The kind you frame and put over your fire place to admire with a glass of wine. There’s no objectively “high quality” code in existence.
- Arainach 3y ago
  >The reviewer could have sat down with the person before a single line of code was written. This is a bigger waste of time. If every change requires asking someone in person, that's an infinite source of distraction and kills productivity for that person. MOST code reviews don't end up in comments asking for big changes, and when they do the one-time cost on the author is much better to pay than the constant tax on your subject matter experts of what you're proposing.
  - withinboredom 3y ago
    How is spending 5 minutes x TEAM_SIZE to go over what you're going to do more time than 1-2 days solving a problem, then another hour or two for the reviewer to write out why it's wrong, then another day or two rewriting everything from scratch?
    - Arainach 3y ago
      None of those timelines are realistic: * Most CLs don't take multiple days to write * Reasonably-sized CLs take 5-10 minutes to review - if I have things to say. Much less if the code looks good and I don't have any requests. * I cannot think of any CL in the last 5 years where I made a comment that caused someone to "rewrite everything from scratch". Most feedback can be done in a few clicks in an IDE which supports refactor or a few minutes of copy/paste.

> A PR is waaaaay too late to bring up No. That’s literally the point of a review.

by thiht 3y ago

> A PR is waaaaay too late to bring up No. That’s literally the point of a review.

No, it's not and there is a ton of literature (even entire books) written on why it isn't the time to bring this up during a review. For starters, it results in a waste of time for literally everyone involved: 1. The person writing has to rewrite it (probably). 2. The reviewer could have sat down with the person before a single line of code was written. 3. Anyone else reviewing just wasted their time because it will be rewritten. If you see code badly designed, walk over to, or call the developer and say "hey, can we discuss the design of the code." The saying "measure twice, cut once" doesn't just apply to wood. Design before you ever write any code.

DandyDev 3y ago
The alternative is a big design upfront?
- withinboredom 3y ago
  Generally, it goes something like this imaginary Slack convo: Me: I think we are getting events from SQS out of order and that’s why we are seeing some weird synchronization issues. What do you think about sending events to the regular queue and a FIFO queue at the same time and comparing them? Team: How would that work? Me: Since we are using a single threaded consumer here, I think we can simply override the transport class and instead of pulling from one queue per transport instance, we set up a feature flag allowing us to pull from two, compare the events and if they are different, alerting us in the logs. Team: Sounds good! You don’t need a full design spec, just agreement from the team on the approach. There won’t be any surprises in the review.
  - thiht 3y ago
    > There won’t be any surprises in the review. There can always be surprises. Sometimes when you see something implemented you can get a « click » as to why another solution would be better. It happens to me regularly both on the receiving and giving end of this and it’s rarely a big deal. Most of the time it happened to me, I could reuse the functional tests I had written and parts of the code anyway. Reworking on something so that it’s better is never a waste of time, and it’s always better to do it before it reaches Prod.
    - withinboredom 3y ago
      Making a suggestion on how to do something better is quite a bit different than saying “this isn’t up to standard. Do not pass go. Do not collect $200” We all see these suggestions. That’s ok, and wanted. But to say “ah, no I think it should be the other way.” [reject] That’s what I was talking about here.
johnnyanmac 3y ago
1. That's the point. It's not up to standard 2. They could but they usually don't. It's fine to sit down and make sure they understand some esoteric structure within a system I owj I'm not going to sit down with someone else to make sure they use for each loops unless it's a very new junior (i wouldn't block that in most code reviews anyway unless the code was legitimately painful to read, but every company is different). 3. IME there may be multiple reviewers but one primary reviewer. Half the time the non-primary ones either focus on "is this code going to break your work flow?" or are simply there for bookkeeping and isn't the approval you're looking for (i.e. A lead on a review that already talked about the plans but doesn't have the knowledge to review that specific module). Anymore than 1 primary reviewer probably needed a smaller PR if possible.
- withinboredom 3y ago
  Anyone who has ever come along in a code review and told me my code “isn’t up to standard” will inevitably start a very long game of attrition with me. Here’s the thing: there’s no such thing as “standard” software architecture (until someone writes a new kind of software architecture called “standard”). I’ve seen and used everything from MVC to DDD to TDD to MVVC to whatever React is to reactive to event-oriented to actors to whatever you can imagine. There simply isn’t a “standard” way to do anything in this industry. If you hired me, you hired me for these different perspectives on problem solving, and that goes for all of us. A code review simply isn’t the place to enforce your unique perspectives on someone else. Sometimes, there is simply a better way to do things. These might even be new ideas in the code base. If it looks like it is, hopefully the author will bring it up with the team beforehand. If the reviewer changes their mind in the code review… well, that’s not the authors problem. The reviewer should rewrite it and the author review it. Actually, that’s exactly what I suggest if someone ever tries that with me (usually when I’m the new guy). Please submit your own PR showing me how it’s done. I’ll move on to another ticket. Usually, about half way through their implementation, they’ll see why I did it the way I did it and approve my PR. It only takes one time before they start asking “why” instead of assuming they know everything; every line of code exists for a reason, after all. In one case, one specific dude (bless his heart) kept blocking PRs after we discussed everything before hand. I had to get HR involved, and other engineers. Dude just didn’t like being wrong and would throw tantrums when he was. He eventually got fired after people realized they could do what I was doing to stop the nits he said was a blocker. Nobody cared if a variable should be renamed from “SameFactory” to “EqualFactory”. Ah, but code quality! It’s so subjective. High code quality, to me, is easy to maintain code. Is the intention clear, do the comments reflect reality, is it easy to read, but more importantly, easy to change? Will changing a line in the module break 15 other sibling modules? God I hope not. Dependencies should be obvious. And, no, I’m not writing an interface if there is exactly one implementation. That’s ridiculous. For some people, they want layers. More layers of abstraction than a layered cake on a wedding day. To them, that’s good code quality! Some people think high code quality is beautiful code. The kind you frame and put over your fire place to admire with a glass of wine. There’s no objectively “high quality” code in existence.
Arainach 3y ago
>The reviewer could have sat down with the person before a single line of code was written. This is a bigger waste of time. If every change requires asking someone in person, that's an infinite source of distraction and kills productivity for that person. MOST code reviews don't end up in comments asking for big changes, and when they do the one-time cost on the author is much better to pay than the constant tax on your subject matter experts of what you're proposing.
- withinboredom 3y ago
  How is spending 5 minutes x TEAM_SIZE to go over what you're going to do more time than 1-2 days solving a problem, then another hour or two for the reviewer to write out why it's wrong, then another day or two rewriting everything from scratch?
  - Arainach 3y ago
    None of those timelines are realistic: * Most CLs don't take multiple days to write * Reasonably-sized CLs take 5-10 minutes to review - if I have things to say. Much less if the code looks good and I don't have any requests. * I cannot think of any CL in the last 5 years where I made a comment that caused someone to "rewrite everything from scratch". Most feedback can be done in a few clicks in an IDE which supports refactor or a few minutes of copy/paste.