21–30 of 36 posts
I think I’ve reached the point where I simply don’t mention most nits. I’m most business software it just doesn’t seem worth it. If the code works, isn’t utterly obtuse, and abides by automated formatting etc, then is the nit really worth it? Granted I’m probably biased towards high churn SaaS apps, which is what I work on.
Somethings are not worth the taps.
It works out pretty well, the marginal cost of fixing the nits is very low when there's other changes to be made, so they get done. If they aren't addressed because there are no actual problems with the code, those things will get addressed later. (all the members of the team tend to take cleanup passes occasionally when they aren't feeling up to the deep work stuff but still want to do something productivish). The net effect is the codebase stays reasonably tidy without feeling bogged down in the "bureaucracy" of nitpicking.
Engineers work to deliver business value and commit the code that flawlessly delivers that value the very first time. When colleagues go to review the code, they are edified in its reading and quickly approve the merge request. There is no need for feedback because the code was perfect. I strongly disagree with the premise of this article. I look at code review the exact same way as I would look at the first draft of…
I'll often do the "first draft" as a design doc with an API sketch or a quick prototype that I'll post in chat or email to the team but not actually "mail for review" per the tool. That way I get early feedback that I'm going in the right direction for a code change. When it comes time to actually get approval to check in, then I try very hard to get it right. I put on my code reviewer's hat and review on my own code…
Yeah, we do a database & api design review before anything is actually coded. If you don't get that right you might have to basically start all over.
Earlier quoted context omitted.
I saw that too, but if there is a "real" difference in returning equal things, then it could indicate a way bigger problem in the design.
If you're using a language where there is a difference between value-equals and instance-equals, it could cause a very subtle bug by changing this code if the result of it was relying on an instance-equals later on.
Because even in the first, you are relying on the order of max being the same everywhere it matters.
Earlier quoted context omitted.
If you're using a language where there is a difference between value-equals and instance-equals, it could cause a very subtle bug by changing this code if the result of it was relying on an instance-equals later on.
I agree it can introduce bugs, my point is relying on that is probably a design issue somewhere else. Because even in the first, you are relying on the order of max being the same everywhere it matters.
As someone who works in a multimillion line codebase that sees millions of executions daily since 2005, these are real issues and saying “it’s a design issue somewhere else” doesn’t exactly fly because that “somewhere else” may no longer have a team maintaining it.
Very well written, and funny too. I wish there was a way to automate some of the lower buckets too. Things like "project consistency" (see his "12 buckets"), for instance, are incredibly important to me personally. IMHO, code should be written so that there is no visible personal style, so that newcomers and new engineers are able to follow what's going on regardless of who wrote it. A human can easily determine if t…
> IMHO, code should be written so that there is no visible personal style This seems reasonable at first but depending what you mean by visible personal style can get you into the weeds so fast. You can end up creating a process where no minor, specific improvements or experiments are tolerated unless they can be justified as generalizable improvements for the whole org. > , so that newcomers and new engineers are ab…
This isn't about camel case or snake case. It's about the overall structure and look and feel of the code. When you know that public functions are at the top, constants are in a certain place, and where package-level comments are; and when you know that important abstractions and models are defined in one file, and their implementations in separate files; and so on.
Basically, it's the combination of all the tiny things that make code look and feel like they belong.
This is similar to when people say "this is not idiomatic Go". What I'm saying is pretty much "idiomatic for this project/company/product/team". Write it so that it matches the rest of the project. That helps people navigate and read your code, which is the most important thing.
Engineers work to deliver business value and commit the code that flawlessly delivers that value the very first time. When colleagues go to review the code, they are edified in its reading and quickly approve the merge request. There is no need for feedback because the code was perfect. I strongly disagree with the premise of this article. I look at code review the exact same way as I would look at the first draft of…
I'll often do the "first draft" as a design doc with an API sketch or a quick prototype that I'll post in chat or email to the team but not actually "mail for review" per the tool. That way I get early feedback that I'm going in the right direction for a code change. When it comes time to actually get approval to check in, then I try very hard to get it right. I put on my code reviewer's hat and review on my own code…
I'll often do the "first draft" as a design doc with an API sketch or a quick prototype that I'll post in chat or email to the team but not actually "mail for review" per the tool. That way I get early feedback that I'm going in the right direction for a code change.
This is also completely orthogonal to "do I have tests for this", "have I run the linter", etc. Certainly there are types of readability problems that running the linter or writing tests for your code can catch. Having focused changes with test coverage is only one part of making sure your code is long-term maintainable by the rest of the team.
Engineers work to deliver business value and commit the code that flawlessly delivers that value the very first time. When colleagues go to review the code, they are edified in its reading and quickly approve the merge request. There is no need for feedback because the code was perfect. I strongly disagree with the premise of this article. I look at code review the exact same way as I would look at the first draft of…
If you get feedback on your code that you already knew about, you wasted someone's time, and I think that's inefficient and a little bit disrespectful.
But I definitely agree that code review is very much a drafting process. It's just that you need to walk a line.
Engineers work to deliver business value and commit the code that flawlessly delivers that value the very first time. When colleagues go to review the code, they are edified in its reading and quickly approve the merge request. There is no need for feedback because the code was perfect. I strongly disagree with the premise of this article. I look at code review the exact same way as I would look at the first draft of…
On the flip side, though, I think it's important for the code that you send to other engineers for review to be at least kind of polished, because otherwise you risk just wasting other people's time. If you get feedback on your code that you already knew about , you wasted someone's time, and I think that's inefficient and a little bit disrespectful. But I definitely agree that code review is very much a drafting pro…
And code reviews are even more collaborative then that. When I'm looking over my friend's email, it's still ultimately their email and there are types of feedback that it would be inappropriate to give. But when you're contributing to a shared codebase, it's more like a novel that's been co-written by multiple different authors—obviously you want everybody on the team to be able to have input and feedback into what the "collective style" of the project is going to be.
> Strive to be intentional, focused, and sparing in our reviews. I get this but I prefer the opposite. By over communicating your preferences, asking questions, leaving “if I follow this correctly” comments, and generally not holding back, you create lots of chances for discussion and knowledge sharing. And you’ll catch more bugs. This is especially true when a junior dev is involved.