4 ms·

All the "code quality" people write shitty systems. You are too zoomed in if you think code quality is really important. It's kind of important, but your system

by vp8989 6y ago

All the "code quality" people write shitty systems. You are too zoomed in if you think code quality is really important. It's kind of important, but your system likely has much more important things wrong with it than the "code quality". Things you could actually get fired for, or seriously reprimanded if the winds don't blow in your favor. Almost all "code quality" discussion in PRs is lightweight value judgements with no rigor or consistency. Just what random bikeshedding objection that person happened to think of, maybe they had a bad night's sleep? Who knows... Remnants of an era where most software was low stakes and had low number of users. We now have many more "-ilities" we are judged on, that are measurable and that actually matter, so ignore the code quality trolls stuck in 2005.

rowanG077 6y ago
It seems to me you are confusing code quality with style. Code quality is most certainly important. Style less so even though it should be consistent. Almost all bike shedding comments are style.
gregmac 6y ago
Most of the code quality comments I make (and recieve) on PRs are about naming and comments, and they're mostly for future changes and maintenance. Good example from a couple days ago: someone made a helper method for compressing something, with the signature string Compress(string value) It was actually doing gzip followed by base64. My comment was to rename it to something like Base64Compress as well as change the documentation comment to state it was doing both. I wouldn't want someone else to inadvertently call that in the future if they stumbled across it, not realizing it was returning base64 and either double-base64 encode it, unnecessarily have base64 when not needed, or even end up with a bigger string than input because of the 33% increase. In a future PR, misuse of that Compress() method would be impossible to spot unless you happened to remember what it was actually doing. Other times it'll be something like a constant 30s timeout hard-coded, and I like that to have comments like: // This usually takes <1s, so 30s timeout for this is generous or // 30s is max or (calling code) times out anyway Why? Because if we need to change that, the comment helps let us know it's arbitrary and safe to modify or not. Without a comment (or good non-squashed commit message) someone in the future (maybr you) is doomed to waste time rediscovering a bug you already know about or researching if it's safe to change.
- sangnoir 6y ago
  People who don't care about code quality write brittle, hard-to-maintain systems - which is occasionally fine, depending on expected longevity of the system. If you're interviewing for a job that involves any sort of maintenance over a long-lived project, organizational attitudes towards code quality are a useful smell test to how miserable it might get. I bear the scars of a couple of wholesale refactors (that cowboy coders wanted no part in, to no ones surprise) to improve readability/maintainability. Merging those gigantic PRs and having everything work was satisfying; it almost made up for the hair pulling. Also satisfying was catching a surprising number of hidden bugs caused by sloppy programming (some logical errors are obvious when you give functions, arguments and variables good names)
  - gregmac 6y ago
    > Also satisfying was catching a surprising number of hidden bugs caused by sloppy programming (some logical errors are obvious when you give functions, arguments and variables good names) I agree, and I've found the same adding tests to existing code. It often uncovers bugs that have been there for years, and there's a certain schadenfreude from doing that. Sometimes it turns out that the bug doesn't really occur only because that function never happens to be called in a certain way. I've also run into cases where when I'm trying to figure out how to reproduce the bug for real that I uncover a another bug that is blocking it. Both of these are examples of brittle code: the right change or separate bug fix is going to expose this, and hope your QA is good enough to find it. I've also been able to identify and fix a couple long-standing bugs this way: the kind where it's been observed in production a handful of times and in more than one deployed environment, but no one had ever figured out reliable reproduction steps. It's a great moment when suddenly you realize all this crazy behavior can be fully explained.
    - sangnoir 6y ago
      > I've also run into cases where when I'm trying to figure out how to reproduce the bug for real that I uncover a another bug that is blocking it. During a JS project refactor, I ran into a couple of those "This should be broken, why is it working?" headscratchers. In my case, 4 out of 5 times was that there was an earlier "quick-fix" further up the call-chain that papered over the bug for certain use-cases, but did not generalize well; I feel that that sort of bug is harder to catch using (unit) tests.
mcv 6y ago
Code quality is absolutely important, although there seems to be a lot of variation in what people really mean when they talk about code quality. To me it's proper design, readable, maintainable code, that's easy to test and easy to change. To others it means the actual unit testing itself. So some it means the style you tend to enforce through linting rules. Style is in my opinion very arbitrary. It's good to be consistent, and for that reason I guess it's good to have rules that are enforced, but with the number of times I have to revisit my changes because apparently I'm violating some obscure style rule, and therefore the build server rejects my change (or sometimes it even rejects it because someone else's change violated something, which makes me wonder how that ever got through), it feels like it's a waste of time to be too strict about it. A bit of personal style isn't going to kill anyone.
tdeck 6y ago
I disagree completely. The code isn't just for the computer - a human being will need to understand and maintain it in the future, and often that human being won't be the author. Having a second person look at the code and say "I don't understand what's going on here, please make it clearer" or "this violates some convention we use for a reason" is a worthwhile exercise. Sure it'll be inconsistent, and it'll lead to some changes that are idiosyncratic to the reviewer, while leaving other issues in place. But that's true of copy-editing as well, and I've never heard someone argue that it's not worth getting editorial feedback because that feedback might depend on the person. Perhaps you had some experience with someone who cared about "code quality" in a way that rubbed you the wrong way, and they also designed a shitty system. Or perhaps your idea of what code quality means just differs from mine. I've spent enough time reading and fixing up left-behind cowboy code to spare a thought for the next engineer who'll have to come along and touch that system.

