[buro9]

I'd still rather earlier code reviews... design reviews about 1/3 of the way into writing the code. Enough time to have passed to have discovered the dragons in the whiteboard design, but not enough to have written code that could only undergo minor fixes in a code review.

I prefer the idea of a code review happening at a time that it could still steer the ship... too many code reviews catch bugs, but don't correct poor system design. That's also where the greatest benefit/education exists for the author... not minor changes, but "what about approaching it like this".

[eric_h]

I work on a very small team (4 engineers), so we have all commits for all branches posted to our engineering chatroom.

Everyone knows what everyone else is working on (and we all work in the same room), so when someone is on a task that we know my have some snags/difficult-to-design solutions, we'll all periodically take a break and look at decisions other people are making on their particular feature, providing feedback when appropriate.

Of course, since we all work in the same room, we can also use the WTF/minute metric to see (well, hear) when one of us is deep in the weeds and could use a second pair of eyes to pull us out.

[EDIT: and I know this won't scale too far beyond a team of our size, but for us it works quite well right now]

[dmarg]

I work on a team with the same size as yours. Although we do not all sit in the same room, we still do code reviews. Since we are small we are not the most critical on style.

I am a big believer in code reviews especially starting early with a small team. This would set the culture from the beginning because it is harder to bring that in later.

[machinshin_]

imo, when it comes to style, if your language ecosystem has one, you should use it to enforce one. ideally as a git pre-commit hook.

[wahnfrieden]

One way to enforce this is to use source control for design docs, so that you can use the same pull request workflow as you do with code. We've done this by having user stories written in gherkin, so that new user stories can pass review, and then later can be implemented as tests. This can probably be extended to other types of design documentation.

[fishnchips]

This is the role of design documents which get heavily reviewed by the team before even the first line of code is written.

[gpderetta]

In my experience, no design document, no matter how carefully drafted survives contact with the enemy^W^W an IDE. At best you can define interface boundaries between independently developed modules.

Edit: broken leftover line

[fishnchips]

Things change a lot during the project, I agree. But assuming everything will change and using that as an excuse not to plan at all sounds like a poor choice to me.

"In preparing for battle I have always found that plans are useless, but planning is indispensable." - Dwight D. Eisenhower

[gpderetta]

I do actually agree that some amount of planning, and especially putting down a sketch on paper do help, in particular as a way to nail down requirements and figuring out obvious roadstops, but it is important to be under no illusion that the final project will resemble in any way the plan

[lmm]

> But assuming everything will change and using that as an excuse not to plan at all sounds like a poor choice to me.

I know that's how it sounds. But in my experience it turns out to be a lot more effective than planning.

[draw_down]

We write those on my team. They usually end up only faintly resembling what actually gets shipped. No matter how much agreement there is beforehand, as soon as people see working software they change their minds. The best thing is to plan for and expect change, not try to mitigate its manifesting.

[phasmantistes]

Yep, and design docs are the most important part of that planning for change. If you haven't planned the desired design, you can't see how circumstances are forcing you to change it, and what tradeoffs you'll be making to accommodate those circumstances.

[fishnchips]

True, if your software is well designed and modular then it's easy to contain necessary changes to a single subsystem. But a good design document would allow you just that - designate boundaries and interfaces between subsystems. I personally found out that writing an IDL (Protocol Buffers, Thrift or what have you) along your design document is very useful since it nicely bridges abstract concepts with actual code.

[ArkyBeagle]

Well, except for the elephant-in-the-room of the dread lord Requirements Traceability, which will frequently eat the entire design cycle.

[jaimeyap]

This is generally why smaller changes are better. They are more easily reviewed, and more easily "steered" as you put it by a good review.

And of course having a design doc ahead of time that the team can review and comment on for a longer, more complicated change, is a great thing to do. And compliments the subsequent code review[s].

[ben_jones]

I think it will always depend on the team. Process X might be a savior to team Y, but completely screw team A even if team A == team Y (javascript truthiness here).

On the flip side I think you can also say the potential downsides of excessive code review are significantly less then the potential downsides of zero code review.

I dunno.

[frandroid]

> the potential downsides of excessive code review

What would those be?

[gurkendoktor]

Large commits getting stuck in review forever, or until they have too many conflicts to be merged cleanly; small commits being over-reviewed for trivial issues because the reviewer wants to prove that they have actually looked at the code.

[aavz]

On the other hand, if you skip code review for major or complicated changes, you end up with large chunks of code that only one person understands.

Sometimes that works out fine, but other times you don't find out until too late that there are major flaws.

[jupp0r]

You can also refuse changes for being too big to review. Creating incentives for small changes seem like a good thing to me, so I don't see where you complaint is.

[gurkendoktor]

As I said, review culture can also create incentives against small commits, because they will often be over-analysed. I've seen this sentiment on the internet, so it can't just be my own anecdata.

https://twitter.com/girayozil/status/306836785739210752

Not to mention trivial commits, like clarifying comments, something which doesn't happen anymore in my current project - nobody can be bothered to go through the review process for that.

I'm not against code reviews, but I prefer casual post-commit reviews for uncontroversial changes.

[ThrustVectoring]

Bikeshedding and nitpicking. People can try to put you down a peg through overly critical review.

[TheGRS]

I'm not sure what ben_jones had in mind, but we've been doing a lot of pairing and mob programming where I work. I really enjoy it and it feels like the code review is basically just being done at all times (the reviewer is sitting right next to you as you write). But others feel like it eliminates some of the advantages of parallelism (i.e. 2 people working on 2 different things). It also slows down the amount of time to complete stories if one developer is less skilled than the other, but that issue should fade with time.

[frandroid]

Pair programming is an extreme version of code reviewing though :)

[golergka]

Imagine your team prototyping game design ideas with average speed of 1-2 days per concept.

[frandroid]

Prototyping ideas is not a core period for code review though...

ETA: If you end up using a prototype, then you clean up the code at the end of the 1-2 days, and request a review at that point. Code reviews are not meant to get in the way of development. They're a post-development step that happens before QA/testing.

[maldusiecle]

How much code review you need is going to vary based on the kind of change. When my team has a large design decision, we anticipate that we're going to have to create some exploratory code that won't ever reach production. We review "spike" pull requests--maybe several generations, depending on the scope of the change. And if needed we have on and offline discussions about our alternative, the tradeoffs, etc.

I think this works a lot better than trying to impose design reviews on every change. Most changes shouldn't require this kind of scrutiny; you don't want to slow down 95% of code reviews for the sake of the difficult 5%.

[kzhahou]

Y not both?