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

331–340 of 376 posts

Re: How to do a code review

#331

Post by jorblumesea »

Shaanie wrote: post #225
jorblumesea wrote: post #189

Earlier quoted context omitted.

That's kinda funny, because their interviews are exactly the opposite. Toy problems you'd never see in the real world, crazy abstracted solutions, O(n) demands, almost encyclopedic knowledge of data structures and algos. Do as I say, not as I do I guess.

I suppose you just wanted to complain, but in case you're serious: interviews and code reviews fulfill entirely different purposes. Choosing the right data structure may or may not be over-engineering, it depends on the application. Google absolutely want developers that are capable of over-engineering.

> Google absolutely want developers that are capable of over-engineering.

Why, so, when they get on the job, they need to be told not to over-engineer?

Re: How to do a code review

#332

Post by Traubenfuchs »

meitham wrote: post #273
Traubenfuchs wrote: post #245

I have never not seen code reviews end up being a massive social/cultural pain point for most people involved. Most people hate getting their code reviewed in depth. In situations where there isn't just one lead dev who is responsible for reviews, this leads to "Merge Request symbiosis", where two people uncritically approve each others requests so they don't have to deal with the third.

I agree, my role of thumb is to only review code if I was asked by the author to do so, and never offer to review someone's else code. Code is a bit like music, there is very little right or wrong, but a lot of flavours and opinions.

Very little wrong? You haven't seen enough code! I have seen everything in merge requests: From liquibase scripts that would have wiped the production database to production secrets in yml files.

Re: How to do a code review

#333

Post by gwd »

petilon wrote: post #99

Here's the part that resonated with me most: A particular type of complexity is over-engineering, where developers have made the code more generic than it needs to be, or added functionality that isn’t presently needed by the system. Reviewers should be especially vigilant about over-engineering. Encourage developers to solve the problem they know needs to be solved now, not the problem that the developer speculates…

> where developers have made the code more generic than it needs to be, or added functionality that isn’t presently needed by the system.

I agree with this when it's internal interfaces. When you have public interfaces that you expect to have to support in a backwards-compatible manner for (ideally) years in the future, it's worth taking some time to think about how people might want to extend it in the future.

Re: How to do a code review

#334

Post by kortilla »

chrisseaton wrote: post #279
kortilla wrote: post #222

Earlier quoted context omitted.

>But at good companies like Google good architecture is a given. Absolutely not. Google’s interview process and inflow of fresh graduates does not bode well for good architecture. Having spent time at G and FB, I can certainly tell you that employees at both are no better at architecting code in a sane way than SWEs at other companies. Code architecture requires experience. Google does not.

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.

The hiring bar at Google does not test for the ability to organize code coherently. The skill is completely unrelated to algorithms and data structures.

It’s like requiring candidates to deadlift 500 lbs and then assuming it means they can all run marathons.

Re: How to do a code review

#335

Post by alexhutcheson »

gwd wrote: post #333
petilon wrote: post #99

Here's the part that resonated with me most: A particular type of complexity is over-engineering, where developers have made the code more generic than it needs to be, or added functionality that isn’t presently needed by the system. Reviewers should be especially vigilant about over-engineering. Encourage developers to solve the problem they know needs to be solved now, not the problem that the developer speculates…

> where developers have made the code more generic than it needs to be, or added functionality that isn’t presently needed by the system. I agree with this when it's internal interfaces. When you have public interfaces that you expect to have to support in a backwards-compatible manner for (ideally) years in the future, it's worth taking some time to think about how people might want to extend it in the future.

Within a monorepo like most code at Google, there isn't as much of a bright line between public and internal interfaces. If you don't get a public interface quite right on the first try, but it still has This assumes that the interface is a function or method call within a binary. If it's an RPC, then making changes is much trickier, since you can't assume that both sides of the call were built at the same commit. This requires a lot of thought to make sure any changes to your RPC messages are both backwards- and forwards-compatible.

Re: How to do a code review

#336

Post by electrograv »

enneff wrote: post #118
electrograv wrote: post #20

Earlier quoted context omitted.

As a general rule, absolutely! Unfortunately, if applied too strictly though, it can be weaponized or stifling in ways you may not anticipate (which tends to be true of almost any rule used too universally). For example: I’ve seen cases where there’s disagreement as to which of two technical paths to pursue, where one side (not liking the way consensus is currently leaning) demands a data driven proof that one of the…

> in this case, it would take no less work/time to gather said data than to entirely implement both solutions and directly compare them. So why not do this?

In this case either of the solutions would satisfactorily solve the problem, so implementing both would be redundant and wasteful. The problem appears in this case when there’s disagreement on which of the two satisfactory solution paths should be taken. Implementation work cannot proceed until it’s decided which solution path is to be taken (even though they both work), but there’s no way to prove which is best aside from general intuition arguments, or actually implementing both (which as mentioned, would be a waste of resources for no benefit).

Re: How to do a code review

#337

Post by electrograv »

heavenlyblue wrote: post #39
electrograv wrote: post #20

Earlier quoted context omitted.

As a general rule, absolutely! Unfortunately, if applied too strictly though, it can be weaponized or stifling in ways you may not anticipate (which tends to be true of almost any rule used too universally). For example: I’ve seen cases where there’s disagreement as to which of two technical paths to pursue, where one side (not liking the way consensus is currently leaning) demands a data driven proof that one of the…

Isn’t that simply a sign of deeper problems in the organisation?

Yes most likely; that’s my point. It can become a serious organizational problem when useful codified heuristics like this are able to be abused in this way (by treating them as immutable rules, or used as a weapon to win arguments).

Re: How to do a code review

#338

Post by e12e »

ridaj wrote: post #151

It's very interesting that so much of it is about the social aspects of engineering - a lot of it kind of reads like " don't be a jerk ". In CS engineering classes, where presumably we would learn to become great engineers, I don't recall learning about any of this, and instead I remember the emphasis being on technical knowledge and accomplishment. I'd probably have been a better engineer in my early career if I'd u…

> It's very interesting that so much of it is about the social aspects of engineering - a lot of it kind of reads like "don't be a jerk". In CS engineering classes, where presumably we would learn to become great engineers, I don't recall learning about any of this, and instead I remember the emphasis being on technical knowledge and accomplishment.

Yes. Dorm life is supposed to provide the social aspect. And to a lesser extent, real group assignment/projects.

Re: How to do a code review

#339

Post by baby »

uryga wrote: post #293
baby wrote: post #195

Earlier quoted context omitted.

Thanks! I agree so much. Zero-cost abstraction is a misnomer.

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.

Re: How to do a code review

#340

Post by jf22 »

kccqzy wrote: post #200
petilon wrote: post #99

Here's the part that resonated with me most: A particular type of complexity is over-engineering, where developers have made the code more generic than it needs to be, or added functionality that isn’t presently needed by the system. Reviewers should be especially vigilant about over-engineering. Encourage developers to solve the problem they know needs to be solved now, not the problem that the developer speculates…

Does Google really practice what they preach though? Just recently I was looking at Angular, Google's web frontend framework. Services, modules, directives, angular-specific markup. Coming from React, I find this grossly over-engineered.

React does must less than Angular...
Post reply on HN