Comment by arcticbull | Hacker News Reader

Comment by arcticbull | Hacker News Reader

Our code review policy is to require a minimum of 2 people who did not contribute to your changeset review and approve your code prior to merging. We've been able to reduce the amount of time a review takes in four big ways:

(1) Automated style checker. By adding a pre-commit hook to validate code style in an automated way, the amount of noise in PRs went down dramatically.

(2) Don't do architecture review in PRs. If you're going to make architectural changes, do so in a lightweight design document and get approval there first.

(3) If it languishes, go and physically (or virtually) ask the specific people to take a look. This works best in my experience when you find someone else who's having trouble getting their code reviewed and offering a trade. This person is then likely to review your code quickly in the future if you do the same.

(4) Feature flags allow you to get incomplete but atomic and correct units of work merged to master without showing them to your end-user.

This way, code reviews are not about style (much), not about architecture (much) and simply about making sure you executed on the plan correctly. Turn-around times went from days to hours in many cases.

> (2) Don't do architecture review in PRs.

The issue I'm finding in reviews is often that I end up with a summary of "this problem can't be solved nicely on the current architecture of this module so there should have been a redesign first". Such a review is essentially saying "start over from the beginning" and it's a huge waste of time. Often you just don't respond with that, instead making a mental note to fix the technical debt, and you do a superficial review of what was actually written.

(In this scenario the developer is inexperinced or not familiar with the code base, so wouldn't see the architectural problem, and instead tries to implement the feature in anger on the current architecture).

One could require the lightweight design document approval first for all issues, but that would be a huge overhead. I have banged my head against this problem for years.

Light document is exactly how I do it. I do mostly backend and API design, and usually when it is up to me I flesh out a spec (one page, after discussion with the main client, being mobile devs, web devs, and so on) then have all stakeholders ok it. Surprisingly when its a one page prose everyone reads it pretty quickly. As long as the end result code matches that spec the other technical comments are not that hard to deal with and usually easily reversible should I merge then realize there was a mistake.

+1 to design docs and feature flags. We started to keep design docs in the repo, too and have the design review process work the same way (if slower) code review does. So far so good.

I think automated style checkers are great too. One problem I have with them is it fails the build, which has poor locality with the problem. Instead, I wanted the failures to actually comment on the line of code. That's why I wrote an automatic style checker to do that. You can run without having it baked into the company process (just from the CLI), which was important for me at the time. You can find it here: https://github.com/justinabrahms/imhotep

I also wrote a service version of it at https://betterdiff.com/. One relies on the other, so the CLI app will have the same feature set, just not automatic. Several people use the CLI app from jenkins or similar. :)

> (1) Automated style checker. By adding a pre-commit hook to validate code style in an automated way, the amount of noise in PRs went down dramatically.

Would you mind sharing your precommit hook? We have a simple one that prevents console.log etc., but nothing super fancy.

Here's mine

    grunt eslint
    npm test

I'm using eslint. Example: http://github.com/greggman/twgl.js/

http://pre-commit.com/ might help you out.

Reply on news.ycombinator.com