
Ask HN: Code review vs. Unit test - cryptozeus
Hi,<p>Within our company there is an internal debat going on about what process should we focus on first to improve code quality. Unit testing frameworks or code review tools ?<p>IMO code reviews has become just a formality where developers just check for syntactical issues instead of overall design. This is mainly due to tight deadline where everyone is trying to finish their work.<p>Has anyone tried putting this process in large team ? What are the before and after effcts ?
======
jwilliams
In reality, you need both. However, unit tests are code. If you're writing
poor quality code, then you risk writing poor quality unit tests. Brittle,
overly specific unit tests can be a major burden. Even just mediocre unit
tests can weigh down a project.

Code reviews have an outsized impact in the organizations I've worked in. More
people understand more of the code. Quality goes up not only because the bad
code is spotted, but also the reviewers can learn good practices too. You also
get better at reading code. This is a fundamental, critical skill.

For this reason, I'm a fan of having developers of all skill levels be
reviewers. However, code reviews are a skill and take an investment for people
to learn how to be effective.

So if you're really, really having to pick, do code reviews. Use the code
reviews to drive sensible, smart adoption of unit testing.

------
bzalasky
Honestly, you need both. Here's a comment on code review I wrote year ago or
so that seems relevant:

Code reviews shouldn't dwell on style issues that can be handled by linting.
To the extent that you can automate some of the review process, and prevent
sloppy commits from being merged in by having them fail CI, you'll eliminate
part of the problem. Another thing that helps is reducing the semantic
distance between what a commit message says a commit does, and what it
actually does. This is pretty easy to enforce relative to more subjective
aspects of coding. The last thing that can make a big difference is not
getting hung up on wanting to rewrite a commit as you would have written it,
but focusing on improving understanding and clarity for whoever will inherit
the code next. If there are serious issues with the architecture and patterns
the code is using, CR can be too late in the process to address them
effectively (depends on the scale involved). I'd try to collaborate with other
engineers earlier in the process to help set them on the right path [1].

As for unit tests, make sure to isolate the unit (generally a class or
function), and test its interface. Tests that are coupled to the
implementation aren't as useful. Investing in writing unit tests now will
increase your velocity in the long run. You'll be able to take on more
technical debt in the short term with better terms, so to speak, since you'll
have a robust test suite to refactor against. Obviously, this doesn't mean you
should write bad code, but this will give you the option of considering
performance and reusability tradeoffs if you need to hustle a feature out the
door.

