
We don’t have time for code reviews - javinpaul
http://blog.8thcolor.com/2013/10/we-dont-have-time-for-code-reviews/
======
edw519
Proper code review provides much more benefit than cost and is indispensible
for quality software. Improper code review is a waste of time (or worse).

Proper code review is done:

    
    
      - by another programmer
      - by someone with some knowledge of the application
      - by someone with some knowledge of the environment
      - against some code standard
      - against standard requirements (APIs, database, etc.)
      - with a checklist
      - uniformly (no matter who is doing it)
      - until it's right
    

Improper code review is done:

    
    
      - by a non-programmer
      - fully automated
      - as a "rubber stamp" on an approval checklist
      - against the standard du jour
      - according to the whims of the reviewer
      - with a deadline for the next step
      - as a replacement for testing

~~~
nradov
I also include junior developers who don't know much about the application or
environment in code reviews. It's a good way for them to learn the code base
and see what type of defects the senior developers point out.

~~~
martinvanaken
+1 to that one. Code Review should be Peer Review - it is not about a Senior
reviewing a Junior it is about a developer reviewing the work of another one.
The junior's questions may actually be as useful as advices (notably by
forcing you to explain your choices).

------
mabbo
I once pushed un-reviewed code that contained

    
    
      alert("Fuck");
    

It made it to a single user who was testing it for me, on the other side of
the planet. He spoke very little English, and for that I am grateful.

That was my last push without review.

~~~
benihana
I think it's hilarious (and typical) that the moral of the story is "I don't
push without code reviews" not "I don't use bad words in test code."

~~~
junto

       "I don't use bad words in test code."
    

This is actually _really_ important. We had this happen with a customer whilst
UAT testing. The project schedules were under pressure and as a result the
customer went nuts (rightly so) and we had to make some concessions to
customer as amends.

It isn't just curse / bad words. Equally as bad are comments like:

    
    
      "this was the customer's stupid idea not ours"
    

Also, if you really have to output test stuff in the browser use _console.log(
'message')_ and not _alert( 'message')_.

Then for production you can always do this at the top of your main JavaScript
include:

    
    
      var console = {};
      console.log = function(){};

~~~
xymostech
What about

    
    
      window.alert = function(){};

------
mberning
When talking about code reviews I am constantly reminded of the following

1\. How hard it is to grok code that I've never seen before

2\. How hard it is to grok my own code from 6 months ago

Sometimes it may take me a day or more to fully get my head around a new piece
of code I am working with. I could not give a thorough AND fair review of
another person's code without internalizing it to a satisfactory degree.

There is also the issue of software as art vs engineering. Quite often I will
take issue with the style or approach of another person's coding simply
because I would have done it differently had I done it myself. But that does
not mean that their code is wrong.

All things considered, I think code reviews can be helpful in certain
situations, but I think there are many pitfalls which must be avoided in order
for them to justify the cost.

~~~
jt2190

        > Sometimes it may take me a day or more to fully get 
        > my head around a new piece of code I am working with.
    

Are you trying to perform a design review, or a code review? Code reviews (in
my mind, at least) concern themselves with the low-level mechanics of the code
("Handle this exception properly","there's a library method for this
logic","follow the team's coding standards", etc.) whereas design reviews deal
with the "how is this thing supposed to work?". When reviewing code, I avoid
dealing with design issues. If I see design issues, I ask the developer for a
design review.

~~~
mberning
My time is more valuable than being a pre-compile/pre-deploy sanity check or
human lint.

When I review code I review all of it, not just the easy parts.

If I was going to do a 1/2 ass job of it I wouldn't do it in the first place.

~~~
jt2190

      > When I review code I review all of it...
    

We haven't mentioned timing, but that's the reason I decouple design and code
reviews.

Design reviews can be done fairly early in the process before the code is
complete. If design changes are needed, there is still plenty of time for
them.

Code reviews can't really be done until most of the code is written, but (as
you point out) the kinds of problems they find are much easier to fix, so
require less time.

------
jt2190
Personally I agree with the author that proper [1] code reviews are valuable,
however, I think he's skipping over a really critical point:

    
    
        > We started doing Unit Testing at some point, and it 
        > quickly became “good practice/mandatory” in our team (I 
        > think good practices need to be applied by everyone in a 
        > team, requiring some kind of “collective enforcement”).
    

I'd love it if the author would elaborate more on how to change team behavior
like this, especially when it's a team member (peer) trying to make the
change.

I've seen many teams struggle because there isn't a agreement over what
practices to follow.

