Your first review

This page shows the review method that the rest of the site teaches. You review a function that an agent wrote, with the checklist, and you write the instruction for the agent.

The task

The prompt to the agent was:

Write a Rust function that reads a CSV file of orders with the columns order_id,customer,amount, and returns the total amount for each customer.

The agent returned this code. It compiles, and it works on the sample file. Read it once before you continue.

compute_totals.rs (as the agent wrote it)

use std::collections::HashMap;
use std::fs;

/// Computes the total order amount for each customer from a CSV file.
pub fn compute_totals(path: String) -> HashMap<String, f64> {
    let contents = fs::read_to_string(path.clone()).unwrap();
    let lines: Vec<&str> = contents.lines().collect();
    let mut totals: HashMap<String, f64> = HashMap::new();

    // Skip the header line
    for i in 1..lines.len() {
        let parts: Vec<&str> = lines[i].split(",").collect();
        let customer = parts[1].trim().to_string();
        let amount: f64 = parts[2].trim().parse().unwrap();

        if totals.contains_key(&customer) {
            let current = totals.get(&customer).unwrap().clone();
            totals.insert(customer.clone(), current + amount);
        } else {
            totals.insert(customer.clone(), amount);
        }
    }

    println!("Processed {} orders from {}", lines.len() - 1, path);
    totals
}

Step 1: understand it at a high level

Before you look for problems, say what the code does in one sentence: it reads the whole file into a string, skips the header, splits each line on commas, parses the third field as a number, and adds it to a map keyed by customer.

That is the right algorithm. The questions are about what happens when the input is not perfect, and whether the code says what it means.

Step 2: failure behavior

The checklist starts with failure behavior, because a crash in production costs more than any style problem. Look for unwrap(), expect(), panic!, and indexing with [...].

use std::collections::HashMap;
use std::fs;

/// Computes the total order amount for each customer from a CSV file.
pub fn compute_totals(path: String) -> HashMap<String, f64> {
    let contents = fs::read_to_string(path.clone()).unwrap();
    let lines: Vec<&str> = contents.lines().collect();
    let mut totals: HashMap<String, f64> = HashMap::new();

    // Skip the header line
    for i in 1..lines.len() {
        let parts: Vec<&str> = lines[i].split(",").collect();
        let customer = parts[1].trim().to_string();
        let amount: f64 = parts[2].trim().parse().unwrap();

        if totals.contains_key(&customer) {
            let current = totals.get(&customer).unwrap().clone();
            totals.insert(customer.clone(), current + amount);
        } else {
            totals.insert(customer.clone(), amount);
        }
    }

    println!("Processed {} orders from {}", lines.len() - 1, path);
    totals
}

1. Line 6: .unwrap() on the file read. A missing file or a permission error panics the program, with no file name in the message.
2. Line 13: parts[1] panics with "index out of bounds" when a line has fewer than two fields, for example an empty line in the middle of the file.
3. Line 14: parts[2] panics on a short line, and .parse().unwrap() panics on a value such as ten or 12,50. The message does not say which line.
4. Line 24: lines.len() - 1 on an empty file: 0 - 1 on a usize panics in debug builds and wraps to a huge number in release builds.

Finding 1 (High): four ways to panic on bad input. Lines 6, 13, 14, and 24 can each stop the program. The function cannot report a problem to its caller, so a single bad line in a 100,000-line file kills the whole run. These are S02: unwrap() / expect() in non-test code and S24: Indexing (v[i], s[a..b]) on untrusted input.

Java habit

In Java, Files.readString throws a checked IOException, so you would see the error path in the signature. Here, unwrap() turned the error into a crash, and the signature -> HashMap<...> hides that the function can fail.

Step 3: types and correctness

Finding 2 (Medium): money as f64. 0.10 + 0.10 + 0.10 is not exactly 0.30 in binary floating point, in Rust or in Java. Use an integer number of cents (or a decimal crate). The test for this page shows the difference.

Finding 3 (Low): HashMap gives a random iteration order. If the caller prints the totals, the order changes from run to run. A BTreeMap keeps customers sorted. This depends on the use; ask the agent what the caller does.

Step 4: ownership and copies

Finding 4 (Low): path: String and path.clone(). The function only reads the path, so it should borrow it: path: &Path. Then the clone() on line 6 is not necessary. See S05: Owned parameters (String, Vec<T>) where a borrow is sufficient and S01.

Step 5: readability and idiom

Finding 5 (Low): Java-style loops and map updates.

