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

321–330 of 376 posts

Re: How to do a code review

#321

Post by mytailorisrich »

jsolson wrote: post #320
mytailorisrich wrote: post #308

Earlier quoted context omitted.

Because a code review is not to pat each other on the back, it is to inspect and report issues. It is already costly enough without going off topic. As said, if you want to praise then you are free to do it offline. This is a professional procedure in a professional setting, not warm words from an encouraging teacher at school... I don't want to have to go through comments that do not add any value to the exercise of…

> This is a professional procedure in a professional setting, not warm words from an encouraging teacher at school... Citing a good practice in someone's code as "yes, please do more of this" alongside "don't do this please" is not, in my opinion, fluff. > I don't want to have to go through comments that do not add any value to the exercise of finding issues, and I have never seen people leave such comments in 20 yea…

> Citing a good practice in someone's code as "yes, please do more of this"

That simply isn't the purpose of a code review.

Good practices should be documented externally, so you can check them consistently during code review ;) It's also quite useful to have a checklist when doing a code review.

Re: How to do a code review

#322

Post by Noxmiles »

Terminology:

There is some Google-internal terminology used in some of these documents, which we clarify here for external readers:

CL: Stands for “changelist,” which means one self-contained change that has been submitted to version control or which is undergoing code review. Other organizations often call this a “change” or a “patch.”

LGTM: Means “Looks Good to Me.” It is what a code reviewer says when approving a CL.

quote from: https://google.github.io/eng-practices/#terminology

Re: How to do a code review

#323

Post by 8note »

jchanimal wrote: post #142
flowerlad wrote: post #129

Earlier quoted context omitted.

I can tell you what I do: I allow the total cost to increase by no more than 10% for future proofing. It is all about controlling cost. It is not justifiable to spend more than 10% of the time for future proofing because you have no idea what the future is going to be. Of course if you do have some idea about requirements coming in the near future then it may be justifiable to spend more.

It’s more than a flat cost estimate. It’s more about risk mitigation. Trying to add multi-tenancy to a system that wasn’t designed for it, can mean a major rewrite. Building the system from the beginning with the idea that there could be multiple tenants may cost 10% more, but the opportunity cost of not being able to address the multi-tenant market could be much bigger.

building a multi-tenant system without having at least two tenants from the get go means you're still likely to need that rewrite. The chances that the assumptions you made for your first tenant meet what your next one needs are pretty low

Re: How to do a code review

#324

Post by awinter-py »

Cthulhu_ wrote: post #252
awinter-py wrote: post #181

in real companies that don't have G's resources, quality and velocity are tradeoffs, not things you can have both of god bless them for trying though in particular, the idea of having one or more reviewers doing multiple rounds in a day is pure fiction in strapped startups. Rejecting a code review for readability isn't always an option. Even getting a timely review from someone who understands this part of the codeba…

And yet. Technical debt costs you time and money over time. Fixing code six months down the line takes 5-10x as much effort than just fixing it while it's still fresh. Worst case scenarios is where you end up throwing away an application and rebuild it because the technical debt has made progress grind to a halt. If there's nobody left who understands the code, or if there's hero coders who 'own' one section of the a…

when a group of people decide together what 'properly' will look like in hindsight in 6 mos, you can end up with some interesting outcomes

Re: How to do a code review

#325

Post by laythea »

badpun wrote: post #289
colek42 wrote: post #172

Earlier quoted context omitted.

Good dev's understand how to simplify complex problems by breaking it into smaller components. Bad devd add complexity to already complex problems.

Breaking down complex problem does not simplify it, you merely move the complexity to the graph of dependencies between smaller components.

I've seen this a lot. Where it is broken down so much that it involves mental gymnastics to follow whats going on. It also results in Spaghetti-code with fragmented functionality.

In my experience, projects using Java are the worst offenders here.

Re: How to do a code review

#326

Post by anshumanravi »

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…

Looks like Magento2 could make use of this, every part of it is too over-engineered, adding un-necessary complexity to make it look like enterprise ready application

Re: How to do a code review

#327

Post by mxz3000 »

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.

Sure when you're the only one working on a project. Otherwise, the only right way of writing code is that agreed upon by the whole team. Any personal opinion, while affecting the 'opinion' of the team, does not matter.

Re: How to do a code review

#328

Post by reallydude »

afarrell wrote: post #303
mav3rick wrote: post #254

Earlier quoted context omitted.

Code reviews by senior people are massively helpful for junior devs. It's one of the perks of being in a good big company that very few startups can replicate.

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.

Re: How to do a code review

#329

Post by ed_balls »

> Also, if a reviewer makes it very difficult for any change to go in, then developers are disincentivized to make improvements in the future.

Google requires you sign CLA in order to make contributions :)

Re: How to do a code review

#330

Post by mendeza »

I found the DS100 course textbook and the author describes the phenomenon here: https://www.textbook.ds100.org/ch/02/design_srs_vs_big_data....

This is insightful, but how does one deal with determing if your dataset is non-random? I can imagine manually inspecting the data and using cross-validation are ways to help identify skewed datasets. Are there any other ways to test if your data is non-random?

Post reply on HN