6 ms·
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
by buro9 10y ago
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".
- jaimeyap 10y agoThis 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].
- fishnchips 10y agoThis is the role of design documents which get heavily reviewed by the team before even the first line of code is written.
- gpderetta 10y agoIn 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 10y agoThings 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 10y agoI 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 10y ago> 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 10y agoWe 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.
- fishnchips 10y agoTrue, 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.
- phasmantistes 10y agoYep, 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.
- ArkyBeagle 10y agoWell, except for the elephant-in-the-room of the dread lord Requirements Traceability, which will frequently eat the entire design cycle.
- eric_h 10y agoI 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 10y agoI 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_ 10y agoimo, 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.
- ben_jones 10y agoI 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 10y ago> the potential downsides of excessive code review What would those be?
- golergka 10y agoImagine your team prototyping game design ideas with average speed of 1-2 days per concept.
- frandroid 10y agoPrototyping 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.
- TheGRS 10y agoI'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 10y agoPair programming is an extreme version of code reviewing though :)
- wahnfrieden 10y agoOne 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.
- maldusiecle 10y agoHow 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 10y agoY not both?