
Effective Code Reviews – A Primer - pplonski86
https://deepsource.io/blog/code-review-best-practices/
======
z5h
Whoever OKs a code review can be called upon at any moment to explain the
logic of the change in front of the team. If you OKed it, you must understand
it, right?

If no one is willing to accept a PR with that provision, the author needs to
make their code clearer.

In my experience, people doing a code review don't want to bruise their egos
by saying "I don't understand this.". So they superficially check the code and
OK it.

~~~
Benjammer
There are also places where the incentives just are not there for in depth
code reviewing. You could work in a culture where you are literally hurting a
junior's chances of promotion by being tough on their code and asking for
changes, because their peer on another team is getting twice as many tickets
"done" because his team just "rubber stamps" LGTM on everything.

Many teams would never actually put in the effort to trace issues back to
individual failures in a code review process, when they could be spending the
time just fixing the problem and moving forward.

------
BeetleB
What is completely missing, and IMO more important, is how to be the
_reviewer_.

Most reviewers have a tendency to go the Socratic approach: Asking questions
to lead to an answer. In my opinion, and consistent with recommendations from
books on general communication: Don't ask questions _without_ expressing your
concern.

Example:

Q: Why did you do X here?

Completely valid and sincere response:

A: It solved the problem.

The coder at this stage has no idea why the reviewer is asking, and there
could be many possible answers depending on his concern, but since the
reviewer did not state his concern, we are reduced to the simplest,
noncontroversial answer.

Instead, the question should be:

Q: I'm concerned that doing X here has the following drawbacks: A, B and C.
Was there a particular reason you chose to do X here?

Or even better:

Q: I'm concerned that doing X here has the following drawbacks: A, B and C. If
you do Y instead, you don't have these drawbacks. Was there a particular
reason you chose to do X here?

A: Yes, because although X has drawbacks A, B and C, Y has drawbacks D and E.

(And the conversation can continue).

~~~
graton
I recommend not using words like "you" and "your". It can make people
defensive. Try to only talk about the code and changes. Not the person who
wrote the change.

Q. Why is X done here?

Q. What is the reason to do X here.

~~~
cjcenizal
I also love the wording "Can you help me understand the reasons behind X?"
because it shows that you're a) just trying to understand/learn and b) you
respect the other person, because you're asking for their help.

------
valuearb
I’ve found reviewing the code together face to face (or video call) with
author is very effective. It allows you to discuss why they made certain
decisions, why you recommend different choices, and the relative benefits of
each option.

It also the best forum to remind them that my code isn’t perfect either, that
this is a process to help make us both better developers.

~~~
eequah9L
Online code review puts pressure on the reviewer to find the problems quickly
so as not to take the other engineer's time. Usually I want to take my sweet
time, look at the surrounding code, figure out things for myself. Sometimes I
just want to stare at the screen for five minutes to think about it. That's
really uncomfortable when there is another person on the other end of the
line.

If something is so unclear that I cannot figure it out on my own, the code /
commit message / cover letter likely need to be improved.

Nothing like git-blaming your way to a two-year old commit and get a commit
message that actually explains the whys and trade-offs.

I agree that some things do need discussion, but code review tools typically
have some sort of chat interface for that purpose. I am doing open source work
and most of the time just do code review over the e-mail, which obviously
works well for this purpose. [Edit: and of course the calls work as well here!
In any case, the result of the discussion should make its way to the code /
commit messages / cover letter.]

~~~
EB66
> Online code review puts pressure on the reviewer to find the problems
> quickly so as not to take the other engineer's time. Usually I want to take
> my sweet time, look at the surrounding code, figure out things for myself.
> Sometimes I just want to stare at the screen for five minutes to think about
> it. That's really uncomfortable when there is another person on the other
> end of the line.

Yes, absolutely this.

I think it's essential that the reviewer takes their time to inspect the code
without distractions and without being rushed.

If you want to have a face-to-face code review, in my experience it works
better to have that meeting after finishing the code review, so you can go
over your review and discuss each point in greater detail.

~~~
valuearb
These are both good points that I neglected. In fact that's actually what I
do, I never do a face to face unprepared, I always have a list of things I've
found that I want to review.

Another thing that I maybe didn't make clear enough was it's important to be
emphatic, and treat them as equal team-members, even in a Sr to Jr review.
Give them an opportunity to weigh in on why they made certain choices and
discuss all the pros/cons. Sometimes I even change my mind and agree with
their choices when it's clearer what their objectives were.

------
beager
Exceedingly superficial, unfortunately, as it seems all the content marketing
on this blog is.

------
edw519
No real insight is offered here, just a list of "givens":

    
    
      1. Keep the changes small and focused
      2. Ensure logical coherence of changes
      3. Have automated tests, and track coverage
      4. Self-review changes before submitting for peer review
      5. Automate what can be automated
      6. Be positive, polite and respectful
    

Who could argue with these?

I've always believed there are 2 levels of code review, defined by their
deltas:

1\. Review against standards, defined by the delta between the code and shop
standards, which should be well defined for everyone ahead of time. Things
like formatting, white space, variable naming, conditionals, iterations,
standard modules, etc. Every shop has (or should have) a standard way of doing
a lot of these things. Much of this level of code review can be automated.

