// ✅ DO: Clear ownership, explicit borrows
pub struct User {
    pub id: UserId,
    pub name: String,
    pub email: String,
}

impl User {
    pub fn new(id: UserId, name: String, email: String) -> Self {
        Self { id, name, email }
    }
    
    // Borrow when you don't need ownership
    pub fn display_name(&self) -> &str {
        &self.name
    }
    
    // Take ownership when consuming
    pub fn into_dto(self) -> UserDto {
        UserDto {
            id: self.id.to_string(),
            name: self.name,
            email: self.email,
        }
    }
}

// ❌ DON'T: Unnecessary clones, unclear ownership
impl User {
    pub fn display_name(&self) -> String {
        self.name.clone() // Unnecessary allocation
    }
}

// ✅ DO: Custom error types with thiserror
use thiserror::Error;

#[derive(Error, Debug)]
pub enum UserError {
    #[error("User not found: {0}")]
    NotFound(UserId),
    
    #[error("Invalid email format: {0}")]
    InvalidEmail(String),
    
    #[error("Database error")]
    Database(#[from] sqlx::Error),
}

pub type Result<T> = std::result::Result<T, UserError>;

pub async fn get_user(db: &DbPool, id: UserId) -> Result<User> {
    let user = sqlx::query_as::<_, User>(
        "SELECT id, name, email FROM users WHERE id = $1"
    )
    .bind(&id)
    .fetch_optional(db)
    .await?
    .ok_or(UserError::NotFound(id))?;
    
    Ok(user)
}

// ❌ DON'T: String errors, unwrap(), expect()
pub async fn get_user(db: &DbPool, id: UserId) -> Result<User, String> {
    let user = sqlx::query_as::<_, User>(
        "SELECT id, name, email FROM users WHERE id = $1"
    )
    .bind(&id)
    .fetch_one(db)
    .await
    .expect("Database query failed"); // Panics!
    
    Ok(user)
}

// ✅ DO: Use types to enforce invariants
#[derive(Debug, Clone)]
pub struct Email(String);

impl Email {
    pub fn new(email: impl Into<String>) -> Result<Self> {
        let email = email.into();
        if Self::is_valid(&email) {
            Ok(Self(email))
        } else {
            Err(UserError::InvalidEmail(email))
        }
    }
    
    fn is_valid(email: &str) -> bool {
        email.contains('@') && email.len() > 3
    }
    
    pub fn as_str(&self) -> &str {
        &self.0
    }
}

// Now it's impossible to have an invalid email
pub struct User {
    pub id: UserId,
    pub email: Email, // Guaranteed valid
}

// ❌ DON'T: Rely on runtime checks everywhere
pub struct User {
    pub id: String,
    pub email: String, // Could be invalid
}

// ✅ DO: Proper async/await with error handling
use tokio::task;

pub async fn process_users(user_ids: Vec<UserId>) -> Result<Vec<User>> {
    let tasks: Vec<_> = user_ids
        .into_iter()
        .map(|id| {
            task::spawn(async move {
                user_service::get_user(id).await
            })
        })
        .collect();
    
    let mut users = Vec::new();
    for task in tasks {
        let user = task.await
            .map_err(|e| AppError::TaskJoin(e))??;
        users.push(user);
    }
    
    Ok(users)
}

// ❌ DON'T: Blocking operations in async context
pub async fn process_file(path: &str) -> Result<String> {
    // BAD: Blocks the async runtime
    let content = std::fs::read_to_string(path)?;
    Ok(content)
}

// GOOD: Use async file I/O
pub async fn process_file(path: &str) -> Result<String> {
    let content = tokio::fs::read_to_string(path).await?;
    Ok(content)
}

// ✅ DO: Well-designed traits with clear contracts
pub trait UserRepository: Send + Sync {
    async fn find_by_id(&self, id: UserId) -> Result<Option<User>>;
    async fn save(&self, user: &User) -> Result<()>;
    async fn delete(&self, id: UserId) -> Result<bool>;
}

// Generic over trait
pub struct UserService<R: UserRepository> {
    repository: R,
}

impl<R: UserRepository> UserService<R> {
    pub fn new(repository: R) -> Self {
        Self { repository }
    }
    
    pub async fn get_user(&self, id: UserId) -> Result<User> {
        self.repository
            .find_by_id(id)
            .await?
            .ok_or(UserError::NotFound(id))
    }
}

// Bad: Clone everywhere
fn process_users(users: Vec<User>) {
    for user in users.clone() {
        send_email(user.clone());
    }
}

// Good: Use references or take ownership
fn process_users(users: &[User]) {
    for user in users {
        send_email(user);
    }
}

fn send_email(user: &User) {
    // Just borrow
}

// Bad: Can panic
fn get_config() -> Config {
    std::fs::read_to_string("config.toml")
        .expect("Config must exist")
}

// Good: Return Result
fn get_config() -> Result<Config, ConfigError> {
    let content = std::fs::read_to_string("config.toml")
        .map_err(ConfigError::IoError)?;
    toml::from_str(&content)
        .map_err(ConfigError::ParseError)
}

// Bad: Trying to return reference to local
fn get_name() -> &str {
    let name = String::from("Alice");
    &name // Error: returns reference to local variable
}

// Good: Return owned data
fn get_name() -> String {
    String::from("Alice")
}

// Or borrow from parameter
fn get_first_name(full_name: &str) -> &str {
    full_name.split_whitespace().next().unwrap_or("")
}

// Bad: Verbose error handling
fn read_config(path: &str) -> Result<Config> {
    match std::fs::read_to_string(path) {
        Ok(content) => {
            match toml::from_str(&content) {
                Ok(config) => Ok(config),
                Err(e) => Err(ConfigError::ParseError(e)),
            }
        }
        Err(e) => Err(ConfigError::IoError(e)),
    }
}

// Good: Use ? operator
fn read_config(path: &str) -> Result<Config> {
    let content = std::fs::read_to_string(path)
        .map_err(ConfigError::IoError)?;
    toml::from_str(&content)
        .map_err(ConfigError::ParseError)
}

[package]
name = "my-app"
version = "0.1.0"
edition = "2021" # Use latest edition
rust-version = "1.70" # Specify MSRV

[dependencies]
# Use specific versions, not wildcards
tokio = { version = "1.35", features = ["full"] }
serde = { version = "1.0", features = ["derive"] }
thiserror = "1.0"

# Optional dependencies for features
sqlx = { version = "0.7", optional = true }

[dev-dependencies]
# Test-only dependencies
proptest = "1.4"

[features]
default = ["database"]
database = ["dep:sqlx"]

[[bin]]
name = "server"
path = "src/main.rs"

[profile.release]
lto = true # Link-time optimization
codegen-units = 1 # Better optimization, slower compile
opt-level = 3

### Rust Expert Review

**Ownership Model**: ✅ Correct borrowing, no unnecessary clones
**Error Handling**: ✅ Proper Result types with thiserror
**Memory Safety**: ✅ No unsafe code, all bounds checked
**Async Compliance**: ✅ Tokio runtime, non-blocking I/O
**Idioms**: ✅ Uses ? operator, iterators, pattern matching

**Clippy**: ✅ 0 warnings (ran with `cargo clippy`)
**rustfmt**: ✅ Formatted
**Documentation**: ✅ All public APIs documented

**Performance Notes**:
- Zero-cost abstractions maintained
- No unnecessary allocations
- Efficient iterator chains

**Recommendations**:
- Consider using `Arc<User>` if User is frequently shared
- Add property tests for validation logic