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

351–360 of 376 posts

Re: How to do a code review

#351

Post by sorokod »

Tyr42 wrote: post #344
sorokod wrote: post #221

Earlier quoted context omitted.

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

And complex analysis informs number theory. An example from software development might be more useful.

Re: How to do a code review

#352

Post by pjm331 »

marcosdumay wrote: post #299
pjm331 wrote: post #139

Earlier quoted context omitted.

I think of it this way: How can I build this so that it only solves today’s problems but doesn’t make it overly difficult to solve tomorrow’s problems? Loose coupling, dependency injection, composition over inheritance, and similar techniques tend to be good answers to this question in my experience. In contrast, over engineering attempts to solve tomorrow’s problems before they arrive and, if they arrive differently…

> Loose coupling, dependency injection, composition over inheritance, and similar techniques tend to be good answers to this question in my experience. So, adding some extra architecture solves it?

In my experience it’s often just “different” rather than “more” but in some cases, yes, it might be a little more. It depends on where you’re at in a project. Very early on, it’s usually more. In a more established project where these patterns are already in use it’s usually not. Definitely an important thing to consider - it’s not a one size fits all solution.

Re: How to do a code review

#353

Post by tbodt »

vonseel wrote: post #112
siquick wrote: post #22

Earlier quoted context omitted.

> I've seen cases where people got hundreds of comments (many of them minor, nitpicky) from more experienced developers and were discouraged by the sheer number of them. That most new developers naturally suffer from imposter syndrome is not helped at all by 100% critical code reviews. This, combined with a large portion of developers lacking social empathy, poor communication skills, and (unfortunately) a desire to…

> hundreds of comments (many of them minor, nitpicky) This is one reason I liked Phabricator’s review system, which allowed drafting comments on an entire PR before submitting the comments. This allows you to be as nit-picky as you want when reading the PR, and then delete or modify any of them at the end. Instant submission of line-level comments, on the other hand... I think reviewers should have the awareness to u…

Critique and Gerrit (code review systems at Google) also do this.

Re: How to do a code review

#354

Post by mytailorisrich »

patthebunny wrote: post #349
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…

Sometimes people don't want to actually review but feel they have to leave comments.

Trivial comments are a sign of that.

Re: How to do a code review

#355

Post by Sammi »

Gibbon1 wrote: post #36
Glyptodon wrote: post #27

Earlier quoted context omitted.

Some places I've worked explicitly won't push/deploy/similar on Friday.

I've mostly worked for small industrial light manufacturers. First job I worked at they were always late with shipping stuff. And always taking stuff down to the UPS office after hours. After watching this for a few years I got the shop manager to adopt a rule. If it's not ready to be boxed up before lunch, it's not shipping today. After they started enforcing that they got a lot more productive and started shipping…

Your choices are often between these two:

1. Higher throughput, higer latency and correctness on one side.

2. Lower throughput, lower latency and some risk of faults on the other side.

It's like the difference between tcp and udp.

The question is: Where do you need to be? What are your requirements?

Re: How to do a code review

#356

Post by Thiez »

reallydude wrote: post #328
afarrell wrote: post #303

Earlier quoted context omitted.

Agreed. I once left a job in large part because there wasn't anyone else to review my code.

It seems incredibly self-defeating, to quit over a lack of CODE REVIEW at any point in someone's career. Hiring someone just to review your code is not a sane business decision, so you might want to think about how you overvalued that aspect. I would be very surprised if someone could make a business out of 3rd party code reviews, but stranger things have happened.

How is that self defeating from the programmers perspective? Code reviews can contribute to growth as a developer, ultimately helping ones career. When a developer desires such reviews, and a company is either unwilling or unable to provide them, why should they stay? There are plenty of other companies around to work at.

Re: How to do a code review

#357

Post by afarrell »

reallydude wrote: post #328
afarrell wrote: post #303

Earlier quoted context omitted.

Agreed. I once left a job in large part because there wasn't anyone else to review my code.

It seems incredibly self-defeating, to quit over a lack of CODE REVIEW at any point in someone's career. Hiring someone just to review your code is not a sane business decision, so you might want to think about how you overvalued that aspect. I would be very surprised if someone could make a business out of 3rd party code reviews, but stranger things have happened.

So suppose you met someone who graduated uni a year ago, was working as the sole developer at a non-profit, and wanted to learn to be better at recognising and writing well-structured code. Suppose they then asked you if they should stay at their job or go work for a company where they were working with other software engineers and you said they shouldn't. How would you advise them to develop their sense of good code style?

Re: How to do a code review

#358

Post by dmitrygr »

tylerl wrote: post #236
dmitrygr wrote: post #41

Earlier quoted context omitted.

If you live in CA, you're good to do it: https://codes.findlaw.com/ca/labor-code/lab-sect-96.html as long as you do it on your time, using your own resources, and do not compete with your workplace. The fact that google (or other employers) do not go out of their way to tell you this is, of course, understandable.

You'll be hard pressed at a well-established tech company like Google to find even one programming topic wherein you're not legitimately competing with your employer. Google's IARC process (where they officially sign over their claim to a given invention) is really, truly unusual. They absolute are going out of their way to be accommodating. Even their (super liberal) open-source process was practically unheard of wh…

Being fired isn't a big deal. In silicon valley you could spit, and hit three companies that'll hire you, at least one of them good

Re: How to do a code review

#359

Post by infinitezest »

NKCSS wrote: post #242

Here's my workflow and it works very good to get everyone on the same level: - Each night, I go over all the commits of the day and do a code review - Each morning at 9:00, we go over all the comments on the commints, with everyone, talk about it and make sure everyone understands. This allows me to explain more advanced or new concepts that one developer uses to make sure everyone understands. It allows me to introd…

This is an interesting approach. How many engineer hours do you think you spend each day doing this? If you're reviewing CLs as a group, it seems like it could get very expensive very quickly, no?

Re: How to do a code review

#360

Post by erikpukinskis »

0x445442 wrote: post #313
siempreb wrote: post #261

What is good design, what is good naming, over-engineering, etc, etc.. How on earth can a group of people agree on all those points? Look around here on HN, all those discussions.. Do we have consensus? No, and that is OK, but it's not in a team by code review. There is only one way a group of people can have healty code reviews, that is when they like each other so much, or enough to accept what they not agree on. B…

I agree with you. This "Agile" cancer that has infected the industry needs to die now. Someone here called it collectivized micromanagement and that's the best term I have heard for what's become of "Agile".

I don’t think you should accept bastardizations of terms into your lexicon.

Yes, acknowledge when someone means “micromanaging” when they say “agile”, and act accordingly. But don’t redefine the word in your own head, otherwise how can you even speak?

Like, what word do you use now for the basic principle of agile development now that you’ve changed it to mean micromanagement in your head?

Post reply on HN