enforcing code reviews for anything more than a few lines of code

General advice: require it especially for small changes. The fix failure rate (bugs introduced per line of code) is highest for one-line changes and then decreases. (See Rapid Development for further information on the data behind this.)

@Austin any further thoughts here?

@RobertCousineau not really - I'm writing less code for the Manifold codebase (and in general) so have fewer strong thoughts about best practices

This should go up over time, assuming manifold gets bigger.

I wish the linting / preview generation and other on-pr actions were faster. The slowness is the main reason I just push to main.

Testing in production so far has not lead to big issues. On the contrary, actually doing code reviews regularly (not just for big features) would require a lot of process changes or slower thoroughput.

@Sinclair Worth playing with https://turbo.build/pack to see if that'd speed up our build times and thus preview generation

@Sinclair You’re linting in the build pipeline? Curious why not locally?

Also makes me wonder if there are frequent lint-hygiene changes getting mixed in alongside the goal-of-the-request code? B/c that is the sort of thing that definitely makes code review a slog. Haven’t really looked at the commit history to see. Whitespace/style/lint/dependency bump changes, yeah, you want a completely separate fast lane for that sort of thing.

Totally agree that keeping the feedback loop to previewing/running the code super tight is an extremely important concern.

@Sinclair that has a ton of potential to not age well

The cost/benefit ratio of code review for defect detection and mitigation, and total cost of quality generally, beats the pants off just about everything else. The data from the studies that have been done are pretty stark.

https://www.pluralsight.com/blog/tutorials/code-review#code-review

@MattCWilson Manual code review in an early-stage startup just ends up wasting time. Using code review selectively for important code that you’re reasonably sure will be around a long time makes sense. Mandating code review for every possible feature, including many that will be thrown away, wastes our most precious resource, time.

@ian Mmm, I agree it’s important to be judicious, but “wasting time” is interesting. Are you talking about the delay that it introduces for the code author into the release loop? Or more the actual eyeball time of the reviewers?

A benefit that imo goes way undersung is the opportunity for new / less familiar staff to gain context on the truly vital and intricate parts of the codebase, so you’re not stuck someday losing a bus lottery.

@MattCWilson I’m talking about the eyeball time it takes for other engineers to review code that they don’t need to understand, or may be thrown away, or changed heavily. I agree it’d be nice to understand the whole codebase but it doesn’t seem worth the cost at this stage.

@ian This is helpful - “thrown away” or “changed heavily” is interesting. That sounds like this is primarily non-production code - experiments, prototypes, etc?

Not knowing much about how the codebase is or has been evolving, it would be really instructive to see what parts are undergoing the most code churn. If the churn is because of known experimentation or prototyping - sure, I can understand the desire to iterate unimpeded.

On the flip side though, sometimes churn is very much a sign that some section of code does need to be better understood by more of the team.

@MattCWilson I'd say look at things that are likely to or have already undergone extensive rewriting/retracting: this is the swipe's 2nd or 3rd rewrite, the current dashboard homepage will likely be replaced, single market CPMM multiple choice, bucketed numeric markets, badges, the old feed, tips, etc. Reviewing all the code for those experiments would've taken away from coding up and trying out other experiments. It doesn't seem like we have strong product market fit, so imo we need to focus on trying new experiments.

@ian Makes sense. If you don’t mind more questions - what level of design review did these experiments pass through, and what fraction of the team participated in those? Was there general consensus going in of like “here’s a high level flow/diagram/description of how Swipe will look/feel/act, and here’s a rough model of the relevant components?” Or were these more like a single coder pushing something out quickly? (Or maybe some A, some B?)

@MattCWilson One other question - when an experiment is deemed a failure, or in need of a rewrite: does the team do any retrospective reporting to help socialize the lessons that were learned?

@MattCWilson Typically we write up proposals on notion that are high level in nature and ask for reviews from the team. There’s less of a UI/UX design element to those proposals and more of a justification of why the feature should be built and what it will essentially do. Implementation and design details tend to come later after the person starts implementing it. Perhaps we could do more design mockups during the proposal phase so that they get more team eyeballs before the users see the feature. A lot of the time we’ve concluded it’s hard to know whether we’ll like the thing until trying it and it’s just best to build the thing if it won’t take too long.

We do retrospectives every month of things that succeeded and failed. On the whole we try to learn things from failures.

@ian I appreciate the info and the transparency. I suppose the main reason I ask all these questions is because I have a similarly sized team, all distributed, and we lean hard on code review, design review, and having primary and secondary stewards on all major components to make sure knowledge and discoveries get shared. I think those practices also help transmit team culture and keep us connected. It’s interesting to see and hear how y’all do as you do, especially from within the product itself, and seeing it evolve.

@MattCWilson And also - they’re never failures; only lessons. Nobody’s perfect.