Estimates, Code Review and Priorities

These are the daily questions: "How long will it take?", "Is this PR ok?", "What should we do first?", "Can we skip the tests this once?". They test your judgement more than your syntax. This lesson gives small habits for each, and model answers.

Read Lesson 0 first for the six-step answer shape. For the experience-round version of these topics see Experience round, Full-Stack Lesson 14; for Git and pull-request workflow see Full-Stack Lesson 24.

Everything here is common practice and opinion, not law. Teams differ. Use the ideas, not the exact words, and always use your own true examples.

Words used in this lesson

• Technical debt is a shortcut that saves time now and costs time later, like a loan with interest.
• A spike is a short, time-limited experiment to learn something ("one afternoon to try this library on a real page"). You throw the code away afterwards if needed.
• A proof of concept (PoC) is a small version that shows an idea can work. It is not production code.
• A reversible decision is one you can undo cheaply (change a variable name, try a library on one page). A hard-to-reverse decision is one that is expensive to undo (database choice, public API shape, a language for the whole backend). Spend more thinking on the second kind.

Thinking about size and speed: estimates

A lead needs dates for planning, and you need to protect yourself. A few honest habits:

• Give a range, and name the unknowns. "Three to five days. The two days of risk are the payment provider's test account, which I have not used yet."
• Break work into small pieces. If a piece is longer than two or three days, it is still too vague to estimate well. Split it.
• Include the invisible work: tests, review, deploy, docs, and fixing what you find on the way.
• Update early. If the estimate changes, tell the lead the same day. Early bad news is cheap; late bad news is not.
• Do a spike for big unknowns. "I need a half day to learn whether this library can do X. Then I can give a real estimate."

For more on estimates as a story see Lesson 38 (coming soon).

Four words that are easy to mix up. One article in the planning list ("Hacks for engineering estimates") separates an estimate (what you think will happen), a target (what someone wants), a commitment (a promise) and a plan (the steps). Many fights about estimates are really one person saying "target" and the other hearing "commitment". Say which one you mean.

Reviewing code like a lead

If you review other people's code, you are already doing a lead's job. Google's public code review guide is the most widely copied example, so here are its main rules in simple words.

The standard: better, not perfect.

• Reviewers should lean towards approving a change once it definitely improves the overall health of the code, even if it is not perfect. The guide says there is no perfect code, only better code.
• Nothing justifies approving a change that clearly makes the code health worse, except an emergency.
• It is always fine to leave comments that help the author learn something. If it is not critical, mark it "Nit:".
• Do not let a change sit because the author and the reviewer cannot agree.

When two people disagree, use this order (the guide's own order):

1. Facts and data beat opinions and personal preference.
2. The style guide is the final word on style. For style points it does not cover, accept the author's choice.
3. Design is almost never "just style". Judge it on principles. If the author shows that several approaches are equally good, accept the author's preference.
4. If nothing else applies, ask for consistency with the current codebase, as long as code health does not get worse.

Speed is part of quality.

• Reply fast. The guide says the maximum is one business day to respond. It also says speed of each reply matters more than how long the whole review takes.
• Slow reviews slow the whole team, frustrate people, and push them to accept weaker code. The guide says most complaints about review are really solved by making it faster.
• Do not lower your standards to be fast. Stay strict on substance and quick in reply.
• If you are busy, send a quick holding reply with a time, or suggest another reviewer. If the changes are minor, you can approve "with comments" and say which comments are optional.

Keep changes small.

• A small change is reviewed faster and more carefully, has fewer bugs, merges more easily and is simpler to roll back.
• There is no hard rule. The guide says about 100 lines is usually fine and about 1,000 lines is usually too large, and the number of files matters too (200 lines in one file is easier than 200 lines in 50 files).
• Keep refactoring separate from a feature or a bug fix. Put the tests in the same change as the code.
• If a change is too big, ask the author to split it (for example one small change first, then the next one on top of it).

Write comments that help.

• Be kind. Comment on the code, not the person. For example, say "this adds concurrency complexity without a measured benefit", not "why did you do this?".
• Explain why (the principle or the benefit), not only what to change.
• Point out the problem and let the author choose the fix, unless a direct suggestion is more helpful.
• Label the weight of a comment: Nit (small, will not matter much), Optional / Consider (a good idea, not required), FYI (for later, not this change).
• If code is confusing, ask the author to make the code clearer, not only to explain it in the review tool.
• Say what is good too. People learn from that.

Interview questions

Key takeaways

• Give estimates as ranges, name the unknowns, and include tests, review, deploy and fixing.
• Split work until each piece is a few days or less. Use a spike for big unknowns.
• Tell the lead about a slipping deadline the same day, with facts and options.
• Quick fix first when users are hurt now, then a ticket with an owner and a date for the proper fix.
• Refactor in small steps, near the code you are changing, when it slows real work.
• Review for "better, not perfect", label small remarks as nits, explain why, and reply fast.
• When everything is urgent, ask the owner to rank, and show what moves if A goes first.
• Say "no" with a reason and an option. Do not trade away real safety or legal problems.
• This is common practice and opinion. Adapt it to your team.

What's next

Next: Incidents, team health and growing as a lead, Lesson 4. Practise: take your last three tasks and write the estimate you would give now, as a range with the biggest risk.