4 ms·

The author's pull requests sound exhausting! Opening a PRs to accommodate a "pet peeve" is a mark of inexperience. Does it solve a problem or fix a bug? Does

The author's pull requests sound exhausting! Opening a PRs to accommodate a "pet peeve" is a mark of inexperience. Does it solve a problem or fix a bug? Doesn't look like it. It doesn't even obviously improve performance. I just see a gratuitously new abstraction that will decrease readability for the rest of the team. As a team you're going to drown in C++ if you let everyone add their favourite coding conventions, standards or libraries to the code - it's the worst language for that sort of sprawl.

kyralis 1y ago
Yes, this is the sort of thing that I'd reject. Why are we churning code (assuming it's working) to potentially introduce stylistic inconsistencies without some existing motivation, especially as a new person on the team who's apparently attempting to have a code style conversation via PR? It's not even that I disagree with his premise, in the abstract, and if he were including this as part of a larger change in that area of code, I could see it being reasonable. But as a "I don't like the way this code looks so I'm going to rewrite it" PR, no.
- bluGill 1y ago
  Motivation: new people can read the code ane figure out what the loop is doing. I've seen too many loops that seemed to be some standard cs101 algorthym but something was subtilly differet and so it took a long time to figure it out. I was sometimes the person who wrote the original - 10+ years ago. plus I'm now getting old enough to realize early retirement might be possible. That means I need to make sure new people understand my code so I'm not called back inside from my (whatever my next life is) just rewriting it because isn't good. However rewriting it in a more expressive way is better.
  - foota 1y ago
    In isolation, sure writing code more expressively isn't a bad thing, and might be fine as an exercise for becoming more familiar with some part of the codebase, but I don't think it's a good use of time for the author or reviewers to go and do this without some other motivation or if you're touching the code already.
    - bluGill 1y ago
      Getting someone new is itself motivation. And if the result is better code thateis a good use of time. often manual loops have unnoticed off by one errors as well so the above exercise can fix weird bugs nobody has figured out.
  - john_the_writer 1y ago
    I had a boss call me a tumbleweed dev (when I was much younger). I would clean up bits, but this would mean the QA had to re-test code that was already tested and in production. I never forgot that term, and now if it an't broke, I don't touch it. (even if I don't like reading it)
    - bluGill 1y ago
      that is both right and wrong. Right that they need to QA it, which is a cost. Depending on current ecconomics one they don't want to pay today. Wrong though because eventually they will need to pay it - when someone needs to spend the time to figure out what the weird code is doing (CS101 algorithms are not always obvious when not given a name even though simple once you realize what is happening). Not cleaning things up as you go is why many projects have declared bankruptcy and done a billion dollar rewrite. We learn things over time, but if you don't apply the lessons you end up with really bad code. OTOH, just changing everything all the time isn't right either. Find the correct balance so your code is constantly improving and as a result maintanable for centuries.
- john_the_writer 1y ago
  I had a job interview where I said almost exactly this.. I was shown a bit of code and asked how I'd improve it. I said I wouldn't touch it. When asked why not I answered. "I'd assume the dev who wrote it did so for a reason, and that it's been through QA. If they picked this approach I'd leave it alone. Unless there was a bug." I got the job. My personal style has to take a back seat to the style of the team (even if I'm the team lead) It is better to get used to not using a turnery, if that's how the team works. Changing a standard on an older code base is silly because then you'll have a different standard in different parts of the code.
bluGill 1y ago
It isnft clear what they are but if he replaces a loop with an algorithm (that implements the loop) it makes it easier to understand the code because the name of the algorithm is clearer than the loop. Normally the speed is the same.
juped 1y ago
I let people do this (and mildly encourage them to) all the time, if their pet peeve makes sense to enough of the (small) team. Often, hard-won experience manifests as getting annoyed by trivial-seeming things! I think I agree that, in projects dominated by C++ or similar languages, I would be less positive about it, just because of how complicated they can get.

kyralis 1y ago

Yes, this is the sort of thing that I'd reject. Why are we churning code (assuming it's working) to potentially introduce stylistic inconsistencies without some existing motivation, especially as a new person on the team who's apparently attempting to have a code style conversation via PR? It's not even that I disagree with his premise, in the abstract, and if he were including this as part of a larger change in that area of code, I could see it being reasonable. But as a "I don't like the way this code looks so I'm going to rewrite it" PR, no.

bluGill 1y ago
Motivation: new people can read the code ane figure out what the loop is doing. I've seen too many loops that seemed to be some standard cs101 algorthym but something was subtilly differet and so it took a long time to figure it out. I was sometimes the person who wrote the original - 10+ years ago. plus I'm now getting old enough to realize early retirement might be possible. That means I need to make sure new people understand my code so I'm not called back inside from my (whatever my next life is) just rewriting it because isn't good. However rewriting it in a more expressive way is better.
- foota 1y ago
  In isolation, sure writing code more expressively isn't a bad thing, and might be fine as an exercise for becoming more familiar with some part of the codebase, but I don't think it's a good use of time for the author or reviewers to go and do this without some other motivation or if you're touching the code already.
  - bluGill 1y ago
    Getting someone new is itself motivation. And if the result is better code thateis a good use of time. often manual loops have unnoticed off by one errors as well so the above exercise can fix weird bugs nobody has figured out.
- john_the_writer 1y ago
  I had a boss call me a tumbleweed dev (when I was much younger). I would clean up bits, but this would mean the QA had to re-test code that was already tested and in production. I never forgot that term, and now if it an't broke, I don't touch it. (even if I don't like reading it)
  - bluGill 1y ago
    that is both right and wrong. Right that they need to QA it, which is a cost. Depending on current ecconomics one they don't want to pay today. Wrong though because eventually they will need to pay it - when someone needs to spend the time to figure out what the weird code is doing (CS101 algorithms are not always obvious when not given a name even though simple once you realize what is happening). Not cleaning things up as you go is why many projects have declared bankruptcy and done a billion dollar rewrite. We learn things over time, but if you don't apply the lessons you end up with really bad code. OTOH, just changing everything all the time isn't right either. Find the correct balance so your code is constantly improving and as a result maintanable for centuries.
john_the_writer 1y ago
I had a job interview where I said almost exactly this.. I was shown a bit of code and asked how I'd improve it. I said I wouldn't touch it. When asked why not I answered. "I'd assume the dev who wrote it did so for a reason, and that it's been through QA. If they picked this approach I'd leave it alone. Unless there was a bug." I got the job. My personal style has to take a back seat to the style of the team (even if I'm the team lead) It is better to get used to not using a turnery, if that's how the team works. Changing a standard on an older code base is silly because then you'll have a different standard in different parts of the code.

bluGill 1y ago

It isnft clear what they are but if he replaces a loop with an algorithm (that implements the loop) it makes it easier to understand the code because the name of the algorithm is clearer than the loop. Normally the speed is the same.

juped 1y ago

I let people do this (and mildly encourage them to) all the time, if their pet peeve makes sense to enough of the (small) team. Often, hard-won experience manifests as getting annoyed by trivial-seeming things! I think I agree that, in projects dominated by C++ or similar languages, I would be less positive about it, just because of how complicated they can get.