huiali/rust-skills/rust-anti-pattern

// ❌ Bad: escaping borrow checker
fn process(user: User) {
    let name = user.name.clone();  // Why clone?
    // ...
}
 
// ✅ Good: use references
fn process(user: &User) {
    let name = &user.name;  // Just borrow
}

// ❌ Bad: may panic
let config = File::open("config.json").unwrap();
 
// ✅ Good: propagate error
let config = File::open("config.json")?;
 
// ✅ Good: with context
let config = File::open("config.json")
    .context("failed to open config")?;

// ❌ Bad: unnecessary allocation
fn greet(name: String) {
    println!("Hello, {}", name);
}
 
// ✅ Good: borrow is enough
fn greet(name: &str) {
    println!("Hello, {}", name);
}
 
// When String is actually needed: ownership or mutation required

// ❌ Bad: error-prone, inefficient
for i in 0..items.len() {
    println!("{}: {}", i, items[i]);
}
 
// ✅ Good: direct iteration
for item in &items {
    println!("{}", item);
}
 
// ✅ Good: with index
for (i, item) in items.iter().enumerate() {
    println!("{}: {}", i, item);
}

// ❌ Bad: unsafe for convenience
unsafe {
    let ptr = data.as_mut_ptr();
    // ... complex memory operations
}
 
// ✅ Good: find safe abstractions
let mut data: Vec<u8> = vec![0; size];
// Vec handles memory management

// ❌ Anti-pattern: clone to satisfy borrow checker
fn process_data(data: &Data) -> String {
    let cloned = data.items.clone();
    cloned.into_iter().map(|x| x.to_string()).collect()
}
 
// ✅ Solution: use references properly
fn process_data(data: &Data) -> String {
    data.items.iter().map(|x| x.to_string()).collect()
}

// ❌ Anti-pattern: unwrap chain
fn load_config() -> Config {
    let content = std::fs::read_to_string("config.toml").unwrap();
    toml::from_str(&content).unwrap()
}
 
// ✅ Solution: Result propagation
fn load_config() -> Result<Config, Box<dyn Error>> {
    let content = std::fs::read_to_string("config.toml")?;
    Ok(toml::from_str(&content)?)
}
 
// ✅ Solution: with context (anyhow)
fn load_config() -> anyhow::Result<Config> {
    let content = std::fs::read_to_string("config.toml")
        .context("failed to read config file")?;
    toml::from_str(&content)
        .context("failed to parse config")
}

// ❌ Anti-pattern: String parameters everywhere
struct Config {
    host: String,
    port: String,
    path: String,
}
 
impl Config {
    fn new(host: String, port: String, path: String) -> Self {
        Self { host, port, path }
    }
}
 
// ✅ Solution: accept &str, store String
impl Config {
    fn new(host: impl Into<String>, port: u16, path: impl Into<String>) -> Self {
        Self {
            host: host.into(),
            port: port.to_string(),
            path: path.into(),
        }
    }
}

// ❌ Anti-pattern: manual indexing
fn sum_even(nums: &[i32]) -> i32 {
    let mut sum = 0;
    for i in 0..nums.len() {
        if nums[i] % 2 == 0 {
            sum += nums[i];
        }
    }
    sum
}
 
// ✅ Solution: iterator chain
fn sum_even(nums: &[i32]) -> i32 {
    nums.iter()
        .filter(|&&n| n % 2 == 0)
        .sum()
}

Code review checklist:
  → Lots of .clone()? Check ownership design
  → .unwrap() in lib code? Need error handling
  → Index loops? Should use iterators
  → pub fields with invariants? Need encapsulation
  → >50 line functions? Should split

1. Is this fighting Rust or working with Rust?
   Fighting → Redesign
   Working with → Continue
 
2. Is this clone necessary?
   Escaping borrow checker → Warning sign
   Actually need copy → Keep
 
3. Will this unwrap panic?
   Might panic → Use ?
   Never panics → expect("reason")
 
4. Is there a more idiomatic way?
   Check std library patterns
   Review other Rust code

Identified anti-pattern?
  ↓
Understand the root cause
  ↓
Find idiomatic alternative
  ↓
Refactor incrementally
  ↓
Test thoroughly

# Check for common issues
cargo clippy
 
# Specific anti-pattern lints
cargo clippy -- -W clippy::clone_on_copy \
                -W clippy::unwrap_used \
                -W clippy::expect_used
 
# Check for complexity
cargo clippy -- -W clippy::cognitive_complexity
 
# Find todos and fixmes
rg "TODO|FIXME|XXX|HACK" --type rust

// ❌ Bad: cloning to satisfy compiler
fn process(items: &Vec<Item>) -> Vec<String> {
    let items_clone = items.clone();
    items_clone.into_iter().map(|i| i.name).collect()
}
 
// ✅ Good: proper borrowing
fn process(items: &[Item]) -> Vec<String> {
    items.iter().map(|i| i.name.clone()).collect()
}
 
// ✅ Better: no clone at all
fn process(items: &[Item]) -> Vec<&str> {
    items.iter().map(|i| i.name.as_str()).collect()
}

// ❌ Bad: panic on error
let data = fetch_data().unwrap();
let parsed: Config = serde_json::from_str(&data).expect("bad JSON");
 
// ✅ Good: proper error propagation
fn load_data() -> Result<Config, Box<dyn Error>> {
    let data = fetch_data()?;
    let parsed = serde_json::from_str(&data)?;
    Ok(parsed)
}

// ❌ Bad: allocating for no reason
fn log_message(level: String, msg: String) {
    println!("[{}] {}", level, msg);
}
 
// ✅ Good: borrow when possible
fn log_message(level: &str, msg: &str) {
    println!("[{}] {}", level, msg);
}