
Ask HN: How much time should I spend reviewing code? - lifeisstillgood
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 :-)
======
lacker
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.

~~~
arvinsim
> 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.

~~~
AndrewDucker
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.

------
tetron
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.

~~~
jasonpeacock
> 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 ...

------
thangalin
"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."

[https://smartbear.com/learn/code-review/best-practices-
for-p...](https://smartbear.com/learn/code-review/best-practices-for-peer-
code-review/)

~~~
lifeisstillgood
Thank you

------
segmondy
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.

------
arenaninja
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.

------
chatmasta
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.

------
dwd
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!

------
davidw
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?

------
theshadowmonkey
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.

------
tetron
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.

~~~
dwd
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.

------
tetron
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.

------
atsaloli
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

~~~
atsaloli
[reformatted]

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

------
ykevinator
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.

------
abhinuvpitale
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!

~~~
quickthrower2
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.

------
PaulHoule
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?

------
bytematic
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

------
lifeisstillgood
Thank you all :-)

