When I'm reviewing a PR, I'm not just looking for memory leaks or logic bugs—Rust's compiler already handles a lot of that. I'm looking for "idiomatic" code. You know the feeling: the code compiles and it works, but it feels like it was written by someone who is still thinking in C++ or Java. It's often technically correct, but it fights the language instead of flowing with it.

Let's look at a concrete example. Imagine we're writing a piece of logic for a system that filters a list of user profiles based on a minimum age and returns their usernames in uppercase. This is a common enough task, but there are two very different ways to write it.

The Push-and-Loop Habit

A lot of developers start with a pattern that looks like this. It's not "wrong," but it's noisy.

fn get_active_users(users: Vec<User>, min_age: u32) -> Vec<String> {
    let mut result = Vec::new();
    for user in users {
        if user.age >= min_age {
            result.push(user.name.to_uppercase());
        }
    }
    result
}

I call this the "Imperative Carry-over." You're manually managing the state of the `result` vector, explicitly calling `push`, and managing the loop. It works, but it's verbose. When I see this in a review, I'm thinking about the cognitive load. Every time you see a `mut` variable, you have to track how it changes over the course of the function. In Rust, we can express the intent of the operation rather than the mechanics of the loop.

The idiomatic way uses the iterator trait. It collapses the logic into a declarative pipeline:

fn get_active_users(users: &[User], min_age: u32) -> Vec<String> {
    users.iter()
        .filter(|u| u.age >= min_age)
        .map(|u| u.name.to_uppercase())
        .collect()
}

Notice two things here. First, I changed the input from Vec<User> to &[User]. Taking a slice is almost always better than taking a reference to a Vec because it's more flexible—it accepts both Vecs and arrays. Second, the iterator chain tells the reader exactly what's happening: we filter, we transform, we collect. It's cleaner, and in many cases, the compiler can optimize these chains better than a manual loop.

The Clone-Everything Reflex

The most common "smell" I see in mid-level Rust code is the overuse of .clone(). When the borrow checker starts yelling, the temptation is to just clone the data to make the error go away. I get it; it's frustrating. But cloning everywhere is a sign that your ownership model is slightly off.

Suppose you have a function that checks if a user's role is "Admin". A naive implementation might take a String, clone it to do a comparison, or require the caller to clone the string just to pass it in. If you see .clone() appearing in every other line of a function, you're essentially paying a performance tax to avoid thinking about lifetimes.

Instead, look for opportunities to use &str or Cow<str> (Clone-on-Write). If you only need to read the data, don't take ownership. If you might need to modify the data occasionally but usually just read it, Cow is your best friend. It allows you to avoid allocation entirely unless a mutation is actually required. I always ask in reviews: "Does this function actually need to own this data, or is it just borrowing it to satisfy the compiler?"

Unwrap as a Shortcut

Finally, let's talk about .unwrap(). In a prototype, it's fine. In a production codebase, it's a landmine. I see a lot of code that handles errors like this:

let config = std::fs::read_to_string("config.toml").unwrap();
let port = config.parse<u16>().unwrap();

This is "optimistic programming." It assumes the file exists and the port is a valid number. When this breaks, it doesn't give a helpful error message; it just crashes the thread. The idiomatic approach is to propagate the error up the stack using the ? operator. This shifts the responsibility of error handling to the caller, who actually has the context to decide whether to retry, log the error, or shut down gracefully.

Comparing unwrap() to ? is really a comparison of reliability. By returning a Result, you're documenting the failure modes of your function in the type signature itself. It makes the code self-documenting. If a function returns Result<T, E>, I know exactly what can go wrong without having to dive into the implementation and hunt for hidden panics.

📋 Practical Task

Refactoring the Configuration Loader

You have been handed a piece of code that "works" but is written in a very non-idiomatic style. It uses unnecessary clones, imperative loops, and risky unwraps. Your task is to refactor the load_settings function to be idiomatic Rust.

Requirements for your refactor:

• Replace the for loop and mut vector with an iterator chain (filter, map, collect).
• Change the function signature to take a slice (&[String]) instead of a Vec<String> to avoid taking ownership.
• Replace .unwrap() with the ? operator, changing the return type to a Result.
• Remove any unnecessary .clone() calls.

struct Setting {
    key: String,
    value: String,
}

fn load_settings(raw_lines: Vec<String>) -> Vec<Setting> {
    let mut settings = Vec::new();
    
    for line in raw_lines {
        // We only want lines that contain an '='
        if line.contains("=") {
            let parts: Vec<String> = line.split('=').map(|s| s.to_string()).collect();
            
            // This will panic if the split didn't produce 2 parts
            let key = parts[0].clone();
            let value = parts[1].clone();
            
            settings.push(Setting { key, value });
        }
    }
    
    settings
}