We don’t have time for code reviews

Are code reviews by non-programmers actually common? How can that possibly work?

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.

Hi, OP here. Interesting list. I think I would approve most of your points, but never worked (or created) such a "proper" environments in my various teams. Could you share the kind of checklist you use? Or give an idea of the kind of "checks" you have there?

Thanks!

This one should be recorded on http://www.codingconfessional.com/

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."

My comments are more along the lines of alert("GOT HERE 1"); alert("GOT HERE 2"); alert("GOT HERE 3");

Not very helpful if someone else sees it, but also not profanity.

As I said in another thread, always 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(){};

To avoid this sort of thing, I have an editor macro that puts a comment like this at the end of the current line:

  alert("Fuck");  // DEBUG FIXME(dlitz)  

You can't see it here, but the comment also includes a few trailing spaces.

This way:

- "FIXME" gets gets highlighted in my text editor

- the whitespace triggers a git warning when committing

- I can configure a pre-push hook that refuses to push commits to origin that contain "DEBUG FIXME"

- I ever do accidentally push it, it's clear that the code was just there for debugging, and that I'm the person to talk to about it.

I also use "git add -p" to do a hunk-by-hunk review of my changes before committing them.

None of this is very hard or time-consuming, and I can't remember the last time that I accidentally pushed debugging code since I started doing this.

I've found code reviews are beneficial to help you catch simple bugs, you don't always have to internalize the purpose of the code just make sure the person dotted their i's, crossed their t's and freed all their memory pointers... Or just help junior guys write more efficient code...

The cost you speak of is directly trading software quality for "speedier" delivery. Heck, this was even posted just yesterday: http://www.sec.gov/litigation/admin/2013/34-70694.pdf. One of the big problems was there was no code review being performed.

Also think of code review as a fantastic learning opportunity for the developers you are reviewing. Practicing an art can only get you so far as an expert, but being critiqued by others can help refine the rough edges you may otherwise ignore throughout your career.

Code reviews don't have to happen in massive context, either; why not follow a practice that every pull request (or whatever your SCM calls it) goes through at least one reviewer before being merged to master? And ensure your pull requests are frequent and preferably 100 lines or less!

A little bit goes a REALLY long way. Don't give in to the fallacy that your code isn't either worth it or is good enough to not be reviewed.

I agree with you, and this is the reason why having small features and reviews for all of them helps. Groking a colleague's code is much simple if it is a small piece, and when you are reviewing his code regularly (and the other way around).

Our typical PR is 1-2 days of work, so I'm reviewing several by week, something by days.

You are of course right that it should not fall into "you should do it my way". Now, when my colleague said "I would have done it differently", I always ask how and why. I will probably not change my code if it is good (or even good enough), but I would have learned something, or got another point of view.

    > 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.

There's also a few benefits of code review that you missed:

1. Reduces bus factor.

2. Increase readability.

Coding in a company is a communication problem. How you solve a problem may be different than someone else. Outside of simply catching more mistakes, it's possible that you may learn something new or the other person can improve their code.

Ideally there should be a team / company coding style simply to ease communication overhead and reduce bikeshedding.

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.

Hi, author here. I agree this may not be easy - but something can always be done. I've been in several situation.

As a team leader facing junior developers, I did simply set-up rules. I explained them, but I had the power to enforce them by myself. I coded with them, and they started coding like me - until they were confident enough to challenge me. I apologize to the "autonomous self organizing teams" evangelists, but in some situations, giving some direction may be the most efficient way to progress.

As a peer, you just need to find one other person in your team that is willing to play along. Although I understand the value in explaining something, doing it is for me much more convincing. If you really think something is a good practice, don't try convincing me if you are not already applying it yourself.

If you are in an Agile/sprint oriented team, just propose to test it for one sprint, then it will pass the retrospective test or it will not - no one should object to a one sprint experience, especially in an agile team. Do the same with your colleagues ideas, even if you find them silly. It shows goodwill, and you can be surprised at some time.

This is actually the way me arrived to our current workflow at 8th color (http://blog.8thcolor.com/2013/09/how-our-own-workflow-is-dri...) - successive retrospectives.

Finally I would not involve non coding management in the discussion if possible, as it will quickly devolve into "what would it cost". Better to handle this inside the technical team.

Hope it helps, and remember, it's the first step that cost. Find one willing colleague and start!

Martin

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.

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.

Its the whole point for me: even if I prefer managers that I can convince, I just call that "development". Replace: "feature is done but need to be tested or reviewed" by "feature is not done".

The fall in the technical/developer responsibility anyway for me.

Not just start-ups; larger companies, too. The process may dictate testing, but if, for example, there's an urgent production bug or some enhancement that "needs" to be pushed quickly, testing often falls by the wayside.

Tests==code. If your test code is not reviewed then you don't know if your tests are any good. Neither code reviews nor TDD are silver bullets.

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.

Code review, done properly, should provide more than just a critique of coding style. It should should also catch certain types of bugs before they get to functional testing. Yes, you can catch those same bugs in functional tests, but in the environments where I've worked, there's a lot more overhead (read: time, money) involved with bugs found in functional test than in code review. Ita can be pretty frustrating to be that far along (functional test) and find a bug that could've easily been found in code review.

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.

Yes the diffs often don't show enough context and the errors may occur outside of the diff so you should really review the whole changed function even if you don't need the debugger.

They are always attached to a branch though so you can check it out to review (and run/test as appropriate).

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.