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

How to do a code review

google.github.io

Post reply on HN
Print view View original

341–350 of 376 posts

Re: How to do a code review

#341

Post by YZF »

cr1222 wrote: post #208
YZF wrote: post #80

> Some hardward manufacturers only ship new hardware once a year. A line snuck in through their code review. One big issue that I'm not really seeing discussed is that in many cases code reviews end up being very subjective. "hard to maintain", "not readable", "brittle", "needs more tests" are all super subjective. There is no magic tool that can tell you the future maintenance costs (vs. the current costs), whether…

Things like maintainability are hard to objectively define and measure, but still important. If you let issues pile up a medium-to-large team working on a codebase for a few years can turn it into something completely incomprehensible and and impossible to chain. I make a lot of comments along those lines when I review code and I think overall it's helped significantly with technical debt and it would be very limitin…

I definitely agree with the overall sentiment; one needs to be constantly on guard to maintain a good code base. I don't think it's just a matter of code review though. A bad team will take this "how to do a code review" and still produce bad stuff. A good team probably doesn't need "how to do a code review". The best code reviews I've participated in were more informal affairs. They're generally in person discussion rather than the distant commenting on some review platform.

Re: How to do a code review

#342

Post by jdhzzz »

Nit: Under Every Line The term "scan" is used where "skim" would be a better term.

Scan can mean either "to investigate thoroughly by checking point by point and often repeatedly" or "to glance from point to point of often hastily, casually, or in search of a particular item". Skim leaves no room for doubt: "to read, study, or examine superficially and rapidly"

Re: How to do a code review

#343

Post by mav3rick »

xthrow wrote: post #298
chrisseaton wrote: post #279

Earlier quoted context omitted.

I think the hiring bar at places like Google and FB is astronomically higher than almost all other places - they definitely have generally higher skilled people there on average.

Checking for memorized algorithms and an MIT degree do not guarantee good code. I've seen more horrible code at a FAANG company than in many other places. Complete absence of overflow analysis in C++, race conditions, architecture astronauts, exploding code bases that no one understands any more. Features are being added on a daily basis, basically what matters is a high line count.

You clearly haven't seen code at Google and its "base" library.

Re: How to do a code review

#344

Post by Tyr42 »

sorokod wrote: post #221
de_watcher wrote: post #217

Earlier quoted context omitted.

Sometimes you can generalize the problem a bit to get a shorter solution.

Pretty much by definition you will be covering a wider surface of possible inputs and behaviours and so acquiring complexity

Here's a fun example from math which requires generalizing to get a good solution to.

Suppose you have a 2^n x 2^n sized courtyard. You have one 1x1 statue, and unlimited L pieces (2x2 with a corner missing).

You would like to have a layout which places the statue in one of the centermost tiles, and fill the rest with L pieces.

----------------

Solution

----------------

Define a layout function with allows for the empty tile to be in any corner. This is trivial for 2x2, as it's just an L peice.

By tiling these squares, you can solve for the next size up. Put three with the hole in the center and fill in with an L piece to get the bigger square.

At the end, take 4 2^(n-1) squares and put the holes in the middle. Add one L and you are done.

What was the point of that?

By generalizing your solve function to have more outputs, it lets you build up a structure from it, in which it's easy to solve your actual goal.

Re: How to do a code review

#346

Post by SkyBelow »

jf22 wrote: post #317
SkyBelow wrote: post #310

Earlier quoted context omitted.

The problem comes in when you can anticipate a problem and also anticipate not being given the resources to solve the problem then when you do have those resources now. For example, had a user who wanted a document produced in one specific format. They promised this was what they wanted and it wouldn't change. It changed 5 times as we were nearing release. So I over-engineered it to allow a template to produce the do…

If the user did not have that problem wouldn't you have wasted engineering time?

It was a calculate risk, same as many others, and by far not the most wasteful event of that year even had it not been needed.

Re: How to do a code review

#347

Post by lunchables »

I had to Google it because I couldn't find any definition anywhere. A "CL" is a change list - maybe, that's what I found from my Google search. It is a term used dozens of times on that site but never defined, as far as I could find.

Re: How to do a code review

#348

Post by patthebunny »

It's a shame my irrational hatred and distrust of all things google prevents me from reading this document.

Also is it really that insightful? Code reviews have been standard at competent companies for a long time.

Re: How to do a code review

#349

Post by patthebunny »

aviraldg wrote: post #13

This is great advice and isn't followed often enough, especially when reviewing code written by people new to an organization/team: > If you see something nice in the CL, tell the developer, especially when they addressed one of your comments in a great way. Code reviews often just focus on mistakes, but they should offer encouragement and appreciation for good practices, as well. It’s sometimes even more valuable, i…

I did a pretty reasonable chunk of C++ code once years ago. I was junior level. Wanted a good review.

I got 21 comments to remove blank lines and none about the code. Thanks. Really useful. This was the team of "experts" on the code base.

Then a principle engineer reviewed some other code and oh look, actual useful comments.

Nitpicking can be worse than useless. Obfuscates or ignore real issues in the code. Seems to usually be a sypmtom of the reviewing not being competent enough to actually review the code properly.

Re: How to do a code review

#350

Post by uryga »

baby wrote: post #339
uryga wrote: post #293

Earlier quoted context omitted.

isn't "zero-cost abstraction" shorthand for "zero- performance -cost abstraction"? i.e. computer, not human, performance?

It is! Which is why I don’t like the term. It often masks the fact that it has a cost in complexity.

i've never read it that way! could you give an example of that?
Post reply on HN