
Introducing code reviews to a team – Part 1 - vincpa
http://www.depthinfinity.com/2016/03/introducing-code-reviews-to-a-team-part-1/
======
pambeesly8
I was really hoping for more advice on letting the team members appreciate
code reviews, and not take it too personally.

We've just recently introduced code reviews on our 3-4 person team. We're
working in a particular environment, and most people have no production level
experience in that environment. These people have gotten used to _just getting
it to work_. We've clearly stated that we will be going with Google's style
guide, but we can always divert, as long as everyone is in agreement. When
pointing out that the changes are not according to the style agreed (frankly,
it's not so much about style, as it is about using patterns from other
programming languages, that are not applicable), the response is:

\- it works

\- I don't think you understood what this does (note: no comments or attempt
to explain)

\- In the future, please do not waste more time with pull requests. Desktop
sharing and a call is the most efficient way to work in remote teams.

It's getting more and more hard to keep an open mind and not take things
personally. We're at a point now, where there's a intermediary person
refactoring the code.

Does anyone have any practical advice on people understand the importance of
structure code, and to limit the _personal factor_?

~~~
fourneau
As for people taking things too personally (you or otherwise): talk to your
team members. Let them know that you perceive their comments as harsh. If you
can't come to a consensus about tone, content and voice, then code reviews
will always be a friction point between you and your team. Work it out with
them, they're all just people with emotions too, after all.

As for the appreciation of code reviews: developers absolutely hate doing
things without reason. For what reason does your team do code reviews in the
first place? Is that reason unanimously understood? If not, then you'll face a
lot of resistance.

In short, get everyone on the same page as to /why/ you want to use this tool
(code reviews) and on the same page as to /how/ to use it.

\--

also general code review advice:

\- Don't make it all about style, and if you do, preface that you're
nitpicking. automate everything you can when it comes to style.

\- Use unambiguous questions, with detail. Set a high bar of quality for
reviews by leading the charge yourself and holding others to that standard.
Don't be afraid to preface questions with: "I don't think I understand how
this works."

\- Don't be afraid to applaud really cool things as well. Code reviews don't
have to be negative comments only.

~~~
rak00n
These are good advice.

In a lot of companies you cannot submit a code without an approval from the
reviewer. That puts the reviewer in a high ground when it comes to asking for
explanation.

------
marktangotango
I've experienced some really toxic cultures around code reviews. I believe the
bottom line is teams need to set standards; linters and findbugs should be
incoporated in the build and fail the build if formatting standards aren't
met. Standards for level of abstraction should at least be discussed. Is using
exceptions for control flow ever allowed? Requirements should not be defined
in code reviews. I go so far to say that code beautification comments should
be handled in separate tasks.

Above all code improvement requirements and comments should be based on
industry defined best practices, like citing SOLID principals for example. Or,
citing agreed upon team standard for appropriate level of abstraction. In
short, there has to be a limit to the pissing matches.

I spent six months in a Big Dumb corp that did none of the above. Every team
member reviewed everything and every comment had equal weight. Code review
participation was part of the performance review process. Sounded nice in
theory, but incentived a truly toxic environment of "my solution is more
clever" and "my solution is better use of abstraction". Which resulted in a
spaghetti mess of over engineered code that still had bugs, and still didn't
meet requirements, even after 100's of hour of reviews.

~~~
KKKKkkkk1
Your last paragraph resonates. Having some experience with this process, I'm
convinced that a few years from now, mandatory code reviews will be the silver
bullet of yesteryear. Ultimately, it's just another attempt at solving the
unsolvable problem of "I hired lazy shitty engineers and I need a way to
quality-proof their code." If you don't have this problem, don't bother with
code reviews.

~~~
AnimalMuppet
No, still do reviews. Reviews catch bugs and problems - not all of them, but
more than zero. They're worth doing.

They also do a bit of cross-training, which is of some value.