2\. Review against expectations, defined by the delta between the code and the
technical specs. Of course this requires technical specs which many shops
don't do. If you write technical specs, and _peer review the specs_ , agreeing
that _this_ is what is expected, then this level of code review becomes much
more straight forward. This includes things like business rules, logic, data
base updates, UX, interfaces and APIs with other apps, etc. This is much
harder to automate, but offers a checklist to streamline the review and keep
it objective. It also makes User Acceptance Testing easier down the line
because code review has already eliminated a large subset of potential UAT
rejections.

I'd love to hear how others handle these 2 levels of code review. Now _that_
would be much more valuable than this list of feel good platitudes offered by
the OP.

~~~
ponyous
> Who could argue with these?

Maybe not argue, but I still find this link useful. I will link it to more
junior engineers in my team. Believe it or not, not everybody does self-review
before they ask for peer review.

~~~
misterman0
>> Believe it or not, not everybody does self-review

That's not hard to believe given the stream of shit that comes out of a feed,
any feed.

But I'm appalled by y'all's assumption that #3 is a given. Why do you all
believe that?

To me, including a test suite in your code base is a code smell.

~~~
ponyous
Well depends, having 0 testing sucks too. I would argue 100% TDD can be a code
smell, especially if you try to unit test everything. Use testing where it
makes sense. Got some complex business logic in some function with several
different edge cases? Cover them in test cases, it will also serve as
documentation.

If you have a problem with #3 you might as well have the problem with #5. You
don't want to automate everything, only where it makes sense.

~~~
misterman0
>> Use testing where it makes sense.

I agree!

>> Got some complex business logic

So complex the only one who understands it is...who? A test?

~~~
volkk
i'm not sure what you're arguing here. if you have a complicated codebase you
write tests so that when you add other code, you have a bit more confidence
that you're not breaking things. furthermore, if a new person joins a team,
they can use old tests to understand what the hell is happening in the code
otherwise you just have an unadulterated mess of code to read through.

you're dealing with humans on a day to day basis that err very easily. the
dumb mistakes i've made because there was a lack of tests are countless.

------
40acres
In my experience code reviews need to be a conversation. I'm on a small scrum
of 3 developers and we all review each other's code. The major thing I've
learned is that you need to depersonalize the code. Don't ask "why did you do
this", or "you did it this way two weeks ago and now it's failing in
production". It's all about the we. I say we so much in my code reviews you'd
think I was French. Also, I really try to make my tone seem more of someone
who wants to learn vs. someone who is criticizing.

------
tempguy9999
Honest question: A couple of years ago I worked for a startup. They used
Agile, which mandates code reviews. "Brilliant", I thought, "that can only be
good." I'd never really had code reviews before even though I've got decades
in industry. I like the idea.

However there was no written (or often, even oral) statement of what it was
supposed to do so all I could do was spot syntactic cruft and offer cleaner
code, and... that was about it.

To me, without clear statement of code's intent it must be impossible to
review that code meaningfully, but I've never come across this most basic
requirement in Agile that the code must have a spec of some sort to review
against.

What am I missing, or was their Agile practice simply wrong, or what? Genuine
question.

~~~
Benjammer
Was this someone on your direct team? Were you not part of the spec-
review/sprint planning/etc meeting(s) where the project goals for the code
you're now reviewing was discussed? If someone just dumped a PR in your lap
with no context and said "do a code review," then this sounds like just a
really terrible process/implementation.

The idea, imho, is that you are supposed to be intimately familiar with all
the projects on your team, while only coding for your own. If you each follow
each other's tech doc/spec, and actually put real, professional effort into
good technical writing and understanding of your team members' communication
styles, you can all have a shared understanding of the problem and the
solution architecture/idea, and then after the code is written, code review
becomes a process of aligning on what the "right" solution is for the team,
given hyper specific context of the one PR/feature/project/bug fix/whatever.
Did it turn out like everyone expected? Why or why not? Does this align with
the maintainability goals of our team? What will ongoing support for this look
like? Does it move the needle for (any) clients? Does it unblock other
projects/tickets? etc...

I don't really agree with the type of code review where you nit pick about
adherence to arbitrary "style guides," read the code line by line to "try and
find bugs," or asking for changes because you think something could be
"cleaner" or whatever. The only code reviews that have real impact in my mind
are the ones focused on how the code maps to real business goals.

------
royosherove
For a team that has low code quality - here's how I like to start my code
review process. not everyone will like it, but I find it effective at growing
people's skills: [https://www.5whys.com/articles/step-4-start-doing-code-
revie...](https://www.5whys.com/articles/step-4-start-doing-code-reviews-
seriously.html)

------
nradov
If you're working in a language like Java or C++ which has good static
analysis tools available then the developer should be required to resolve all
static analysis issues _first_ before submitting code for review. That way the
human reviewers don't have to waste time manually marking simple issues.

------
pplonski86
Any idea how to do code review, when you are solo-founder?

~~~
arethuza
You could switch to working on some other component and then come back after a
few days - I find that length of time often makes me fairly critical of my own
code!

~~~
SharmaKushagra
Unrelated to the original topic but whenever I can, I'd try to work on 3-4
distinct work items in parallel in a sprint... typically doing a context
switch once in a day. I've found that it really helps in letting ideas and
solutions brew up and mature over time against trying to 'close' a single item
quickly. the context switch can be quite useful if you have hit a roadblock or
even just to break monotony.

