Hacker News new | past | comments | ask | show | jobs | submit login
Ask HN: How much time should I spend reviewing code?
30 points by lifeisstillgood on Aug 12, 2019 | hide | past | favorite | 24 comments
So I have picked up some interns for the summer. I am looking for a rule of thumb to gauge if I am reviewing often enough, so would appreciate some thoughts - 15 mins a day? 200 LOC? Pairing up or sending back reviews by email? All thoughts welcome :-)

It really varies. Someone who infrequently produces perfect 50 line pull requests for unimportant systems won’t require much time spent on code review. Someone who frequently produces bug-ridden 1000 line pull requests for mission critical systems requires a ton of time on code review.

The point is not to take a certain amount of time but rather to make sure the code meets your quality standards. Take as much time as this needs.

> The point is not to take a certain amount of time but rather to make sure the code meets your quality standards. Take as much time as this needs.

This is fine if you have time. But between managing people and doing some tasks yourself, it gets very difficult to give code the proper time to review.

Then you're taking the risk of allowing through buggy code.

That's a risk you have to weigh up against the cost of hiring more developers, or other ways of reducing/spreading the load.

There's no magic "I've done enough, so it's not my fault" number someone else can give you.

On our team we review feature branches. When the developer thinks it is ready they have an assigned reviewer. The reviewer makes comments, the developer responds, and they go back and forth until it gets to LGTM. My general rule is that I need to understand what the branch is doing and why, so the review takes as long as it takes. If it takes too long that is often itself a sign that comments or documentation is needed.

For interns, they might not know when to ask for review, so you might need to check in on them once a day.

> If it takes too long

Or that the feature branch is too big (break it into smaller changes).

Or it wasn't well designed before coding started and now you're doing CR & design review.

Or you're arguing about style/best practices and your team needs to establish coding standards.

Or ...

"A SmartBear study of a Cisco Systems programming team revealed that developers should review no more than 200 to 400 lines of code (LOC) at a time. The brain can only effectively process so much information at a time; beyond 400 LOC, the ability to find defects diminishes.

"In practice, a review of 200-400 LOC over 60 to 90 minutes should yield 70-90% defect discovery. So, if 10 defects existed in the code, a properly conducted review would find between seven and nine of them."


Thank you

Your goal is to train them, guide them, coach, mentor and turn them into good developers that you would want to hire. It's a full time job.

My approach is I start by giving them simple tasks to get comfortable. I have them pairing with more experienced folks to get hang of the teams approach. I engage them in discussions, making sure they have solid foundation for what they're working on. In my case for Enterprise app, that's making sure they really understand OOP, Making sure they understand interface and composition over inheritance, making sure they understand SOLID principles.

When it comes to code, I talk a lot about design. We can take a problem and solve it 5 different ways, and weigh the pros and cons. Sometimes there's no best way, and what's more important is understanding your approach and why. You might want to optimize for shorter development time, or for faster code execution, or for extensibility or for reliability and they all require different things. What is the current constraint of our situation calling for? Sometimes we can take the same code and rewrite it over and over in different ways till we are sick of it.

My goal with interns is to make sure they understand the fundamentals, and then to realize that there's truly a hundred ways to skin the cat, and not to run with the first solution they think of. I don't really like the review over software, other team members can do that, but I prefer to pair and we just talk casually and share screen while we talk about the code. If what they did needs a completely revamp, I might give them a pseudocode and send them off after discussion.

I make sure they understand that code is a small part of it. Did they capture the WHY of the code? Did they test? What type of test? unit? e2e? stress? Did they document on how to test/run their code? Did they write documentation?

Your code review must match up to your documented code guidelines, I show why our approach is in alignment with our guideline. You can't review without a guideline. The end goal is to have someone that will eventually become a very productive team member.

In the beginning, spending 2-4hrs is not far fetched for me. It all comes down to their strength and prior background. Good luck to your interns.

What are your concerns regarding how much time is spent?

On my team all code is reviewed by at least two peers and I find it difficult to not review changes. Sometimes people screw up requirements (or they were unclear and the developer is unfamiliar with the existing implementation), sometimes their variable names suck, or the level of abstraction is inappropriate, or the tests say they're testing one thing but really do another.

On larger code changes this can be up to an hour or two a day (depending on how many are opeened) but I see it as an investment in the health of our codebase and avoiding technical debt.

Pull it down and run it at least once a week. Comment on commits as they come in if you see something dangerous. Address problems before any code is written. There should be no surprises in a PR.

I would recommend reviewing early and initially in smaller blocks to set the standard.

Assume you're going to need to read all their code as you're likely going to be responsible for passing it (they're only interns). You could pair up, but make sure you still document any questions you have with the code. Systems like Bitbucket are great for commenting against the code and maintaining a record.

If it impacts your own productivity address that with your manager so they know why and how much. Give them the opportunity to direct you on how much time you should spend.

Don't fix anything or write code corrections yourself, don't waste time if it's clearly unreadable and unmaintainable. Question their code and let them figure it out themselves and you'll find you can get through reviews quickly and effectively.

And if you see some great code - acknowledge it!

Will anyone die because of any bad code they write? Will your company lose millions?

How much of a concern is it to let "bad" (ugly, not in keeping with the rest of it) code into the codebase?

Whats your responsibility on the team and how much of that code will be touched again to add new features or is the code critical part of a customer facing application? I personally review all code across all the applications we have as we have a mandate to do so. Everything is a consumer facing application and we mandate atleast 2 reviews on each PR, one from a senior developer considering most people are new on the team.

One thing that can be really hard is when someone turns in a lousy piece of code and you end up basically telling them how to rewrite it line by line in the review because they don't know how to do it right. These kinds of reviews need to stay positive because they can be super frustrating for both participants.

Yeah, you don't want to end up there.

When reviewing code I prefer to ask questions. - Would a switch statement be better here than a nested if/else? - Would this code be better if it was split into smaller logical chunks? - Should this code be over in this library rather than here?

You give them the opportunity to think about it, justify their decision if it's 50/50, and own the changes.

These are generally questions about re-usability.

Arguably the most important purpose of code review is to ensure the code is maintainable, i.e. the next person to look at it can figure it out. Having standards like naming/formatting conventions and test coverage should exist to support maintainability, rather than be goals in themselves.

What works for us: -specs (rudimentary ok) - design review (saves rework) - including test methodology and results in the merge request (saves rework) - code review before accepting merge request


What works for us:

* specs (rudimentary ok)

* design review (saves rework)

* including test methodology and results in the merge request (saves rework)

* code review before accepting merge request

15 min for a medium size merge request. 30-60 for large to x-large. 30 seconds for a few line low risk change. You're trying to prevent rework and weekend calls but also not burn a ton of cash on it.

Another question you can ask is : How much do you end up re-writing because of errors caused due to negligence?

You could use build agents to automate and review (static analysis) of code upto a considerable extent!

Rewriting is a misleading metric. You might not rewrite the bad code because (a) there is worse code to fix or (b) sales features are prioritised over bug fixes. There might be other reasons. Also good code might lead to rewrites as a step in the right direction refactoring wise might encourage other improvements.

I would turn the question around: How would you know you were spending enough time? How would you know you were not spending enough time?

A small amount per code review. That's why PR's should be small. Because after around 45min-1 hour you start skipping over stuff

Thank you all :-)

Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact