My code is well commented. //eslint-disable-line all over the place.
How I review code
31–40 of 144 posts
Re: How I review code
#32I echo the author's point in "Review the code with its author in mind". Without comments, sometimes it's really really difficult to navigate the code. I have been adding more comments than ever: don't assume every line is obvious, write a comment to explain what the next few lines really do. # base case: stop dividing when we find the largest square. if width == height: return width, height else: # otherwise, we know…
On a tangent: Assuming the first line is "def largest_square_plot(width, height):", you may be interested to know that your code computes the greatest common divisor [1].
If I'd been the one to write this function, I would have done it like:
def largest_square_plot(width, height):
"""Computes the largest square to tile a plot of the given width and height."""
# because we want the grid of squares to fit exactly
# the size of the squares needs to divide both width and height
# to get the largest square, we use the greatest common divisor
from math import gcd
square_size = gcd(width, height)
return (square_size, square_size)
Re: How I review code
#33Earlier quoted context omitted.
Proving correctness is not the point of a code review. In fact it would be difficult to make such a proof in sufficiently complex software. Functional correctness is typically "proven" by tests. A code review ensures that the non-functional quality of the code is high. I.e. that the code is understandable/maintainable by someone other than the programmer himself, that there are no anti-patterns or dangerous-but-corre…
Disagree. A code-review is not purely checking for non-functional properties of the code, it's checking for overall code quality, and that includes bugs. Occasionally a reviewer will spot a bug. Occasionally there will be a false positive that turns out not to be a bug. You really want to deliberately discard this bug-finding opportunity? Why? Even if it's a false positive, doesn't that indicate that something bears…
> Occasionally a reviewer will spot a bug. Occasionally there will be a false positive that turns out not to be a bug.
And most often of all, while explaining what they've done, the reviewee will say "and this does... oh crap, wait" and fix the mistake themselves. Among all their other virtues, a code reviewer is a level 2 debugging duck. (Although all of my code review experience is with buddy check-in style reviewing rather than sending it off to be reviewed. I can understand endless nitpicky questioning being annoying if you're doing it offline.)
Re: How I review code
#34"Senior engineers sometimes need to be reminded that highly performant, abstract, or clever code is often difficult to read and understand later, which usually means asking them for more inline comments and documentation." Ha! That's not a senior engineer. Senior engineers write the most simple-looking code that just works. In every rainy day scenario imaginable. The clever code writers aren't there yet.
Ha it does make me think of this tweet https://twitter.com/KevlinHenney/status/381021802941906944
You nailed it really, senior engineers code is the most simple looking as generally they've picked the right abstraction for the problem.
Re: How I review code
#35A conundrum for me is how to get other people to code review the way I want to be code reviewed? Particularly, I noticed code reviewers on my team are pretty pedantic, obsessed with correctness, and need to be explained why each change is okay. These are people that regularly write good quality code themselves, but there is a high amount of distrust. Why doesn't a team of talented programmers trust each other? (in ca…
I'm an overly pedantic reviewer because I feel the review is one of the few places where I can counteract the accumulation of technical debt. Once the less than ideal code is in the codebase it's there to stay. I also ask a lot of pretty dumb questions during a review because I want to make sure that my understanding of the requirements matches the understanding the other engineer had.
> Once the less than ideal code is in the codebase it's there to stay.
That is not objective fact across projects. That means either the process or culture is bad.
Re: How I review code
#36"Senior engineers sometimes need to be reminded that highly performant, abstract, or clever code is often difficult to read and understand later, which usually means asking them for more inline comments and documentation." Ha! That's not a senior engineer. Senior engineers write the most simple-looking code that just works. In every rainy day scenario imaginable. The clever code writers aren't there yet.
Ha it does make me think of this tweet https://twitter.com/KevlinHenney/status/381021802941906944 You nailed it really, senior engineers code is the most simple looking as generally they've picked the right abstraction for the problem.
Text of the tweet so people don't have to click:
A common fallacy is to assume authors of incomprehensible code will somehow be able to express themselves lucidly and clearly in comments.
Re: How I review code
#37"Senior engineers sometimes need to be reminded that highly performant, abstract, or clever code is often difficult to read and understand later, which usually means asking them for more inline comments and documentation." Ha! That's not a senior engineer. Senior engineers write the most simple-looking code that just works. In every rainy day scenario imaginable. The clever code writers aren't there yet.
Ha it does make me think of this tweet https://twitter.com/KevlinHenney/status/381021802941906944 You nailed it really, senior engineers code is the most simple looking as generally they've picked the right abstraction for the problem.
There is a conundrum here: architectural mismatch.
Often, the "right abstraction" for a problem is not call/return based. So you get to choose between having the right abstraction, and code that is simple when viewed with that abstraction in mind, but "weird". Or alternatively choose the wrong but better-supported abstraction and have code that is needlessly complex but "straightforward".
Re: How I review code
#38A conundrum for me is how to get other people to code review the way I want to be code reviewed? Particularly, I noticed code reviewers on my team are pretty pedantic, obsessed with correctness, and need to be explained why each change is okay. These are people that regularly write good quality code themselves, but there is a high amount of distrust. Why doesn't a team of talented programmers trust each other? (in ca…
You probably need to get everyone on the same page on the coding idioms and style. Personally I find that non-idiomatic code can be a bit distracting when I'm trying to evaluate the code for its overall approach and architecture.
And if people are writing 150 character plus lines, those are really, no joke, a lot harder to read. If the line is going to break anyway, why not just break it yourself at a spot that makes sense?
Re: How I review code
#39> I’d rather read ten lines of verbose-but-understandable code than someone’s ninja-tastic one-liner that involves four nested ternaries.
One of my pet peeves is the inability to solve a K-Map for 6 variables and do stuff based on the resulting boolean expression. Just because it is utterly unreadable. I've tried this with many reviewers but nope.
Re: How I review code
#40A conundrum for me is how to get other people to code review the way I want to be code reviewed? Particularly, I noticed code reviewers on my team are pretty pedantic, obsessed with correctness, and need to be explained why each change is okay. These are people that regularly write good quality code themselves, but there is a high amount of distrust. Why doesn't a team of talented programmers trust each other? (in ca…
> Why doesn't a team of talented programmers trust each other?
If it's not immediately clear to a reviewer that your change is correct, then either your code isn't clear, you're not writing tests, your colleagues don't have sufficient understanding of the code base, or some (non-trivial) linear combination of the above.
The first two cases are something you can work on. If people don't understand your code, then try to write code that is more clear. If your colleagues are as talented as you say, then they should be able to understand your code without explanation. If you're not writing tests, then well, maybe there is just no hope.
If your team does not have sufficient understanding of the code base, then do communal code-review sessions where you pick parts of the system and read through it together. Or organize knowledge share sessions.
Ultimately, as Lenin said, 'trust is good, but control is better'. The whole point of code reviews is that you want to get extra eyes on your code. Trying to bypass that by asking your colleagues to blindly trust you is undermining that process and ultimately undermining the quality of your system.