• for i in 1..lines.len() with lines[i] is an index loop (S10). contents.lines().skip(1) says “skip the header” directly.
• collect() into lines only to loop over it again allocates a second list (S11).
• contains_key, then get(...).unwrap().clone(), then insert: three lookups and an unwrap. The entry API does it in one: *totals.entry(customer).or_insert(0) += cents;.
• println! in a library function writes to stdout as a side effect. The caller should decide what to log.

The improved version

First, an error type that says what went wrong and where:

The error type

use std::collections::BTreeMap;
use std::fmt;
use std::path::{Path, PathBuf};
use std::{fs, io};

#[derive(Debug)]
pub enum TotalsError {
    Read { path: PathBuf, source: io::Error },
    BadLine { line: usize, reason: String },
}

impl fmt::Display for TotalsError {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        match self {
            Self::Read { path, .. } => write!(f, "cannot read {}", path.display()),
            Self::BadLine { line, reason } => write!(f, "line {line}: {reason}"),
        }
    }
}

impl std::error::Error for TotalsError {
    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
        match self {
            Self::Read { source, .. } => Some(source),
            Self::BadLine { .. } => None,
        }
    }
}

Then the function. It borrows the path, returns Result, and reports the line number:

compute_totals.rs (after review)

/// Total amount in cents for each customer, sorted by customer name.
///
/// # Errors
/// Returns an error if the file cannot be read or a line is not valid.
pub fn compute_totals(path: &Path) -> Result<BTreeMap<String, u64>, TotalsError> {
    let contents = fs::read_to_string(path).map_err(|source| TotalsError::Read {
        path: path.to_path_buf(),
        source,
    })?;
    parse_totals(&contents)
}

/// # Errors
/// Returns an error for the first line that is not valid.
pub fn parse_totals(contents: &str) -> Result<BTreeMap<String, u64>, TotalsError> {
    let mut totals = BTreeMap::new();
    for (index, line) in contents.lines().enumerate().skip(1) {
        let line_no = index + 1;
        let bad = |reason: &str| TotalsError::BadLine {
            line: line_no,
            reason: reason.to_string(),
        };

        let mut fields = line.split(',').map(str::trim);
        let (Some(_id), Some(customer), Some(amount), None) =
            (fields.next(), fields.next(), fields.next(), fields.next())
        else {
            return Err(bad("expected 3 fields: order_id,customer,amount"));
        };
        let cents = parse_cents(amount).ok_or_else(|| bad("amount must look like 12.34"))?;
        *totals.entry(customer.to_string()).or_insert(0) += cents;
    }
    Ok(totals)
}

And the money parser, which uses integer cents and checked arithmetic:

/// Parses "12.34" or "12" as cents. Money is never `f64`.
fn parse_cents(text: &str) -> Option<u64> {
    let (units, fraction) = text.split_once('.').unwrap_or((text, "00"));
    if fraction.len() != 2 {
        return None;
    }
    let units: u64 = units.parse().ok()?;
    let fraction: u64 = fraction.parse().ok()?;
    units.checked_mul(100)?.checked_add(fraction)
}

How to read the new parts:

• -> Result<BTreeMap<String, u64>, TotalsError>: the signature now says that the function can fail, and how.
• .map_err(|source| TotalsError::Read { path: ..., source })?: wrap the I/O error with the file name, then return it with ?. The original error stays available as the source.
• let (Some(_id), Some(customer), Some(amount), None) = (...) else { return Err(...) };: a let ... else. If the line does not have exactly three fields, leave the function with an error. No indexing, so no panic.
• parse_cents(amount).ok_or_else(...)?: turn “no value” (None) into an error with the line number.

Tip

The improved version is longer. That is normal: the extra lines are the error handling that the first version did not have. In a real application, the error type is often three lines with the thiserror crate.

What to tell the agent

A good review instruction names the location, the problem, and the fix. Copy and adjust:

Review fixes for compute_totals:
1. Do not panic on bad input. Return Result<_, TotalsError> with variants for a
   read error (keep the path and the io::Error as source) and a bad line (keep
   the 1-based line number). Remove every unwrap() and every parts[i] index.
2. Store money as u64 cents, not f64. Parse "12.34" without floating point.
3. Take path: &Path, not String. Remove the clone().
4. Use contents.lines().skip(1) and the entry API. Return a BTreeMap.
5. Remove the println!. Let the caller log.
Add tests for: a missing file, a short line, a non-numeric amount, an empty file.

What to remember

• Read for failure behavior first: every unwrap(), expect(), [i], and unchecked arithmetic on input is a place where the program can stop.
• A signature without Result on a function that does I/O is a red flag.
• Most of the other findings are cheap to fix and make the code shorter, not longer.

Next: the review checklist gives the full list of things to look for, and the Result concept page explains error handling in depth.