5 ms·

There is a major exception to the personal tastes issue, consistency and predictably in a mature codebase. Things that would be personal taste in a greenfield

There is a major exception to the personal tastes issue, consistency and predictably in a mature codebase. Things that would be personal taste in a greenfield project become important for maintenance in a mature product.

watwut 1y ago
Then make that explicit topic during the setup instead of taking random commits hostage to whatever catches your attention this week.
- rileymat2 1y ago
  It is hard to fully specify all the different dimensions of consistency. Look at the code that exists and write in that voice. Maintenance will be much better.
  - watwut 1y ago
    It is even harder to chase personal consistency feelings of multiple different colleagues. Usually inconsistent with each other. Usually enforced only against less assertive committers and not enforced against those perceived to be strong. That is why you should either make it a rule or let it be.
  - hinkley 1y ago
    You should absolutely enforce any patterns that showed up in RCA findings and the results of any R&D that has found better patterns.
AndyNemmity 1y ago
If there was one personal taste, this is achievable. There is not. There are a variety of personal tastes and they all conflict with each other.
- treis 1y ago
  Which is my fundamental problem with peer review. There should be one gatekeeper to a code base that enforces some style preference. Practically which style they choose matters very little compared to having one predictable and enforced style to follow.
  - rileymat2 1y ago
    To clarify, I am talking about more than surface level style. A small example adding an item to a list, it might be preference whether you use “push” or “append” Don’t care which, but it is maddening to have a unit of code that mixes them. There are all these choices that are numerous that should be consistent, but too numerous to add to a standards document ahead of time.
- hinkley 1y ago
  None of us mature in the same directions at the same speed. What is taken by one as taste may be for another a hard won lesson in neglect or negligence.
  - dietr1ch 1y ago
    I kept explaining the reasoning behind my nits and built my own DB of issues, explaining why it was a problem and what to do to reference as I started to explain my points over and over to different colleagues. I feel that just giving out what to change without context does little to teach the hard won lessons, and is everything that would annoy people who are trying to get a change in.
kazinator 1y ago
Consistency and predictability are encoded in conventions which are someone's personal taste. You don't get to have your own personal taste of the moment when you are reviewing; you have to broadly refer to the documented collection of tastes.
globular-toast 1y ago
Consistency and predictability could also be considered a sign of maturity. Maturity is something to be observed, not declared. It also depends where you draw the line. For example, is dependency injection a matter of taste or a fundamental design decision for a project? What about "defensive" coding practices, like `0 == x` over `x == 0` in C?
- rowanG077 1y ago
  What does 0 == x vs x == 0 have to do with defensive coding practices? I have always understood defensive coding practices to mean always check your pre-conditions.
  - rileymat2 1y ago
    In C it is a "defense" against inadvertent x=0 being assignment. The definitions are cloudy enough that you could consider it defensive programming or not.
    - jevndev 1y ago
      > the definitions are cloudy enough […] This is one of the biggest traps I’ve seen in code review. Generally, everyone is coming from a good place of “I’m reviewing this code to maintain codebase quality. This technically could cause problems. Thus I’m obligated to mention it”. Since the line of “could cause problems (important enough to mention)” is subjective, you can (and will, in my experience) get good natured pedants. They’ll block a 100LOC patch for weeks because “well if we name this variable x that COULD cause someone to think of it like y so we can’t name it x” or “this pattern you used has <insert textbook downsides that generally aren’t relevant for the problem>. I would do it with this other pattern (which has its own downsides but i wont say them)”.
  - nostrademons 1y ago
    In C "x = 0" is a legal expression, so you could have if(x = 0) { ... } and it would assign 0 to x and then treat it as false. The compiler won't complain (unless you have warnings turned on, which you should). But if you do "if (0 = x)", you're assigning to a constant, which will result in a compile error. So "if (0 == x)" guards against a typo where you omit the second =. The GMail outage of 2010, where Google had to restore 10% of GMail accounts from tape backup, was precisely because of this. A C++ migration script had used the "x = 0" construct, which set the variable to false and made the script go haywire. So not a theoretical concern. The other major takeaway from this was to always treat your migration scripts like production code and use the same warnings, tests, and defensive programming that you would use everywhere.
hinkley 1y ago
The sort of clean that a functioning restaurant is, is something many people don't achieve at home. I wonder sometimes if these arguments in software are less of an issue of lack of care for craft, and more just a fundamental misunderstanding that the sorts of arguments and compromises one makes with a roommate over what state the apartment is allowed to be in are simply not analogs for a professional setting. I get the very distinct feeling sometimes that my mis en place rules are being received as if I'm whining about whether coasters get put away when not in use, and how long old magazines get left on the couch. Nearly all of the fastidiousness I can manage in a day I save for work, so I'm very familiar with the other side of this conversation outside of the office.

