4 ms·

In code review I absolutely hate armhair architects and backseat programmers who nitpick and theorize over every little architectural decision you made, every v

In code review I absolutely hate armhair architects and backseat programmers who nitpick and theorize over every little architectural decision you made, every variable name, every loop, every comment (even complaining something is /too well commented/). They weren't the one to dive into the problem and consider it and fix it but they think their knee jerk reactions are always more valid than the remote possibility you know what you're doing as the person in the trenches. I'm going to paste a monologue I gave recently: sometimes as people who care about our craft, we need to step back and keep the big picture in mind. the goal is to make a functioning website, or video game, or text editor. the user couldn't care less about your code formatting, composition vs inheritance decisions, or commit messages. these are useful tools to the extent that they make it easier for yourself or others in the future to provide more value to the user. at my job, people (mostly external contractors) might make pull requests with formatting annoyances, pointless null checks, getters and setters that have no reason to exist, useless comments, "== true", things of that nature. I'd ask these to be fixed and sometimes they would be, but other times someone else would just come in and approve the PR themselves. I realized one day that not a single one of those ugly PR's that made it through have ever come back to bite us in any way. everyone's time was better spent continuing to work on providing value to the user instead. I hold myself to a higher standard, but when reading others' code, I've found it really means nothing to me where they put the brackets or what naming convention they use, or even if they change these up randomly. I care that it does the thing, has no glaring red flags, looks appropriately performant for the problem at hand, and won't be a maintenance nightmare (will spending 30 minutes changing something right now save at least 1 hour dealing with it later? will those code even be around by the time we get to that point, or replaced by something else? in many cases probably not, of course consider how foundational or isolated the code is.) in all my experience, any bikeshedding over aesthetics has proven to be a total waste of time. if we're making a thing that lets the people designing a website make a thing for the visitors, I care that the experience is good for the designer and the visitor, it does what it should, etc. and it's reasonably maintainable, but I'm not going to drag someone into the bikeshed and beat them with the tire pump of subjective aesthetics until they rename their variables to my whimsies.

tkiolp4 3y ago
It’s all about taste. Some engineers have it, some do not. And yes, taste is subjective, but the Mona Lisa is the Mona Lisa regardless of how many people don’t like it. The most important thing about code is probably its correctness… but that doesn’t mean other less important things are not worth looking at (whether to do so when reviewing a PR is a different topic, though). It also depends on whether one feels too attached to the code in question. If I just landed in a codebase and someone asks me to review a PR, well I guess I don’t mind too much (yet) about things besides correctness/performance/etc. But if I have been maintaining a codebase for years and suddenly some newcomer opens a PR with correct code but that doesn’t have “nice” variable names (at least in accordance to the rest of the codebase), well, I do care about that.
- jimmaswell 3y ago
  I say leave taste to the artists. Our job is to herd electrons through sand such that it does something useful for someone else, not produce art. We would all be better off focusing on the machine's point of view of the code instead of misapplying our mate selection circuits it.
butterNaN 3y ago
I have grown to learn to choose my battles when the PR is in a corporate setting. There's a lot of incentive for performative optics there, which makes people to prioritise "looking good" than "delivering good software". If the reviewer wants me to add an unnecessary `try... catch`, I simply do that. No point arguing with someone's ego, which takes way more time. If the reviewer is asking me to code for the eventuality that Google will suddenly change their the JSON model in their API response? You got it bud. You want to preserve some convention without explaining how it applies here? I will squish my code to it, keeping it correct. I know it makes my code less readable, more complex. But I care about delivering the value as early as possible. Also the code is not my baby, it's the company's baby. I try to fight comlpexity in the design phase, while also ensuring the ego folks get to have their optics.
andelink 3y ago
> pointless null checks, getters and setters that have no reason to exist, useless comments, "== true", things of that nature Aside from useless comments, if someone opens a PR with any of those things, I can only assume it’s because they don’t know why they aren’t necessary. As their colleague, I feel compelled to tell them, because personally I cringe at the thought of doing things that aren’t needed and nobody having the courtesy to tell me otherwise.

tkiolp4 3y ago

It’s all about taste. Some engineers have it, some do not. And yes, taste is subjective, but the Mona Lisa is the Mona Lisa regardless of how many people don’t like it. The most important thing about code is probably its correctness… but that doesn’t mean other less important things are not worth looking at (whether to do so when reviewing a PR is a different topic, though). It also depends on whether one feels too attached to the code in question. If I just landed in a codebase and someone asks me to review a PR, well I guess I don’t mind too much (yet) about things besides correctness/performance/etc. But if I have been maintaining a codebase for years and suddenly some newcomer opens a PR with correct code but that doesn’t have “nice” variable names (at least in accordance to the rest of the codebase), well, I do care about that.

jimmaswell 3y ago
I say leave taste to the artists. Our job is to herd electrons through sand such that it does something useful for someone else, not produce art. We would all be better off focusing on the machine's point of view of the code instead of misapplying our mate selection circuits it.

butterNaN 3y ago

I have grown to learn to choose my battles when the PR is in a corporate setting. There's a lot of incentive for performative optics there, which makes people to prioritise "looking good" than "delivering good software". If the reviewer wants me to add an unnecessary `try... catch`, I simply do that. No point arguing with someone's ego, which takes way more time. If the reviewer is asking me to code for the eventuality that Google will suddenly change their the JSON model in their API response? You got it bud. You want to preserve some convention without explaining how it applies here? I will squish my code to it, keeping it correct. I know it makes my code less readable, more complex. But I care about delivering the value as early as possible. Also the code is not my baby, it's the company's baby. I try to fight comlpexity in the design phase, while also ensuring the ego folks get to have their optics.

andelink 3y ago

> pointless null checks, getters and setters that have no reason to exist, useless comments, "== true", things of that nature Aside from useless comments, if someone opens a PR with any of those things, I can only assume it’s because they don’t know why they aren’t necessary. As their colleague, I feel compelled to tell them, because personally I cringe at the thought of doing things that aren’t needed and nobody having the courtesy to tell me otherwise.