khalil Drissi Cms

Tag: Collaboration

  • Code review best practices

    Code review best practices

    Code review is the single highest-leverage habit a team can have, and it is also the one most often done badly. I have been on the receiving end of reviews that felt like an interrogation and reviews that approved 800 lines with a thumbs-up emoji in four seconds. Both are failures. A good review catches real problems, spreads knowledge, and leaves the author feeling supported rather than judged.

    Review for the things humans are good at

    Do not spend your attention on formatting, import order, or whether a line is too long. A linter and an auto-formatter handle those, and they never get tired or grumpy. If your team argues about style in PR comments, you have a tooling gap, not a discipline problem. Configure the formatter, commit the config, and move on.

    What machines cannot check is whether the code is correct, whether it solves the actual problem, and whether someone six months from now will understand it. That is where my attention goes:

    • Does this do what the description says it does?
    • What happens at the edges: empty input, nulls, concurrent access, a network call that hangs?
    • Is there a simpler approach hiding behind this one?
    • Will the naming make sense to someone who was not in the room?

    Keep changes small enough to actually review

    The hard limit on review quality is size. Research and my own experience both say the same thing: past a few hundred lines, defect detection falls off a cliff because reviewers skim. When I get a giant PR, I ask the author to split it. A series of small, focused PRs gets genuine scrutiny on each one. This is also why I rebase and squash carefully before opening a PR, a habit I described in git workflow best practices.

    Comment like a colleague, not a compiler

    Tone is most of the job. The same point lands completely differently depending on phrasing. I ask questions instead of issuing verdicts, and I make it clear which comments are blocking versus optional.

    # Instead of:
This is wrong.

# Try:
What happens here if items is empty? I think
we'd index past the end. Could we guard with a
length check, or is that case impossible upstream?

# And label the small stuff:
nit: could inline this, non-blocking

    Marking nits as non-blocking is a small thing that removes a huge amount of friction. The author knows what they must fix to merge versus what is just my taste. I also make a point of saying when something is genuinely good. A review that is only criticism trains people to dread the process.

    Review promptly and finish what you start

    A PR sitting unreviewed for two days blocks a person and rots as main moves underneath it. I treat review requests as near the top of my queue, ideally same day. The cost of a stalled PR compounds: the author context-switches away, then has to reload everything when feedback finally arrives.

    When I review, I try to give all my feedback in one pass rather than dribbling comments out over three rounds. Nothing is more demoralizing than fixing everything, getting re-reviewed, and discovering five new comments that were always visible.

    The author has homework too

    Reviews go faster when the author makes them easy. Before I request review I write a description that explains what changed and why, I leave inline comments on my own diff pointing out anything non-obvious, and I make sure CI is green. A good description can cut review time in half because the reviewer is not reverse-engineering intent from the diff.

    • State the problem, not just the solution.
    • Call out anything you are unsure about and want eyes on.
    • Include screenshots or sample output for anything user-facing.

    Disagree, then commit

    Sometimes the author and reviewer just see it differently. When the disagreement is about taste rather than correctness, I default to the author’s call after voicing my view once. Dragging a PR through five rounds over a stylistic preference burns goodwill that you will want later. Save the firm stands for things that actually matter: correctness, security, and the data contracts I care so much about in REST API design guidelines. Get those right and let the small stuff go.

  • Git workflow and branching best practices

    Git workflow and branching best practices

    I have worked on teams that treated Git like a sacred ritual and teams that treated it like a junk drawer. Neither extreme ships software well. After years of cleaning up messy histories and untangling merge disasters, I have landed on a set of habits that keep things calm. None of them are clever. That is the point.

    Pick one branching model and stop arguing about it

    The model matters less than the agreement. For most product teams I use trunk-based development with short-lived feature branches. You branch off main, you do your work in a day or two, you merge back, you delete the branch. The longer a branch lives, the more it drifts, and the more painful the eventual merge becomes. Long-lived release branches have their place in software that ships on a fixed cadence to customers who cannot update on demand, but for a web app deploying several times a day they are pure overhead.

    What I actively avoid is the elaborate GitFlow setup with develop, release, hotfix, and feature branches all interleaving. I have watched it confuse new hires for weeks. If your deployment is continuous, your branching should be too.

    Write commits that explain the why

    A commit message is a note to whoever is reading the history at 2am during an incident, and that person might be you. The diff already shows what changed. The message needs to capture why. I keep the subject line under about fifty characters, written in the imperative, and I use the body to explain reasoning when the change is not obvious.

    fix: prevent double charge on retry

The payment client retried on a 504 even though the
charge had already gone through on the gateway side.
We now key the request with an idempotency token so
the gateway dedupes it. Closes #482.

    Atomic commits are the other half of this. One logical change per commit. When a commit does three unrelated things, you can never cleanly revert one of them, and bisecting becomes useless. If you find yourself writing “and” in a subject line, that is two commits.

    Rebase your own work, merge shared work

    This is the rule that saves the most pain. Before I open a pull request, I rebase my branch onto the latest main so my changes sit on top of current reality and review is a clean read. But once a branch is shared or a PR is open and others have looked at it, I stop rebasing and merge instead, because rewriting published history forces everyone else to recover their local state.

    • Rebase to tidy local commits before they are public.
    • Use interactive rebase to squash the inevitable “fix typo” and “actually fix it” commits.
    • Never force-push a branch other people are building on.
    • Protect main so nobody can push to it directly.

    Keep main always releasable

    The single most valuable property of a repository is that main always works. If main is green, you can cut a release at any moment, and a broken deploy can be fixed by reverting one merge. I enforce this with required status checks: tests and linting have to pass before a merge button even appears. This connects directly to how I run reviews, which I wrote about in code review best practices. A clean history makes reviews faster, and good reviews keep the history clean. They feed each other.

    Make reverting boring

    When something breaks in production, the fastest safe action is usually to revert, not to debug live. Squash-merging each PR into a single commit on main makes this trivial: one PR is one commit, and reverting it removes the whole feature cleanly. I like squash merges for exactly this reason on application code, though for libraries where individual commit history carries real value I keep the full history.

    Tag your releases so you can always answer “what was running last Tuesday.” A lightweight tag costs nothing and turns a vague question into a one-line answer.

    Some habits that pay off quietly

    • Commit a sensible .gitignore on day one so secrets and build artifacts never enter history. Removing a leaked credential from history is a bad afternoon.
    • Pull with rebase by default to avoid the noise of merge commits on every sync.
    • Keep PRs small. A 200-line PR gets a real review. A 2000-line PR gets a rubber stamp.

    Git rewards discipline more than knowledge. You do not need to memorize the plumbing commands. You need a small set of agreements that everyone actually follows. The same thinking shows up when I design data stores, which I covered in database schema best practices, where a few firm conventions early save enormous cleanup later.