Gå till innehållet

Skriven av ledningen för apputvecklingen.

Code reviews

Reviews are a shared responsibility. The goal is not to find fault. It is to improve the code together and catch problems before they reach the main branch. Be specific, be constructive, and distinguish between blockers and suggestions. Code reviews are also important for sharing knowledge.

Who should review my code? It might be tempting to ask a group leader or the tech lead to review, but we want you to find another developer to the largest extent possible. This is to encourage knowledge sharing, so that you aren't the only one who has seen that piece of code.

You might be asked to review a pull request. This can seem daunting at first if you don’t have a lot of experience. But don’t worry, it is not the end of the world if some bad code gets merged, we can always fix it later. It is usually when something goes wrong that you learn the most! Read on to see how a review should be carried out.

Some tips when reviewing a pull request:

  • Sit next to the person you are reviewing if possible! This way you can ask them any questions that come up directly. Sometimes just talking about the code helps you find any mistakes
  • Check out the code and test it locally. In the end the most important part is that it works as it should.
  • Make sure that the code follows our Code style.

What to Look For

Correctness

  • Does the code do what the PR description claims it does? (Test it!)
  • Are edge cases handled (empty inputs, nulls, boundary values)?
  • Could the change introduce a regression elsewhere?

Logic and Control Flow

  • Are there off-by-one errors or incorrect loop bounds?
  • Are error paths handled correctly, or is an error silently swallowed?

Safety and Robustness

  • Are unwrap() / expect() or similar exit calls justified? Could they panic in production?
  • Is input from external sources validated before use?
  • Are resource lifetimes and ownership correct?

Tests

  • Are new behaviors covered by tests?
  • Do existing tests still pass, and do they still test what they claim to test?
  • Are tests testing behavior, not implementation details?

Readability

  • Can a future reader understand the code without the PR description?
  • Are names clear and consistent with the surrounding code?
  • Is there dead code or commented-out blocks that should be removed?

Commit hygiene

  • Does the commit history follow the conventions in this document?
  • Are commits atomic; each one a single logical change?
  • Do any commits carry a Co-Authored-By AI trailer? If so, apply extra scrutiny to that code. Pay closer attention to edge cases, logic, and whether the implementation actually matches the intent. AI agents tend to things in many places for a single fix. Make sure the structure of the change makes sense.

Leaving Review Comments

All comments must be resolved before a PR may merge. "Resolved" means the author has either addressed the comment or explicitly replied explaining why the change was not made; not simply dismissed.

Please preface comments with one of the following so that the author knows the severity of your comment.

  • Blocker: a correctness issue, safety problem, or clear violation of project conventions. Must be fixed.
  • Suggestion: an improvement worth considering. The author may resolve it by making the change or by replying with a reasoned explanation of why the current approach is preferred. Prefix with nit: for minor style observations.
  • Question: you need more context before you can assess the code. Must be answered before merge. Please phrase this as a genuine question ("Why is X done this way rather than Y?" or "How exactly does Z work?"). This kind of observation may warrent further documentation (if the reviewer is asking the question, it's likely to be asked about again when reading the code).

Approve when you are confident the code is correct and consistent with the codebase, even if you have left suggestions (resolve if not critical). Do not withhold approval over personal style preferences.

Example Pull Request and Review

Below is an example of a PR and the kind of review it should receive.

PR: [API] (Update): Add support for filtering activities by date range

Description

Add a date_range query parameter to the activity listing endpoint, allowing callers to restrict results to a given start/end date. Integrates with the existing filter chain.

Diff (simplified)

// api/src/routes/activities.rs

pub fn filter_by_date_range(
    mut self,
    start: Option<NaiveDate>,
    end: Option<NaiveDate>,
) -> Self {
    if start.is_none() && end.is_none() {
        return self;
    }
    self.filters.push(Filter::DateRange { start, end });
    self
}

// api/src/filters.rs

Filter::DateRange { start, end } => {
    if let Some(s) = start {
        query = query.filter(date_col.ge(s));
    }
    if let Some(e) = end {
        query = query.filter(date_col.le(e));
    }
}

// api/tests/activity_tests.rs

#[test]
fn test_date_range_filter() {
    let start = NaiveDate::from_ymd(2024, 1, 1);
    let end = NaiveDate::from_ymd(2024, 12, 31);
    let q = QueryBuilder::new()
        .filter_by_date_range(Some(start), Some(end))
        .build();
    assert!(q.to_sql().contains("date_col"));
}

Review

[Blocker] (executor.rs) Inverted date range not rejected

Nothing prevents the caller from passing start > end. This would produce a logically impossible filter that silently returns zero rows. Either document that this is the caller's responsibility, or add a guard:

Filter::DateRange { start, end } => {
    if let (Some(s), Some(e)) = (start, end) {
        debug_assert!(s <= e, "start must not be after end");
    }
    // ...
}

If this should be a hard error rather than an assertion, return a Result from filter_by_date_range instead.

[Blocker] (query_tests.rs) Test does not verify filtering behaviour

The test checks that "date_col" appears somewhere in the SQL string. This is testing an implementation detail, not the behaviour. A refactor that renames the column would break the test without breaking the feature.

Consider testing with an in-memory database and asserting on which rows are returned, or at minimum assert that the SQL contains the expected >= and <= predicates with the correct date values.

[Suggestion] (routes/activities.rs) Consider a dedicated DateRange type

Using two Option fields means callers can easily pass (None, None) by accident, which the early return silently discards. A named type makes the intent clearer and prevents the degenerate case at the type level:

pub struct DateRange {
    pub start: NaiveDate,
    pub end: NaiveDate,
}

pub fn filter_by_date_range(mut self, range: Option<DateRange>) -> Self { ... }

[Suggestion nit] (query_tests.rs) from_ymd is deprecated

NaiveDate::from_ymd was deprecated in chrono 0.4.23. Use from_ymd_opt(...).unwrap() instead?

Author Response

Re: [Blocker] Inverted date range not rejected

Good catch. I've added a guard in filter_by_date_range that returns an Err when start > end, and updated the callers to propagate the error. A hard error feels right here since an inverted range is almost certainly a bug on the caller's side, not an intended edge case.

Re: [Blocker] Test does not verify filtering behaviour

Agreed, the test was effectively useless. Replaced it with an integration test that seeds an in-memory SQLite database with three activities (before, within, and after the range) and asserts that only the in-range row is returned. The SQL string assertion is gone.

Re: [Suggestion] Consider a dedicated DateRange type

I've introduced the DateRange struct as suggested. It also gives us a natural place to put the start <= end invariant check, so I moved the validation there instead of inside the filter chain. The signature is now filter_by_date_range(mut self, range: Option<DateRange>) -> Result<Self, FilterError>.

Re: [Suggestion nit] from_ymd is deprecated

Fixed, switched to from_ymd_opt(...).unwrap() throughout the test file.

Approval and Merge

After a PR has been approved, the author of the PR is responsible for merging it, provided they have sufficient permissions. Use rebase. Also update the examples with links to your code if it contains patterns useful elsewhere!

[Approved]

All blockers addressed. Resolving nit suggestion that you can take or leave on a follow-up.

PR rebased into main with the commit (a PR could have several commits though!):

[API] (Update): Add support for filtering activities by date range