
How to be a faster code reviewer - jaimefjorge
http://blog.codacy.com/top-10-faster-code-reviews/
======
quaunaut
As a less experienced coder reviewing coders with half-decades or more
experience on me, I always had a difficult time knowing whether I was looking
at something that I just didn't understand the purpose of, or if I should
question their wisdom.

For awhile, this resulted in me effectively checking to be sure it looked
'grammatically correct'\- as in, followed style guides, was linted right, etc.
The bullshit stuff.

Then a few got through where I had reviewed them, wrote down "What's this
for?" as a thing to review a month or so later, and they turned out to be
_huge_ problems- sometimes performance, sometimes functionality that would
break under the right circumstances.

Have others gone through this, and if so, how would you recommend 'teaching' a
newer coder how to do quality code reviews of the elder coders around him?

~~~
Patrick_Devine
You can ask the senior engineers, but nothing beats rolling up your sleeves,
diving in and peeling the onion back. If you have a good manager, you need to
tell them you need more time to absorb and understand the code base you're
working on. They should be more than willing to accommodate, particularly for
a more a coder with less experience.

My biggest pet peeve when I send code out for review is someone responding
with the superficial bullshit stuff, so it's great that you identified it as a
problem. You do tend to get reviews like that from junior folks, but I also
see it with really insecure engineers. People start using code reviews as a
weapon to take pot shots at each other.

I also think it becomes a problem when you're doing these smaller code reviews
as the blog post is recommending; if you're doing bite sized chunks of code,
it's really easy to lose context and overlook things which might be
performance bottle necks.

~~~
matwood
_My biggest pet peeve when I send code out for review is someone responding
with the superficial bullshit stuff_

Send something out with a fairly complex algo and you get back people
complaining about a brace being 2 spaces instead of 4 or something else that
will be automatically cleaned up by your editor before commit.

~~~
jakub_g
You should have an automatic code formatter / checker if possible. Plus code
linter. Automate everything if possible and make build fail in case of
discrepancies. Then there'll be no discussions like that.

------
deathanatos
The first point isn't something you can do: that's something you others have
to do for you. Unless you count convincing people to change their workflow,
which I've found to be near equivalent to moving mountains.

In my experience, it seems to boil down to not being effective with their VCS
(or in some cases, the VCS not being an effective tool). But it's hard to
convince people to go through the trouble of learning something: the short-
term cost is just too much, and they seem to be blind to the long term gains.
It's like convincing someone to use vim (who isn't an emacs or equivalently-
powerful-editor user).

------
sunir
A related question. I'm wondering what other people do to ensure quality of
outsourced code. I'm intrigued at Codacy. A lint in the cloud could solve some
of my problems, but probably not all.

I'm in the position of occasionally outsourcing non-critical development but
not having time myself (I used to be technical; now focused on marketing) or
able to distract the dev team to look at code in depth.

Do you outsource development? How often do you code review that code? Never,
once in a while, on delivery, frequently? Do you do anything else to ensure
outsourced code quality?

~~~
jaimefjorge
Hi Sunir. Great questions. Hope to get more people answering this.

Please also take a look at a parallel discussion in proggit:
[http://www.reddit.com/r/programming/comments/1zqbx1/10_ways_...](http://www.reddit.com/r/programming/comments/1zqbx1/10_ways_to_be_a_faster_code_reviewer/cfvymys)

From my experience in Codacy, some of our users use it to grade and quickly
send out comments from the tool to the outsourced team. They like being able
to review the code as it's being done (since it's per commit) with the help of
our linting code patterns.

We also give out grades which is an overall vision of the project without
having to go into much technical detail. "Why is this a C?" is a great
question to ask :) I'd love to hear more about your experience and your pains
and check if we can be of assistance. Here's my email: jaime at codacy dot
com.

------
voidlogic
They missed the most important one, practice. It might sound like I am being
snarky or stating the obvious, but like any new task at first you will be slow
at code review and it will take up "too much time". Its a skill, in some
situations, its not really too much code to review once you have gotten good
at reading code. Everyone can do with being better at reading code too-
traditionally people have been better at writing their own new code realtive
to their ability to reading and grok their teammates code- and that doesn't
scale well.

------
taude
Be sure to add tasks to the sprint for the code review to happen. If it's not
an actual sprint item, I find it likely won't get done effectively. If you
have a couple hours, it's a great time to train the developers on some of the
libraries, code standards, practices, too.

~~~
einhverfr
> Be sure to add tasks to the sprint for the code review to happen.

Just make sure it is a sprint and not a sprintf.

------
Kiro
I don't understand the argument of keeping your commits small. In my team we
do code review after a task is done, not for every commit so the size of the
commits won't affect the review.

~~~
jakub_g
A commit should be like UNIX tools: should do one thing and do it well. If the
commit is adding hell lots of new features, fixing bugs, and doing heavy
refactoring all at once, it's way too much.

When developing say a new widget (or heavily extending an existing one), you
can develop basic functionality, commit, add bells and whistles, commit, and
some more stuff, commit, send for code review, do the fixes, commit, ask for
re-review maybe, and then squash the commits into one commit if needed. With
DVCSes like Git it's extremely easy, and reviewing smaller commits is easier
than having to go through a behemoth (of course it always depends on your
particular situation).

I sometimes tend to open half-baked pull requests to ask for quick early
feedback if I'm doing things the right way, and what to be careful about, and
continue developing in background while the more senior person takes a look at
the code.

~~~
pdovy
Does that really work well in practice, though? I find that I might get the
base functionality down to a workable state, but then as you add the bells and
whistles you're likely to find and fix your own bugs, realize you need to
refactor, etc.

You can make all those changes in distinct, clean commits, but if the reviewer
works commit by commit, they have to follow the evolution of your code rather
than simply review the final state. You might argue that provides a basis for
a better review, but it's also much more time consuming for the reviewer.

~~~
jakub_g
When you find an issue with previous commit while adding new things, you can
make a temporary commit, fix the issue, commit once again and reorder commits,
then meld the fix into the previous one. I'm pretty comfortable with doing
things like that but some guys not that much. You have to like this workflow
and work from the start like this.

You can of course split a huge commit into smaller ones using sth like `git
add -p` but it's way more work than just doing lots of small commits regularly
and then squashing them into medium-sized commits before the review. I like to
do temporary commits regularly as "checkpoints", meaning, "this code works",
and anytime I make a stupid mistake and break something, I can analyze a short
diff to find the bug I just introduced rather than trying to figure out the
bug just by looking at the code. It's usually a lot faster for me.

------
subb
The link to codacy at the bottom doesn't work.

~~~
jaimefjorge
Thanks for heads up. Updated!