watwut 1y ago

Then make that explicit topic during the setup instead of taking random commits hostage to whatever catches your attention this week.

rileymat2 1y ago
It is hard to fully specify all the different dimensions of consistency. Look at the code that exists and write in that voice. Maintenance will be much better.
- watwut 1y ago
  It is even harder to chase personal consistency feelings of multiple different colleagues. Usually inconsistent with each other. Usually enforced only against less assertive committers and not enforced against those perceived to be strong. That is why you should either make it a rule or let it be.
- hinkley 1y ago
  You should absolutely enforce any patterns that showed up in RCA findings and the results of any R&D that has found better patterns.

AndyNemmity 1y ago

If there was one personal taste, this is achievable. There is not. There are a variety of personal tastes and they all conflict with each other.

treis 1y ago
Which is my fundamental problem with peer review. There should be one gatekeeper to a code base that enforces some style preference. Practically which style they choose matters very little compared to having one predictable and enforced style to follow.
- rileymat2 1y ago
  To clarify, I am talking about more than surface level style. A small example adding an item to a list, it might be preference whether you use “push” or “append” Don’t care which, but it is maddening to have a unit of code that mixes them. There are all these choices that are numerous that should be consistent, but too numerous to add to a standards document ahead of time.
hinkley 1y ago
None of us mature in the same directions at the same speed. What is taken by one as taste may be for another a hard won lesson in neglect or negligence.
- dietr1ch 1y ago
  I kept explaining the reasoning behind my nits and built my own DB of issues, explaining why it was a problem and what to do to reference as I started to explain my points over and over to different colleagues. I feel that just giving out what to change without context does little to teach the hard won lessons, and is everything that would annoy people who are trying to get a change in.

kazinator 1y ago

Consistency and predictability are encoded in conventions which are someone's personal taste. You don't get to have your own personal taste of the moment when you are reviewing; you have to broadly refer to the documented collection of tastes.

globular-toast 1y ago

Consistency and predictability could also be considered a sign of maturity. Maturity is something to be observed, not declared. It also depends where you draw the line. For example, is dependency injection a matter of taste or a fundamental design decision for a project? What about "defensive" coding practices, like `0 == x` over `x == 0` in C?

rowanG077 1y ago
What does 0 == x vs x == 0 have to do with defensive coding practices? I have always understood defensive coding practices to mean always check your pre-conditions.
- rileymat2 1y ago
  In C it is a "defense" against inadvertent x=0 being assignment. The definitions are cloudy enough that you could consider it defensive programming or not.
  - jevndev 1y ago
    > the definitions are cloudy enough […] This is one of the biggest traps I’ve seen in code review. Generally, everyone is coming from a good place of “I’m reviewing this code to maintain codebase quality. This technically could cause problems. Thus I’m obligated to mention it”. Since the line of “could cause problems (important enough to mention)” is subjective, you can (and will, in my experience) get good natured pedants. They’ll block a 100LOC patch for weeks because “well if we name this variable x that COULD cause someone to think of it like y so we can’t name it x” or “this pattern you used has <insert textbook downsides that generally aren’t relevant for the problem>. I would do it with this other pattern (which has its own downsides but i wont say them)”.
- nostrademons 1y ago
  In C "x = 0" is a legal expression, so you could have if(x = 0) { ... } and it would assign 0 to x and then treat it as false. The compiler won't complain (unless you have warnings turned on, which you should). But if you do "if (0 = x)", you're assigning to a constant, which will result in a compile error. So "if (0 == x)" guards against a typo where you omit the second =. The GMail outage of 2010, where Google had to restore 10% of GMail accounts from tape backup, was precisely because of this. A C++ migration script had used the "x = 0" construct, which set the variable to false and made the script go haywire. So not a theoretical concern. The other major takeaway from this was to always treat your migration scripts like production code and use the same warnings, tests, and defensive programming that you would use everywhere.

hinkley 1y ago

The sort of clean that a functioning restaurant is, is something many people don't achieve at home. I wonder sometimes if these arguments in software are less of an issue of lack of care for craft, and more just a fundamental misunderstanding that the sorts of arguments and compromises one makes with a roommate over what state the apartment is allowed to be in are simply not analogs for a professional setting. I get the very distinct feeling sometimes that my mis en place rules are being received as if I'm whining about whether coasters get put away when not in use, and how long old magazines get left on the couch. Nearly all of the fastidiousness I can manage in a day I save for work, so I'm very familiar with the other side of this conversation outside of the office.