5 ms·

Yeah no, part of code review is ensuring code quality is maintained. If you start letting badly written code fly because it passes tests, that’ll bite you. If s

Yeah no, part of code review is ensuring code quality is maintained. If you start letting badly written code fly because it passes tests, that’ll bite you. If something isn’t blocking I’ll make a note that it should be fixed if theres budget knowing sometimes you gotta let things go. But you can’t just ignore code quality and call it nitpicking.

withinboredom 3y ago
Badly written, or badly designed? Bad code, to me, would be an unrolled for-loop, or a for-loop where a foreach loop would do better (though now, we're getting in the realm of nit-picking). A PR is waaaaay too late to bring up architectural/design improvements (unless it is a WIP PR opened expressly for discussing the approach). I very rarely see bad code in PR's unless it is from a Junior programmer, and even then, it's usually because they don't know better. Even then, I choose to trust them to do the right thing. In most environments, you can even review the code AFTER it has been merged. So, if they don't address the issue, I can still review the code and let them know that I expect to see a follow-up PR addressing it.
- thiht 3y ago
  > A PR is waaaaay too late to bring up No. That’s literally the point of a review.
  - 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
- ravenstine 3y ago
  Kind of off topic but I'm curious what it is about a foreach that you think would do a better job than a syntactical for-loop. In my experience, for-loops are almost always the better choice, unless the implementation of "foreach" is done in a way that is functional and recursive. Foreach in the usual sense (like the method in JavaScript) provides too little control to the callee and is only really of benefit if one really needs to conditionally pass in different callback functions. Besides, that, I mostly agree with your stance on code reviews. The way that most programmers do code review is so very "waterfall" and they don't even realize it.
  - withinboredom 3y ago
    > what it is about a foreach that you think would do a better job than a syntactical for-loop It depends on the language. For example, an optimizing compiler can optimize the shit out of a for-each loop, but can only go so far in a for-loop. For example, in a for-loop, you can "skip around" the indices, and even go backward after going forward, while in a for-each loop, it will always go one-by-one, and the compiler can rely on that fact. Even the CPU can probably optimize it better since the data access will be more predictable.
    - ruds 3y ago
      Not only that, but a code reader can rely on this too. If you only use a raw for loop when you're skipping around, then the reviewer / future reader can slow down to understand what's happening here. If you use raw for loops for the for-each case, now the reader has to slow down on all your for loops. It's similar to the reasoning for "const everywhere": if you don't mark variables const, I need to figure out as a reader whether the variable is modified later, and that slows me down and increases the mental load of reading a function.
ravenstine 3y ago
Code "quality" is almost always an opinion, and not one that is based on anything objective. That doesn't mean that the idea of it doesn't have a purpose, but it's almost always used as a bat to knock someone over the head with. It's not like code can be distinguished as fresh or moldy and rotten like fruit. If that were so, then most programmers would hardly ever bicker about code quality; it would be self-evident, just as any moron can separate the good fruit from the bad. If a unit of code fixes or improves the user experience without slowing it down and is memory efficient, then it becomes incredibly hard to objectively define said code as being bad. Perhaps it ruins the developer experience, but then again, many programmers object to things they simply don't like on a philosophical or theological basis. Programmers, depending on the language community, may consider anything that isn't "object oriented" to be poor quality, but good luck finding objective proof that object orientation is always better.

withinboredom 3y ago

Badly written, or badly designed? Bad code, to me, would be an unrolled for-loop, or a for-loop where a foreach loop would do better (though now, we're getting in the realm of nit-picking). A PR is waaaaay too late to bring up architectural/design improvements (unless it is a WIP PR opened expressly for discussing the approach). I very rarely see bad code in PR's unless it is from a Junior programmer, and even then, it's usually because they don't know better. Even then, I choose to trust them to do the right thing. In most environments, you can even review the code AFTER it has been merged. So, if they don't address the issue, I can still review the code and let them know that I expect to see a follow-up PR addressing it.

thiht 3y ago
> A PR is waaaaay too late to bring up No. That’s literally the point of a review.
- 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
ravenstine 3y ago
Kind of off topic but I'm curious what it is about a foreach that you think would do a better job than a syntactical for-loop. In my experience, for-loops are almost always the better choice, unless the implementation of "foreach" is done in a way that is functional and recursive. Foreach in the usual sense (like the method in JavaScript) provides too little control to the callee and is only really of benefit if one really needs to conditionally pass in different callback functions. Besides, that, I mostly agree with your stance on code reviews. The way that most programmers do code review is so very "waterfall" and they don't even realize it.
- withinboredom 3y ago
  > what it is about a foreach that you think would do a better job than a syntactical for-loop It depends on the language. For example, an optimizing compiler can optimize the shit out of a for-each loop, but can only go so far in a for-loop. For example, in a for-loop, you can "skip around" the indices, and even go backward after going forward, while in a for-each loop, it will always go one-by-one, and the compiler can rely on that fact. Even the CPU can probably optimize it better since the data access will be more predictable.
  - ruds 3y ago
    Not only that, but a code reader can rely on this too. If you only use a raw for loop when you're skipping around, then the reviewer / future reader can slow down to understand what's happening here. If you use raw for loops for the for-each case, now the reader has to slow down on all your for loops. It's similar to the reasoning for "const everywhere": if you don't mark variables const, I need to figure out as a reader whether the variable is modified later, and that slows me down and increases the mental load of reading a function.

ravenstine 3y ago

Code "quality" is almost always an opinion, and not one that is based on anything objective. That doesn't mean that the idea of it doesn't have a purpose, but it's almost always used as a bat to knock someone over the head with. It's not like code can be distinguished as fresh or moldy and rotten like fruit. If that were so, then most programmers would hardly ever bicker about code quality; it would be self-evident, just as any moron can separate the good fruit from the bad. If a unit of code fixes or improves the user experience without slowing it down and is memory efficient, then it becomes incredibly hard to objectively define said code as being bad. Perhaps it ruins the developer experience, but then again, many programmers object to things they simply don't like on a philosophical or theological basis. Programmers, depending on the language community, may consider anything that isn't "object oriented" to be poor quality, but good luck finding objective proof that object orientation is always better.