------
wolfspider
These experiences mirror my own in an uncanny way (recently made team lead,
3-4 devs, just started code reviews) and found all of this information
extremely useful. I would add that discussing how much mileage can be
estimated on the current stack is important as well. Good code can still be
dangerous if someone in the team knows or feels they are only going to get 3
months out of it before dependencies will change and will need to update it
again. I've found discussing sustainability from time to time helps to figure
out who is checking in their code just to make it to the next day and who has
an eye towards creating a low maintenance product. Life pressures can cause
the best devs to live one day at a time they just need the proper forum to
express it to the whole team. Sometimes it just simply gets forgotten or lost
in the shuffle- the fact that library or API "X" will no longer be available.

------
chewyfruitloop
I've been code reviewing everything for my entire team for about 6 weeks. The
idea being that we have some kind of quality control on the stuff being
developed. We're using TFS to ask for reviews, which actually works really
well. ...what doesn't work is the junk I keep being sent....I bounced the same
review 3 times yesterday because it didn't even compile... When the stuff you
get is decent reviews are ok....when it's junk, it's soul destroying

~~~
AnimalMuppet
In our environment, you're supposed to have compiled it _and run the tests_
before you even bother asking for a code review.

~~~
dllthomas
That's typically the case for us, although there's an occasional "before I
finish it, what do you think of this direction?". Not a problem when rare and
called out special.

------
nsfyn55
>when it was time to review code, what we really looked out for were obvious
bugs, problems with the design or architecture and where code should be
located. Variable naming was inconsistent but no one really bothered to
enforce them. I don’t blame them, making code review a nit-picky task really
puts a damper on the mood.

As a veteran of a dozen different code review approaches I've identified a few
properties of those that succeed and those that fail.

If you want your code review process to fail its easy just...

1\. Make it about you and your preferences. Does this code adhere to my
particular aesthetic preferences? Are the variables named the way I would name
them? Do I consider this code readable? How can I force others to adopt my
perspective? Remember the purpose of code reviews it to keep iron-fisted
control of the code base such that it never becomes that dreaded "big ball of
mud"

2\. Demand that the team adhere to a set of rigid rules regardless of how
practical their application is.

3\. Most important focus mainly on the subjective qualities of the
code(format, spacing, naming, etc.) Allow non-critical path items to hold up
delivery and use those items to critique others based on an arbitrary
measuring stick.

If you want your code review process to succeed. Its a little harder....

1\. Be problem focused. What has bitten you in the past? Are you sure you
understand the cause? Acknowledge that there is no "right way" and that you
will build the perfect code review model through trial and error.

2\. Start with the bare minimum and build on it with a regular retrospective
process. Allow your team(s) to take ownership of the code review process both
as an expression of what they want to accomplish and as a way to improve their
daily lives.

3\. Accept that you might have been the one doing it wrong all this time.

4\. Focus on the objective qualities in the code. Is this the best approach to
solving this problem? Does it work? Are there any obvious bugs? Based on our
collective experience will this code cause problems later?

5\. If you feel like you are nit picking then you are nit picking. Don't nit
pick.

Code reviews can either be a tool to write better code or a form of weaponized
OCD. Don't be the latter OP, you're better than that.

~~~
fourneau
"Weaponized OCD" is my new favourite way of describing bad code reviews.

------
forrestbrazeal
What do you all think about the practice of pushing changes to an open pull
request based on code review feedback? My org has not established a consistent
policy around this and often people will push changes after some reviewers
have already approved the request, which I find concerning. On the other hand,
it is nice to see review comments being addressed in the same branch/PR.
Thoughts?

~~~
ccoggins
We use Bitbucket at work and were able to configure it such that when this
happens Bitbucket automatically unapproves the pull request. This seems to
work pretty well and allows code reviews changes to be seamless, e.g. the
extra commits can be squashed. I would think github/gitlab etc probably have
similar features.

------
sriram_iyengar
Good start and my best wishes. We follow these on stash using fork & pull
request model, for a large project with 10 teams, each around 6-8 devs. \-
everyone has a fork \- no one commits outside their fork \- each module has
3-4 reviewers \- good practices are constantly shared via sessions \- wiki
drafted and updated \- comments are given reasoning with concepts, discussion
threads and as well to other modules \- object calisthenics, if required

