161–170 of 302 posts
Worth noting that Google engineers had this level of satisfaction long before the mostly useless AI suggestions came on the scene. Those additions are comparatively recent.
Earlier quoted context omitted.
Generally, it goes something like this imaginary Slack convo: Me: I think we are getting events from SQS out of order and that’s why we are seeing some weird synchronization issues. What do you think about sending events to the regular queue and a FIFO queue at the same time and comparing them? Team: How would that work? Me: Since we are using a single threaded consumer here, I think we can simply override the transp…
> There won’t be any surprises in the review. There can always be surprises. Sometimes when you see something implemented you can get a « click » as to why another solution would be better. It happens to me regularly both on the receiving and giving end of this and it’s rarely a big deal. Most of the time it happened to me, I could reuse the functional tests I had written and parts of the code anyway. Reworking on so…
We all see these suggestions. That’s ok, and wanted. But to say “ah, no I think it should be the other way.” [reject]
That’s what I was talking about here.
> Developers need to make progress; overly difficult reviews can discourage future improvements. This, to me, is the most important aspect of code review: get out of the way . If you are putting in nit picks, asking for changes that aren't related to a bug or bad architecture choice, etc, then get out of the way. If you are holding up the approval because you want someone to rewrite the code the way you would have wr…
Yeah no, part of code review is ensuring code quality is maintained. If you start letting badly written code fly because it passes tests, that’ll bite you. If something isn’t blocking I’ll make a note that it should be fixed if theres budget knowing sometimes you gotta let things go. But you can’t just ignore code quality and call it nitpicking.
If a unit of code fixes or improves the user experience without slowing it down and is memory efficient, then it becomes incredibly hard to objectively define said code as being bad. Perhaps it ruins the developer experience, but then again, many programmers object to things they simply don't like on a philosophical or theological basis. Programmers, depending on the language community, may consider anything that isn't "object oriented" to be poor quality, but good luck finding objective proof that object orientation is always better.
Earlier quoted context omitted.
I'd wonder what the rest of that API surface looked like. If the rest of the functions were longDescriptiveCamelCase() and then you tried to slip in mkdir(), then I'd say yes, "stylistic consistency" was gained during that unnecessarily difficult back-and-forth. If the other functions were chdir(), remove(), rmdir() and so on, and somehow the reviewers picked your code change as the time to change it all, then yea, w…
It was a mix of short and long and there wasn't really much "style". The company had taken over the maintainership from someone else at some point (before I joined) and it was all fairly inconsistent. I don't really mind the inconsistencies as such, it's just the argueing over nothing that I mind. This kind of stuff was typical. At some point there was a lengthy discussion which prevented rolling out a rather importa…
No? You can clearly have loops that exit. Loops that can be infinite should only be so where you really want that to happen in those cases / are fine with it. "Infinitely until the user does X with no timeout" is a fair one.
I generally like the tool. When your code review tool is really nice one unexpected negative is that it can create a culture of nitpicking. Sometimes it’s not worth arguing over small details like variable naming but the tool makes it really easy for things to head in that direction. Sometimes variable naming isn’t a small detail. But sometimes it is and it’s a waste of everyone’s time to argue about it.
Earlier quoted context omitted.
Excessive subjective feedback can be soul-crushing.
Anything subjective that doesn't fix an identifiable execution problem must be explicitly labeled as a suggestion . You don't get first choice over the code because you are asked to be the reviewer. You are there to (1) catch mistakes and (2) teach the other coder if they appear to not know something useful that you do know. If you have a preference about a simple stylistic matter that is not covered by your style gu…
I generally like the tool. When your code review tool is really nice one unexpected negative is that it can create a culture of nitpicking. Sometimes it’s not worth arguing over small details like variable naming but the tool makes it really easy for things to head in that direction. Sometimes variable naming isn’t a small detail. But sometimes it is and it’s a waste of everyone’s time to argue about it.
Writing readable code is not "nitpicking". It is common that an author doesn't see why their parameter name or function name is misleading but their reviewer, who comes to the change without as many preconceptions, sees it right away.
To put it this way, you should use Kotlin vals with get methods with getters rather than fun getXXX() functions, but this is controversial, so I always suggest it as a nit (Nit: maybe use val xxx get() = ... rather than fun getXXX() = ...).
Earlier quoted context omitted.
Anyone who has ever come along in a code review and told me my code “isn’t up to standard” will inevitably start a very long game of attrition with me. Here’s the thing: there’s no such thing as “standard” software architecture (until someone writes a new kind of software architecture called “standard”). I’ve seen and used everything from MVC to DDD to TDD to MVVC to whatever React is to reactive to event-oriented to…
I'd like to agree. But there is one part of that distinction that is real: there is such a thing as low quality code.
Earlier quoted context omitted.
Yeah no, part of code review is ensuring code quality is maintained. If you start letting badly written code fly because it passes tests, that’ll bite you. If something isn’t blocking I’ll make a note that it should be fixed if theres budget knowing sometimes you gotta let things go. But you can’t just ignore code quality and call it nitpicking.
Badly written, or badly designed? Bad code, to me, would be an unrolled for-loop, or a for-loop where a foreach loop would do better (though now, we're getting in the realm of nit-picking). A PR is waaaaay too late to bring up architectural/design improvements (unless it is a WIP PR opened expressly for discussing the approach). I very rarely see bad code in PR's unless it is from a Junior programmer, and even then,…
Besides, that, I mostly agree with your stance on code reviews. The way that most programmers do code review is so very "waterfall" and they don't even realize it.