Skip to content

S17: Struct with many Option fields that are really states

S17Severity: Medium

Which fields are set depends on a hidden state that an enum should express.

Clippy: no lint finds this. Look for it in review.

A subscription is in a trial, active, or cancelled. Each state has its own data, and all of it is in Option fields:

pub struct Subscription {
pub customer: String,
pub trial_ends_on: Option<String>,
pub paid_until: Option<String>,
pub card_last4: Option<String>,
pub cancelled_on: Option<String>,
pub cancel_reason: Option<String>,
}
pub fn describe(s: &Subscription) -> String {
if let Some(day) = &s.cancelled_on {
format!("cancelled on {day}")
} else if let Some(day) = &s.paid_until {
format!(
"paid until {day}, card {}",
s.card_last4.as_deref().unwrap_or("????")
)
} else if let Some(day) = &s.trial_ends_on {
format!("trial until {day}")
} else {
"unknown".to_string()
}
}
  • It is the shape of a database row or a JPA entity. A table has one column for each value, and columns that only some rows use are nullable. The agent translates each nullable column to an Option.
  • It is the safe translation of null. The agent knows that Rust has no null, so each nullable Java field becomes Option<T>. That is correct for one field. For a group of fields that depend on each other, it misses the state.
  • New states add new fields. “Add a cancel reason” becomes one more Option.
  • Impossible states compile. Five Option fields have 32 combinations of set and not set. “Cancelled, but no cancel date”, “paid, but no card”, and “trial and cancelled” are all valid values. The test worse_accepts_states_that_cannot_exist builds one.
  • The state is hidden in the order of checks. describe tests cancelled_on first, then paid_until, then trial_ends_on. That order is now a business rule, and it is written nowhere else.
  • Callers must handle None where it cannot happen. An active subscription always has a card, but the code needs unwrap_or("????"), or an unwrap() that a reviewer must check.
  • There is an “unknown” case. The last else exists only because the type allows a subscription in no state at all.
  1. WorseFive Option fields, an if let chain whose order is a rule, and an "unknown" case.

    pub struct Subscription {
    pub customer: String,
    pub trial_ends_on: Option<String>,
    pub paid_until: Option<String>,
    pub card_last4: Option<String>,
    pub cancelled_on: Option<String>,
    pub cancel_reason: Option<String>,
    }
    pub fn describe(s: &Subscription) -> String {
    if let Some(day) = &s.cancelled_on {
    format!("cancelled on {day}")
    } else if let Some(day) = &s.paid_until {
    format!(
    "paid until {day}, card {}",
    s.card_last4.as_deref().unwrap_or("????")
    )
    } else if let Some(day) = &s.trial_ends_on {
    format!("trial until {day}")
    } else {
    "unknown".to_string()
    }
    }
  2. BetterA State enum. Each variant has exactly the fields that it needs. describe is one exhaustive match.

    pub struct Subscription {
    pub customer: String,
    pub state: State,
    }
    pub enum State {
    Trial {
    ends_on: String,
    },
    Active {
    paid_until: String,
    card_last4: String,
    },
    Cancelled {
    on: String,
    reason: Option<String>,
    }, // a reason is really optional
    }
    pub fn describe(s: &Subscription) -> String {
    match &s.state {
    State::Trial { ends_on } => format!("trial until {ends_on}"),
    State::Active {
    paid_until,
    card_last4,
    } => format!("paid until {paid_until}, card {card_last4}"),
    State::Cancelled { on, .. } => format!("cancelled on {on}"),
    }
    }

In the “better” version, an active subscription always has paid_until and card_last4, as plain String values. There is no “unknown”. The fields that all states share (customer) stay in the struct. One Option remains, reason, because a cancellation really can have no reason.

  • Fields that are optional on their own: a nickname, a second address line, a phone number. Each can be present or not, in any combination, and no state decides it.
  • At the boundary, the type that a database row or a JSON document is read into can match its shape. Convert it to the enum once, with TryFrom, and reject rows that are in no valid state.
  • No Clippy lint reports this pattern.
  • Look for three or more Option fields with names that belong to states: *_at, *_on, *_until, cancel_*, refund_*.
  • Look for if let ... else if let ... chains over different fields, and for unwrap() on a field “that is always set when…”.
Subscription has Option fields that depend on its state. Replace them with
an enum State { Trial { .. }, Active { .. }, Cancelled { .. } }, where each
variant holds only the fields that state needs, without Option where the
value is always present. Keep shared fields in the struct. Replace the
if-let chains with one exhaustive match. Add a TryFrom conversion from the
database row type that rejects rows in no valid state. Run the tests.

Related: S15: bool flags or string constants instead of an enum, Enums and pattern matching, and Option, not null.