[1]:
[https://news.ycombinator.com/item?id=11257835](https://news.ycombinator.com/item?id=11257835)

------
hellwd
I have tried a lot of times to enforce the code reviews because I have seen
lot of bullshit being checked in to the source control but if you don't have
support from other team members then even having the code review is waste of
time. Code review should come from the gut feeling, you should be always self
critical and let your code being evaluated by other people who care about
software development in general and not by the people who just want to make
their job done and to have something to mention during the stand up meeting.
So depending on what kind of people you are surrounded with, you should decide
is it worth to do it or not.

Code review is not a task or ticket, it should be more natural in all teams.
Like it's normal to take a shower or brush your teeth, it's also normal to do
code reviews and keep the code clean and good. But recently most of the teams
are just skipping everything to make managers and owners happy. And they are
always happy with beta, preview or even prototype - so sad.

Unit tests are something else. Good, clean and testable code starts from good
and clean requirements that are easily understandable. From there you are able
to write clear unit tests and imagine the whole story. If your manager or
someone who is defining what needs to be done doesn't have the clear vision or
plan then you have a good chance that your code will be bad - then unit tests
are implicitly bad.

------
psyc
My intuition based on 20 years of working on large-ish projects, is that code
reviews are always helpful, and mandatory unit tests waste a lot of time and
effort, at least for statically typed code. I don't have much experience with
dynamic languages. This doesn't mean don't write any unit tests, but to use
discretion instead of enforcing coverage. So, it's perfectly reasonable for a
code reviewer to say, "a unit test could be really helpful here."

------
jamestimmins
Sounds like it's worth taking a step back and looking at your processes from a
high level. If there's pressure to push code more quickly than is possible
while maintaining effective code reviews, then you may want to consider where
that pressure is coming from. If velocity is causing quality control issues,
then velocity is likely too fast. Not to mention that limiting quality checks
will reduce velocity in the long term.

Once you have both product and engineering (or whatever forces are at play) on
the same page about reasonable expectations, I'd recommend using both. Code
review and automated testing both serve distinct yet important roles.

~~~
tdbgamer
Yeah, if code reviews are just syntax checks, then they're doing it wrong and
no tool will ever fix their problem. However if they have more junior devs and
want to prevent major design mistakes before they happen, pair programming is
a thing.

------
toast0
If you're in a tight deadline environment, I'm guessing people aren't going to
spend quality time on unit testing or code reviews. It's quick and easy to
write terrible unit tests that are counter productive; it's also pretty easy
to just stamp all the code reviews without taking a real look.

Not every piece of code needs to be high quality. Instead, you should
indentify the parts of your system that need higher quality and spend real
time reviewing code and writing appropriate tests (not just unit tests, also
do performance tests and integration tests of key components). Especially in
an environment with tight deadline and changing requirements, focusing on key
areas will save a lot of effort.

------
wolco
I think unit testing and automated tests are the way to go with pull requests
that require two developers approval.

Two developer number is important on the reviews to keep everyone honest and
to keep everyone interested.

------
shados
I'd spend time trying to fix the environment where deadlines are so silly that
you can't do proper code reviews.

------
quickthrower2
If you have to pick one, pick code reviews. Without them you really can end up
with shitty code and not know about it for months or years and have it bite
you. Unit tests without code reviews can be useless if they are testing the
wrong things or not deep enough.

------
yen223
We do both. Unit tests can help with the review process, by making it clear
the intention behind certain aspects of code changes.

Code reviews and unit tests are only as effective as the developers who
participate though, can't escape that.

------
jbreckmckye
Code reviews come first, because you can't write good unit tests without them.
And poor tests can make your life horrible.

------
llccbb
What are the actual problems that your company is encountering? Bugs?
Spaghetti? Unmaintainability?

------
twobyfour
Code reviews are not a good substitute for unit tests; and it sounds like what
you're describing your team as doing is more like linting, which you should
use an automated tool for.

The purpose of unit tests is to catch regressions. They can also be an
excellent tool to speed development in real time by using automated tests to
confirm the code does what you want it to, instead of repeated click-through
testing, which is often slower.

A habit of unit testing _can_ (but doesn't always) encourage developers to
adjust their mindset about writing code and to get into better habits. For
instance, feature-oriented spaghetti code is hard to test. API-oriented code
is easier to test, easier to maintain, and easier to extend. Developers
thinking ahead about testing, or writing tests and application code in
parallel, are also more likely to use patterns like dependency injection.

Code review is a terrible way to catch bugs, but it can be an excellent way to
share knowledge. Both knowledge of the craft, shared from seniors to juniors;
and domain knowledge / institutional memory of business practices, ins and
outs of existing code, and the like. It's also a great opportunity to review
design decisions and teach best practices in that regard.

You mention code review failing due to tight deadlines and everyone "just
trying to finish their work". I see two potential issues here.

The first is the obvious one that it sounds like your team is a little
overworked/understaffed. There are all sorts of reasons (that I'm not going to
go into here) why it's bad for a team to have zero slack time. But not having
time to step back and assess the outcome of the work is - as you've discovered
- a major one.

The second issue is that it sounds as if your team might be a little short on
cohesion. One way this might manifest is that they don't trust one another to
give or receive feedback in an empathetic way, so they're afraid of requesting
or providing in-depth feedback. Another is that they don't feel a personal
obligation to one another to make time to provide the sort of in-depth
feedback that anyone should desire if they want to continue improving at their
craft (and which is one of the primary benefits of working on a team instead
of solo). A useful code review on a 1000-line changeset doesn't have to take
more than 15 minutes.

I don't know exactly what sorts of code quality issues your organization has
been encountering, but I can add an anecdote.

My team has introduced both code review and unit testing in the past 18
months.

Code review was adopted hesitantly at first, but then with enthusiasm. I
wouldn't say it's had much impact on typical code quality or caught many bugs,
but it has improved communication and caught a few architectural mistakes that
might have been painful to deal with if they'd made it to production.

Unit testing didn't really catch on until we implemented continuous
integration to make sure the tests ran - and passed - after every change. In
fact, getting the first green build took a week of fixing tests that hadn't
been run or updated in months. Since then we've also added a Slack bot to
shame anyone who breaks the trunk build, and our test suite has expanded from
roughly 25 tests to about 600. And our tests have uncovered or prevented more
regressions than I can count.

Some team members have taken to unit testing with more enthusiasm than others,
despite a directive that all new development must have test coverage. One team
member uses it proactively, even to reproduce bugs before fixing them; another
uses it only for green-field development; another I have to pull teeth to
convince to add tests (even though he concedes that they're enormously useful
for catching regressions and verifying that dependency upgrades are safe, and
even though his design decisions have leveled multiple levels up since we
started testing); another built an entire back-end feature by running his code
only in the test suite because he didn't have a UI to run it in and the setup
was too complex to run comfortably in the REPL.

