4 ms·

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 exampl

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.

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.