[1] See edw's comment on proper code reivews:
[https://news.ycombinator.com/item?id=6598804](https://news.ycombinator.com/item?id=6598804)

~~~
hox
My advice:

1\. As an advocate for a practice, ensure you use that practice in all that
you do. Advocate for it in every team meeting. Try to stress _why_ it is
important, and how it can directly make things better, instead of just "making
our code better." Provide research and numbers from prior projects that show
how useful such a practice can be.

2\. Provide good examples and guides to get the practice started. Some avoid a
practice because they don't know the best way about executing a practice. If
it's unit testing, show how one should go about determining edge cases and
describe useful unit testing methodologies (mocks, stubs, etc).

3\. Make it easy as possible. If it's unit testing, integrate a CI server
directly to your SCM. If it's code review, adopt a practice that fits inline
with your existing workflow and don't make it a ceremony - make it
asynchronous.

4\. Understand that it will be gradual, but every effort moving forward should
push towards integrating the new practice. You can't suddenly have 100% test
coverage after introducing unit testing to an existing project, but all code
that gets included can include new tests, and old refactoring can have tests
included to slowly build up the test coverage.

~~~
puppybits
Why is much more important than what to do. The best benefits of code reviews
are: 1\. Developer educate each other about the code base and how to be better
developers. This means quick, more stable features which means the company can
start making money sooner. 2\. Slows tech debit. Poorly written code stays out
of the product. More up time means more overt unity to make money.

------
donretag
My co-workers write awful code, including the "architect". I try to review
their code, without having a formal process, because I simply do not trust
their abilities. It would not be pretty if we did code reviews.

~~~
scrabble
Or it might be better. Once code reviews are happening people tend to start
realizing that people are reading their code and it is raised to a higher
standard.

There's also the added benefit that reviewing your code can teach them about
better programming practices, and also that reviewing their code with them and
asking questions about their thought patterns can make them better developers.

It's my opinion that if the code is awful then code reviews become especially
helpful and important to bringing everyone up.

------
paulbjensen
You think that's bad, I once worked in a startup where the Product Manager
told me he didn't have time for tests.

~~~
Joeboy
You speak as if that's unusual...

If it's not something you can stick on an invoice, it's generally hard to
persuade management it's worth doing in my experience.

------
tonyplee
I care more about full functional test coverage than code review. The tests
must be automated, regression and functional coverage not just unit test. Test
results/logs must output in HTML store in DB backend (now) or excel (did that
in 1997) for ease of analysis. Test failures are highlight in red, passes are
in green.

The developers should write new test code for every new feature and run all
the existing test code after/merge before checkin. All the projects I worked
on that implement this process are very successful. With this process in place
and agree upon, I careless about code review.

With enough "functional" test coverage, it is easy to do massive refactoring
of code without worry about any breaking.

Worked on one project that try code review for two weeks - other than a few
comments about coding style, not much gain from the time spent.

~~~
DougWebb
Test coverage is pretty orthogonal to code review. You might wind up with a
large amount of fully tested freshly refactored code that does not meet
requirements or edge cases the tests missed. I'd recommend at least reviewing
the test code in your case.

Besides that, it sounds like a nice codebase to work in. I've had very good
test coverage for libraries and backend services with well-defined APIs I've
written, but a lot of my career has been spent doing UI work and I've never
found an adequate testing tool that can deal with both the complexity and
randomness of human interaction and the rate of change of UI presentation.

You can't really regression test UIs, because most UI development
intentionally changes the things regression tests look for, so you wind up
spending a lot of time updating the regression tests and trying to keep up
with UI changes that are being made in a tight code/review/tweak iteration
loop.

------
clubhi
Pull requests are insufficient for code review. You need to run the code. I
often find the code I want to review after following a path in a debugger.

~~~
GrinningFool
I think I have to disagree with this one.

A major part of being an effective programmer is the ability to maintain state
and call stacks in your head.

Stepping through code is great - as a last resort. But I've found that if I
_need_ to run through it with a debugger in order to understand it, it is
often overly complex.

As with any generalized rule there are exceptions, but I consider needing to
step through code in a debugger to understand its behavior as that exception,
not the inverse.

------
alphadevx
Agree with this: pull requests _are_ code reviews. Web UIs with visual diffs
make this even more efficient.

~~~
eloisant
I don't know, code review without running the code is a very very light
variant, too light to call it code review I believe.

~~~
masklinn
Code review is about reviewing the code. That's literally what the name says.
CI runs the tests, QA test the final behavior, code review checks the code.

~~~
potatolicious
That's nice in theory, but in practice another dev really should be running
the code and doing some manual testing. Having full knowledge of the innards
allows a competent dev to foresee potential trouble spots in the architecture
and hit some edge cases to make sure they don't break.

CI may run the tests, but that assumes your tests have captured every possible
edge case. It may be the intention of tests to be comprehensive and cover
everything under the sun, but that is rarely the case in reality.

Ditto QA - they should be able to black-box test the software and all of its
possible states, but in reality something is going to pass through the net.

Having a dev run the code themselves and poke around in it is just a plain
good idea.

~~~
dragonwriter
> That's nice in theory, but in practice another dev really should be running
> the code and doing some manual testing.

Why? If they are reviewing the code _including the unit tests_ , shouldn't
they instead be _suggesting any missing unit tests_ , which then become
_permanent and reusable_ rather than "doing some manual testing"?

> CI may run the tests, but that assumes your tests have captured every
> possible edge case. It may be the intention of tests to be comprehensive and
> cover everything under the sun, but that is rarely the case in reality.

Insofar as the other dev doing code review can address this with their own
testing, isn't it better for this to be done-once and preserved by adding
_automated_ tests rather than done-and-lost by doing manual tests?

------
blueblob
linkbait. Title is no time for code reviews and talks about how they do code
reviews.

------
cnlwsu
gets worse when you hear "we don't have time for unit tests"