by vp8989 6y ago

rowanG077 6y ago

It seems to me you are confusing code quality with style. Code quality is most certainly important. Style less so even though it should be consistent. Almost all bike shedding comments are style.

gregmac 6y ago

Most of the code quality comments I make (and recieve) on PRs are about naming and comments, and they're mostly for future changes and maintenance. Good example from a couple days ago: someone made a helper method for compressing something, with the signature string Compress(string value) It was actually doing gzip followed by base64. My comment was to rename it to something like Base64Compress as well as change the documentation comment to state it was doing both. I wouldn't want someone else to inadvertently call that in the future if they stumbled across it, not realizing it was returning base64 and either double-base64 encode it, unnecessarily have base64 when not needed, or even end up with a bigger string than input because of the 33% increase. In a future PR, misuse of that Compress() method would be impossible to spot unless you happened to remember what it was actually doing. Other times it'll be something like a constant 30s timeout hard-coded, and I like that to have comments like: // This usually takes <1s, so 30s timeout for this is generous or // 30s is max or (calling code) times out anyway Why? Because if we need to change that, the comment helps let us know it's arbitrary and safe to modify or not. Without a comment (or good non-squashed commit message) someone in the future (maybr you) is doomed to waste time rediscovering a bug you already know about or researching if it's safe to change.

sangnoir 6y ago
People who don't care about code quality write brittle, hard-to-maintain systems - which is occasionally fine, depending on expected longevity of the system. If you're interviewing for a job that involves any sort of maintenance over a long-lived project, organizational attitudes towards code quality are a useful smell test to how miserable it might get. I bear the scars of a couple of wholesale refactors (that cowboy coders wanted no part in, to no ones surprise) to improve readability/maintainability. Merging those gigantic PRs and having everything work was satisfying; it almost made up for the hair pulling. Also satisfying was catching a surprising number of hidden bugs caused by sloppy programming (some logical errors are obvious when you give functions, arguments and variables good names)
- gregmac 6y ago
  > Also satisfying was catching a surprising number of hidden bugs caused by sloppy programming (some logical errors are obvious when you give functions, arguments and variables good names) I agree, and I've found the same adding tests to existing code. It often uncovers bugs that have been there for years, and there's a certain schadenfreude from doing that. Sometimes it turns out that the bug doesn't really occur only because that function never happens to be called in a certain way. I've also run into cases where when I'm trying to figure out how to reproduce the bug for real that I uncover a another bug that is blocking it. Both of these are examples of brittle code: the right change or separate bug fix is going to expose this, and hope your QA is good enough to find it. I've also been able to identify and fix a couple long-standing bugs this way: the kind where it's been observed in production a handful of times and in more than one deployed environment, but no one had ever figured out reliable reproduction steps. It's a great moment when suddenly you realize all this crazy behavior can be fully explained.
  - sangnoir 6y ago
    > I've also run into cases where when I'm trying to figure out how to reproduce the bug for real that I uncover a another bug that is blocking it. During a JS project refactor, I ran into a couple of those "This should be broken, why is it working?" headscratchers. In my case, 4 out of 5 times was that there was an earlier "quick-fix" further up the call-chain that papered over the bug for certain use-cases, but did not generalize well; I feel that that sort of bug is harder to catch using (unit) tests.

mcv 6y ago

Code quality is absolutely important, although there seems to be a lot of variation in what people really mean when they talk about code quality. To me it's proper design, readable, maintainable code, that's easy to test and easy to change. To others it means the actual unit testing itself. So some it means the style you tend to enforce through linting rules. Style is in my opinion very arbitrary. It's good to be consistent, and for that reason I guess it's good to have rules that are enforced, but with the number of times I have to revisit my changes because apparently I'm violating some obscure style rule, and therefore the build server rejects my change (or sometimes it even rejects it because someone else's change violated something, which makes me wonder how that ever got through), it feels like it's a waste of time to be too strict about it. A bit of personal style isn't going to kill anyone.

tdeck 6y ago

I disagree completely. The code isn't just for the computer - a human being will need to understand and maintain it in the future, and often that human being won't be the author. Having a second person look at the code and say "I don't understand what's going on here, please make it clearer" or "this violates some convention we use for a reason" is a worthwhile exercise. Sure it'll be inconsistent, and it'll lead to some changes that are idiosyncratic to the reviewer, while leaving other issues in place. But that's true of copy-editing as well, and I've never heard someone argue that it's not worth getting editorial feedback because that feedback might depend on the person. Perhaps you had some experience with someone who cared about "code quality" in a way that rubbed you the wrong way, and they also designed a shitty system. Or perhaps your idea of what code quality means just differs from mine. I've spent enough time reading and fixing up left-behind cowboy code to spare a thought for the next engineer who'll have to come along and touch that system.