Live data from Hacker News

Design/code reviews in a startup

news.ycombinator.com

11–20 of 20 posts

Re: Design/code reviews in a startup

#11
Every formal code review I ever sat through was a painful experience that went on for longer than planned, covered way less code than planned, resulted in minor improvements at best and had zero long-term impact on the quality of the codebase.

Of the many reasons for this, I'll mention one: by the time a code review takes place, the key design decisions surrounding that part of the system have typically already been made. To do a retroactive redesign during a meeting is out of the question; that would be more work than writing the original code in the first place. So all people end up doing is scrutinizing tiny things like null checks. I'm not saying those don't matter; of course they do, but they're only a small part of making good code.

So the code-review approach boils down to saying "do the big stuff on your own and look over the small stuff together". That seems backwards to me.

(By the way, I'm talking about commercial projects. There's an entire literature around formal inspections, as you know, and good results have been said to be obtained with them, but if you want to go that route you're talking about heavy-duty process control, slowing development to a glacial pace that is unworkable for most commercial software.)

I've personally observed two things that do work. First, pair programming. It's not for everybody (the community here, for example, is largely derisive of it), and I'm not arguing for it in general. But there's no question that it does what code reviews are supposed to (but don't).

The other thing I've observed to work is to write the code individually but review it on checkin. That's how we do it on my current project. We'll either watch the repository and look over changes as they are committed, or email a patch and ask for review. This kind of review isn't as efficient or as comprehensive as writing the code together in the first place, but it does allow major problems to be caught and dealt with quickly (at the cost of some rework).

Re: Design/code reviews in a startup

#12
post #6

Good standards make code reviews trivial. Well defined, zero tolerance, guidelines virtually eliminate the need for code reviews. Variable naming, standard routines, syntax, how to do iteration, branching, common functions, etc., etc. etc., should all be outlined clearly, agreed upon, and adhered to. I prefer to look at it this way... Code Review: No one gets out of this room alive until this is right (whatever that…

Code reviews should be much more to with medium to big ideas, not small details. Small details should be trivial to a good hacker.

so: "You've got that wired in backwards", "Tim built one of those over here", "When we implement foo, that'll bar" & "That's a security hole because ...." are good.

Re: Design/code reviews in a startup

#14
What sort of problems are you trying to catch?

Niggling little bugs or major design flaws? Both?

Code reviews, as this brutal group process done after the code is committed, are wrong IMO. It's better to talk out the design before any code is written, or circulate a design note. (Not a full design document; the only good design docs are written after the code.)

As for catching the niggling little bugs, or generally improving your approach in the small, pair programming and check-in review work well.

Check-in review is simply a convention that someone else has to read your diff before it's checked into the repo. You can make that an informal or formal part of your check-in process. It works really well; not only does it catch mistakes but it puts the incentive on the reviewee to write readable code, so they won't have their colleague complaining they don't understand what's going on.

That may sound like a lot of work but it's not. Once everyone gets to the point where they know how to write readable code, the check-in review takes just a few minutes out of each developer's day.

Re: Design/code reviews in a startup

#16
Unfortunately I work for a big company, and our team has a ridiculous 150+ designers/developers - how anything gets done is beyond me.

Also unfortunately, it fell upon me to review as much of the code produced as possible for some time. It wasn't a horrible job, but it opened my eyes just how beginner some of these developers seemed, and to how useful code reviews can be.

I never attempted to organise tradition 'sit-down' reviews - it wouldn't have been a good use of peoples time (and 90% of the team are in india!) - I just checked out a particular module and scanned it for red flags, repeated code and attempted to build up an understanding of what it was supposed to do.

Doing that caught many design flaws and simplified many chunks of spaghetti code, not to mention abuse of the database (bad queries, unnecessary queries etc) etc, so I think it was a good use of time.

Catching design flaws at this post implementation review stage is not really productive however, as its too expensive to fix them.

My solution to that was to do white board sessions with the designers, then ask someone to write up the outcome in a couple of pages to be quickly reviewed by the other members at that meeting - doing that was essential, as the number of times miss-understandings were caught before a code editor was even opened was amazing!

So basically, I think that code reviews have their place, but my feeling is that a sit down meeting is generally overkill. I also reckon that they are much more important (perhaps exponentially so) in a large team with many potential mediocre programmers than in a startup with several much more competent hackers!

I have never tried pair programming, but I know a lot of people are fans of it - I can see how it would be useful in a big corporation when someone is implementing someone else's vision, but struggle to see how it would work in a small startup. In the startup, surely people would be trying things and molding the vision in ones head until something useful comes out? I certainly hope to give it a try someday to see how it goes ...

Re: Design/code reviews in a startup

#17
You could try a cool method of random inspection. Have a daemon that listens for submissions to your tree. With a random chance (you determine probability) have it email the contextual diff (maybe even highlighted full diff) and the changelist description to a random developer.

Re: Design/code reviews in a startup

#18
post #6

Good standards make code reviews trivial. Well defined, zero tolerance, guidelines virtually eliminate the need for code reviews. Variable naming, standard routines, syntax, how to do iteration, branching, common functions, etc., etc. etc., should all be outlined clearly, agreed upon, and adhered to. I prefer to look at it this way... Code Review: No one gets out of this room alive until this is right (whatever that…

Yeah, variable naming standards are nice for consistency and I take it "common functions" is a euphemism for "don't cut and paste," but do you really want to dictate 'how to do iteration' in a way that's more limiting than the implementation language's idioms?

Re: Design/code reviews in a startup

#19
I've been in your position with a team that's quadrupled (including international staff). In the team's I've worked in it tends to work on trust, committed alot of code and senior then it's probably not going to be reviewed. Still earning your stripes then a more detailed design and an over the shoulder before commit and/or a formal review post module completion.

I tend to think you have to trust your team so I've tended even for junior guys to operate in permissive mode (I've yet to revoke someone's commit privileges). Small teams have a great advantage in that they can communicate more efficiently and often deliver quicker. If your team is growing and you can't trust (or grow to trust) what devs are committing then you're going to have longer term problems. Code reviews to me are more about mentoring and growing new staff, not enforcing 'good' code. The first question is always how could you have done this better. Why did you do it this way? Smart teams self correct so getting people to think about these things themselves tend to mean they don't repeat mistakes.

What really works is automating your success conditions, especially dumb stuff like formatting and dumb bug checking. Use continuous integration, in our dev team check in will get it running through all the tests including a full ui regression every night so if it makes it through that then it's already reasonably good. Cruise control integrated with .net and java checkstyle findbugs etc, turning warnings on as errors in the build means simple dumb stuff just doesn't happen. If someone consistently breaks the build or is checking in broken stuff then a switch to review first works. Breaking a build in our teams sends an email to the 'list of shame' (all the devs on the team) so people tend to avoid breaking it. We also don't allow any build to be broken for more than 24 hours (usually In a distributed dev team, working remotely or getting close to release then I've found review board really good. http://www.review-board.org/ I found people commit to the continuous integration system, if it breaks they fix it and upload a diff or patch to review board. People get emailed and can mange their time more effectively, they pick the best time for reviews themselves. A 'ship it' from any of our senior team means it's done and hosed.

Post reply on HN