Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

How I review code

engineering.tumblr.com

Post reply on HN
Print view View original

51–60 of 144 posts

Re: How I review code

#51

Post by benji-york »

taneq wrote: post #33
MaxBarraclough wrote: post #25

Earlier quoted context omitted.

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 reviewi…

This "oh crap" effect is one of the reasons I like "self review" as the first step in a code review process.

That is, go over the diff and add commentary explaining to the reviewer why you made particular changes (not explaining what the changes do, that should be in comments in the source).

Re: How I review code

#52

Post by dboreham »

pmcollins wrote: post #44

> We have repositories for the PHP backend, our database schemas, our iOS (Swift/Obj-C) and Android (Java/Kotlin) mobile apps, infrastructure projects written in Go, C/C++, Lua, Ruby, Perl, and many other projects written in Scala, Node.js, Python, and more Why do organizations allow this? I realize that some platforms require their own languages (iOS, Android), but outside of that, just pick one or two and hold the…

Well there's a can of worms!

Thoughts based on experience:

Asymmetric information situation: newly hired smart engineer says that new subsystem should be written in . He says it is so much better than . Everyone on HN says it is so much better. You however, haven't had the time to try it out on a medium sized project to determine if this is true or the usual new language hype. New Engineer seems to know plenty about it and is insistent. Do you tell them no, or do you let them run? Well now you have N+1 languages. Iterate.

Some engineer in your organization develops something that turns out to be useful, as a back-burner project without official approval. They do that in whatever language they think will look good on their resume. Do you pay to have that thing re-written in one of the house languages or do you let it ride and add it to the mix. N+1 languages. Iterate.

And...in general good luck with not "allowing this" in the context of software developers. Cat herding and all that.

Re: How I review code

#53

Post by yaccz »

yeukhon wrote: post #2

I 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…

* receive email from GitHub / git-send-email / whatever

* Maildir/new is queue

* more emails in the same thread bump priority

* emails marked unread are outstanding PRs

Re: How I review code

#54

Post by Sir_Cmpwn »

pmcollins wrote: post #44

> We have repositories for the PHP backend, our database schemas, our iOS (Swift/Obj-C) and Android (Java/Kotlin) mobile apps, infrastructure projects written in Go, C/C++, Lua, Ruby, Perl, and many other projects written in Scala, Node.js, Python, and more Why do organizations allow this? I realize that some platforms require their own languages (iOS, Android), but outside of that, just pick one or two and hold the…

Why?

Re: How I review code

#55

Post by eyalm »

anameaname wrote: post #3

A 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…

> 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?

This has nothing to do with trust or any other personal matter. Everybody makes mistakes, everybody might misinterpret code they use and not every programmer has a complete understanding of the scope of the changes they are making. This happens for both senior and junior engineers. (Obviously one would expect this happening much less for seniors)

The only way minimising the amount of future issues is to have thorough, rigorous and pedantic code reviews. Another major benefit for this approach to code reviews is having more people on the team understand deeply the component you coded.

Re: How I review code

#56

Post by snarf21 »

yeukhon wrote: post #2

I 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…

I recently read an article that was linked from HN comments on a different topic. It spoke about how software is not about code or documentation but rather about building complex mental models of the systems, which the author called the Theory Building View. It was interesting to read but one thing popped into my head while reading it. Too often we make comments about what a function does, which may be necessary but is not sufficient. My light bulb clicked on and I thought that what we really need is to document the function's reason for existing. This doesn't change even if the code inside does and is the thing that you actually really care about as it gives you some idea about the architecture.

Re: How I review code

#57

Post by benji-york »

Vinnl wrote: post #7
anameaname wrote: post #3

A 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…

If you can't see why a line of code is okay without looking at it, doesn't that mean that it might need a comment or something explaining why it is? As a corollary, when I ask a question in code review, I usually don't want it to be answered there - I'd prefer it to be answered in the code, if need be using a comment.

I mostly agree, with a small twist: sometimes the question is about the code; sometimes it is about the change. The code should be described in the file; the change should be described in the review.

This discussion makes me wonder about more tightly connecting the code with past reviews. What if when reading some code, the user had not only access to the revision history, but also the review history around the code in question? Something along the lines of storing reviews in revision control metadata.

Re: How I review code

#58

Post by [deleted] »

yorwba wrote: post #32
yeukhon wrote: post #2

I 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…

[deleted]

Re: How I review code

#59

Post by DenisM »

yeukhon wrote: post #2

I 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…

FYI: If your variables are floats you will be splitting hair by the time this code terminates.

Re: How I review code

#60

Post by ot »

taylodl wrote: post #29

"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.

Reminds me of the "Evolution of a Haskell programmer":

https://www.willamette.edu/~fruehr/haskell/evolution.html

Make sure you don't miss the punchline, "Tenured professor".

It's the same with the progression of engineering seniority: increasing levels of cleverness and unnecessary sophistication, until you reach a point where you don't have anything to prove anymore, and you can feel comfortable writing the simplest and most elegant solution.

Post reply on HN