Step 6 of 6

REVIEW-9: refactor for code review

REVIEW-9: Address review comments on the grade report

The function works and has been in production for a year, but it's next in line to be extended, and the reviewer left comments. Refactor it without changing its output.

Much of a developer's week is reading and improving existing code. Good refactoring keeps behavior exactly the same (tests prove it) while making the code easier to read, harder to misuse and cheaper to run. The usual review comments in Rust:

Smell	Why it matters	Idiomatic fix
parallel `Vec`s (`names`, `scores`)	they can get out of sync, and every reader must remember they're linked	a `Vec` of structs
`Vec<T>` or `String` parameters you only read	forces callers to give up (or clone) their data	`&[T]` and `&str`
`while i < v.len()` and `v[i]`	off-by-one risk, bounds checks, noise	`for x in &v`, iterators
`.clone()` just to read	a needless allocation	borrow
magic numbers (`50`)	what does it mean, and where else is it?	a named `const`
`s = s + &a + &b` chains	hard to read, extra copies	`format!`, `write!`, `push_str`
`if flag == true`	noise	`if flag`
boolean parameters	`ship(3, true, false)` is unreadable at the call site	an enum per choice

Clippy flags several of these on its own (needless_range_loop for for i in 0..v.len() loops, ptr_arg for &Vec<T> parameters, bool_comparison for == true), and opt-in lints such as redundant_clone (in the nursery group) catch more. Running cargo clippy before asking for review saves everyone time.

Here's a small before and after: the same total, from a loop full of smells to an iterator chain.

const FREE_SHIPPING_CENTS: u64 = 5000;

struct Item {
    price_cents: u64,
    quantity: u64,
}

// Before: parallel vectors, an index loop, a magic number
fn total_before(prices: Vec<u64>, quantities: Vec<u64>) -> u64 {
    let mut total = 0;
    let mut i = 0;
    while i < prices.len() {
        total = total + prices[i] * quantities[i];
        i = i + 1;
    }
    if total < 5000 { total + 499 } else { total }
}

// After: one struct per line item, a slice, a named constant
fn total_after(items: &[Item]) -> u64 {
    let total: u64 = items.iter().map(|item| item.price_cents * item.quantity).sum();
    let shipping = if total < FREE_SHIPPING_CENTS { 499 } else { 0 };
    total + shipping
}

fn main() {
    let items = [Item { price_cents: 1200, quantity: 2 }, Item { price_cents: 350, quantity: 4 }];
    println!("{}", total_before(vec![1200, 350], vec![2, 4]));
    println!("{}", total_after(&items));
}

4299
4299

How to refactor safely

Pin the behavior first. Write tests for the current output, including edge cases (empty input, boundaries like exactly 50). Here the hidden tests do that for you.
Change one thing at a time, compiling and testing in between.
Keep the diff focused. A refactor PR that also changes behavior is hard to review. Do those in separate PRs.

Common mistakes

"Improving" the output. Even a fixed typo in a report can break a script that parses it. Same output, byte for byte.
Forgetting the empty case. The old code avoided dividing by zero; the new one must too.
writeln! into a String without use std::fmt::Write. The macro needs that trait in scope; the compiler error suggests the import.

Your task

The seed's grade_report works, but the review asks for:

Take students: &[Student] instead of two parallel Vecs (Student is in the seed).
No index loops (while, [i]) and no .clone().
A named constant const PASS_MARK: u32 = 50; instead of the magic number.
Build the text with format!, write!/writeln! or push_str instead of + chains.

The output must stay exactly the same: one line per student, NAME: pass (SCORE) or NAME: fail (SCORE), then average: N (rounded down) or average: n/a when there are no students.

Challenge 2

REVIEW-12: Boolean parameters in the shipping calculator

Call sites like ship_cost(4, true, false, true) keep causing bugs: nobody remembers which bool is which. Replace the flags with enums.

Add enum Speed { Standard, Express } and enum Zone { Domestic, International } and change the signature to fn ship_cost(weight_kg: u32, speed: Speed, zone: Zone, member: bool) -> u32 (keeping member: it's a real yes/no fact, and one bool is readable). Use a match on (zone, speed) to pick the base price and the price per kg, and drop the == true / == false comparisons. The prices must stay exactly the same.

Challenge 3

REVIEW-15: This parser has no tests

parse_duration turns "1h30m" into 5400 seconds for the job scheduler. It looks right, but nothing proves it, and the next person to touch it will break it. Add tests before approving.

Write at least 4 #[test] functions in the tests module. Your tests must pass on the code as it is, and they must be good enough to catch common mistakes: hidden broken copies of parse_duration (a wrong multiplier for hours or minutes, a forgotten reset between units, missing checks for empty input or a number without a unit) must each make at least one of your tests fail.

Cover the units, combinations like 1h30m, and the error cases ("", "10", "5x"). Use assert_eq! on the Ok values and assert!(... .is_err()) for errors.

Previous: SHOP-240: the order workflow state machine