Looking for a cleaner way to handle large number of Options

[ilmoi]

I have a struct that holds a rewards schedule that looks like this:

pub struct TierConfig {
    pub reward_rate: u64,

    pub required_tenure: u64,
}

pub struct FixedRateSchedule {
    pub base_rate: u64,

    pub tier1: Option<TierConfig>,

    pub tier2: Option<TierConfig>,

    pub tier3: Option<TierConfig>,
}

I need a function that can take any starting point and ending points, and return the total accrued rewards. You can think of it as finding the integral of a stepped curve.

My best attempt so far (brace yourself) is this monstrocity:

pub fn reward_amount(
    &self,
    start_from: u64,
    end_at: u64,
    gems: u64,
) -> Result<u64, ProgramError> {
    let (t1_start, t1) = self.extract_tenure("t1");
    let (t2_start, t2) = self.extract_tenure("t2");
    let (t3_start, t3) = self.extract_tenure("t3");

    // triage based on starting point on the outside, ending point on the inside
    let per_gem = if t3.is_some() && start_from >= t3_start.unwrap() {
        // simplest case - only t3 rate is applicable
        t3.unwrap().get_reward(start_from, end_at)
    } else if t2.is_some() && start_from >= t2_start.unwrap() {
        if t3.is_some() && end_at >= t3_start.unwrap() {
            let t2_reward = t2.unwrap().get_reward(start_from, t3_start.unwrap())?;
            let t3_reward = t3.unwrap().get_reward(t3_start.unwrap(), end_at)?;
            t2_reward.try_add(t3_reward)
        } else {
            // simplest case - only t2 rate is applicable
            t2.unwrap().get_reward(start_from, end_at)
        }
    } else if t1.is_some() && start_from >= t1_start.unwrap() {
        if t3.is_some() && end_at >= t3_start.unwrap() {
            let t1_reward = t1.unwrap().get_reward(start_from, t2_start.unwrap())?;
            let t2_reward = t2
                .unwrap()
                .get_reward(t2_start.unwrap(), t3_start.unwrap())?;
            let t3_reward = t3.unwrap().get_reward(t3_start.unwrap(), end_at)?;
            t1_reward.try_add(t2_reward)?.try_add(t3_reward)
        } else if t2.is_some() && end_at >= t2_start.unwrap() {
            let t1_reward = t1.unwrap().get_reward(start_from, t2_start.unwrap())?;
            let t2_reward = t2.unwrap().get_reward(t2_start.unwrap(), end_at)?;
            t1_reward.try_add(t2_reward)
        } else {
            // simplest case - only t1 rate is applicable
            t1.unwrap().get_reward(start_from, end_at)
        }
    } else {
        if t3.is_some() && end_at >= t3_start.unwrap() {
            let base_reward = self.get_base_reward(start_from, t1_start.unwrap())?;
            let t1_reward = t1
                .unwrap()
                .get_reward(t1_start.unwrap(), t2_start.unwrap())?;
            let t2_reward = t2
                .unwrap()
                .get_reward(t2_start.unwrap(), t3_start.unwrap())?;
            let t3_reward = t3.unwrap().get_reward(t3_start.unwrap(), end_at)?;
            base_reward
                .try_add(t1_reward)?
                .try_add(t2_reward)?
                .try_add(t3_reward)
        } else if t2.is_some() && end_at >= t2_start.unwrap() {
            let base_reward = self.get_base_reward(start_from, t1_start.unwrap())?;
            let t1_reward = t1
                .unwrap()
                .get_reward(t1_start.unwrap(), t2_start.unwrap())?;
            let t2_reward = t2.unwrap().get_reward(t2_start.unwrap(), end_at)?;
            base_reward.try_add(t1_reward)?.try_add(t2_reward)
        } else if t1.is_some() && end_at >= t1_start.unwrap() {
            let base_reward = self.get_base_reward(start_from, t1_start.unwrap())?;
            let t1_reward = t1.unwrap().get_reward(t1_start.unwrap(), end_at)?;
            base_reward.try_add(t1_reward)
        } else {
            // simplest case - only base rate is applicable
            self.get_base_reward(start_from, end_at)
        }
    }?;

    gems.try_mul(per_gem)
}

Yes this looks terrible.

I tried jamming as much as I could into functions such as:

pub fn get_reward(&self, start: u64, end: u64) -> Result<u64, ProgramError> {
    let duration = end.try_sub(start)?;
    self.reward_rate.try_mul(duration)
}

But clearly that's not enough.

Replacing unwrap()s with ? would make it more readable, but I can't do that because there are other functions I'm calling (like try_add) that return an error, and the compiler is telling me I gotta pick one of Option or Result to use ? for.

Can someone with more brain help me develop a cleaner version?

[elsuizo]

Can you share a minimal program that compiles so it's easier to refactor?

[Jcarp]

I find Match statements clean up my code a lot.

You can even assign based on a match as long as you align the types on the branches correctly. I also really don't like unwrap as a single misstep crashes everything. Matches help avoid this as well by making errors on branches more explicit and you can choose how it's handled.

[steffahn]

I’ve mocked up your example so that I can try refactoring

pub struct Foo;
pub struct ProgramError;

#[derive(Clone)]
pub struct Bar;

impl Bar {
    fn get_reward(&self, _: u64, _: u64) -> Result<u64, ProgramError> {
        todo!()
    }
}

trait TryOps: Sized {
    fn try_add(self, other: Self) -> Result<Self, ProgramError>;
    fn try_mul(self, other: Self) -> Result<Self, ProgramError>;
}
impl TryOps for u64 {
    fn try_add(self, _: Self) -> Result<Self, ProgramError> {
        todo!()
    }

    fn try_mul(self, _: Self) -> Result<Self, ProgramError> {
        todo!()
    }
}

impl Foo {
    pub fn extract_tenure(&self, _: &str) -> (Option<u64>, Option<Bar>) {
        todo!()
    }
    pub fn get_base_reward(&self, _: u64, _: u64) -> Result<u64, ProgramError> {
        todo!()
    }
    pub fn reward_amount(
        &self,
        start_from: u64,
        end_at: u64,
        gems: u64,
    ) -> Result<u64, ProgramError> {
        let (t1_start, t1) = self.extract_tenure("t1");
        let (t2_start, t2) = self.extract_tenure("t2");
        let (t3_start, t3) = self.extract_tenure("t3");

        // triage based on starting point on the outside, ending point on the inside
        let per_gem = if t3.is_some() && start_from >= t3_start.unwrap() {
            // simplest case - only t3 rate is applicable
            t3.unwrap().get_reward(start_from, end_at)
        } else if t2.is_some() && start_from >= t2_start.unwrap() {
            if t3.is_some() && end_at >= t3_start.unwrap() {
                let t2_reward = t2.unwrap().get_reward(start_from, t3_start.unwrap())?;
                let t3_reward = t3.unwrap().get_reward(t3_start.unwrap(), end_at)?;
                t2_reward.try_add(t3_reward)
            } else {
                // simplest case - only t2 rate is applicable
                t2.unwrap().get_reward(start_from, end_at)
            }
        } else if t1.is_some() && start_from >= t1_start.unwrap() {
            if t3.is_some() && end_at >= t3_start.unwrap() {
                let t1_reward = t1.unwrap().get_reward(start_from, t2_start.unwrap())?;
                let t2_reward = t2
                    .unwrap()
                    .get_reward(t2_start.unwrap(), t3_start.unwrap())?;
                let t3_reward = t3.unwrap().get_reward(t3_start.unwrap(), end_at)?;
                t1_reward.try_add(t2_reward)?.try_add(t3_reward)
            } else if t2.is_some() && end_at >= t2_start.unwrap() {
                let t1_reward = t1.unwrap().get_reward(start_from, t2_start.unwrap())?;
                let t2_reward = t2.unwrap().get_reward(t2_start.unwrap(), end_at)?;
                t1_reward.try_add(t2_reward)
            } else {
                // simplest case - only t1 rate is applicable
                t1.unwrap().get_reward(start_from, end_at)
            }
        } else {
            if t3.is_some() && end_at >= t3_start.unwrap() {
                let base_reward = self.get_base_reward(start_from, t1_start.unwrap())?;
                let t1_reward = t1
                    .unwrap()
                    .get_reward(t1_start.unwrap(), t2_start.unwrap())?;
                let t2_reward = t2
                    .unwrap()
                    .get_reward(t2_start.unwrap(), t3_start.unwrap())?;
                let t3_reward = t3.unwrap().get_reward(t3_start.unwrap(), end_at)?;
                base_reward
                    .try_add(t1_reward)?
                    .try_add(t2_reward)?
                    .try_add(t3_reward)
            } else if t2.is_some() && end_at >= t2_start.unwrap() {
                let base_reward = self.get_base_reward(start_from, t1_start.unwrap())?;
                let t1_reward = t1
                    .unwrap()
                    .get_reward(t1_start.unwrap(), t2_start.unwrap())?;
                let t2_reward = t2.unwrap().get_reward(t2_start.unwrap(), end_at)?;
                base_reward.try_add(t1_reward)?.try_add(t2_reward)
            } else if t1.is_some() && end_at >= t1_start.unwrap() {
                let base_reward = self.get_base_reward(start_from, t1_start.unwrap())?;
                let t1_reward = t1.unwrap().get_reward(t1_start.unwrap(), end_at)?;
                base_reward.try_add(t1_reward)
            } else {
                // simplest case - only base rate is applicable
                self.get_base_reward(start_from, end_at)
            }
        }?;

        gems.try_mul(per_gem)
    }
}

As far as I can tell, you assume that whenever t1/t2/t3 is Some(…), then t1_start/t2_start/t3_start is Some(…) as-well. This should, in my view, best be expressed in the type signature of extract_tenure itself (assuming that that’s an API you control). Instead of returning -> (Option<u64>, Option<…>), it should return -> Option<(u64, …)>.

Also the some case assume that t2 or t1 can be unwrapped without checking for t2.is_some()/t1.is_some() (which might be okay depending on your program logic).

With that said, you could turn then the ifs into if let

pub struct Foo;
pub struct ProgramError;

pub struct Bar;

impl Bar {
    fn get_reward(&self, _: u64, _: u64) -> Result<u64, ProgramError> {
        todo!()
    }
}

trait TryOps: Sized {
    fn try_add(self, other: Self) -> Result<Self, ProgramError>;
    fn try_mul(self, other: Self) -> Result<Self, ProgramError>;
}
impl TryOps for u64 {
    fn try_add(self, _: Self) -> Result<Self, ProgramError> {
        todo!()
    }

    fn try_mul(self, _: Self) -> Result<Self, ProgramError> {
        todo!()
    }
}

impl Foo {
    pub fn extract_tenure(&self, _: &str) -> Option<(u64, Bar)> {
        todo!()
    }
    pub fn get_base_reward(&self, _: u64, _: u64) -> Result<u64, ProgramError> {
        todo!()
    }
    pub fn reward_amount(
        &self,
        start_from: u64,
        end_at: u64,
        gems: u64,
    ) -> Result<u64, ProgramError> {
        let t1_tenure = self.extract_tenure("t1");
        let t2_tenure = self.extract_tenure("t2");
        let t3_tenure = self.extract_tenure("t3");

        let check_start = |&&(tn_start, _): &_| start_from >= tn_start;
        let check_end = |&&(tn_start, _): &_| start_from >= tn_start;

        // triage based on starting point on the outside, ending point on the inside
        let per_gem = if let Some((_, t3)) = t3_tenure.as_ref().filter(check_start) {
            // simplest case - only t3 rate is applicable
            t3.get_reward(start_from, end_at)
        } else if let Some((_, t2)) = t2_tenure.as_ref().filter(check_start) {
            if let Some((t3_start, t3)) = t3_tenure.as_ref().filter(check_end) {
                let t2_reward = t2.get_reward(start_from, *t3_start)?;
                let t3_reward = t3.get_reward(*t3_start, end_at)?;
                t2_reward.try_add(t3_reward)
            } else {
                // simplest case - only t2 rate is applicable
                t2.get_reward(start_from, end_at)
            }
        } else if let Some((_, t1)) = t1_tenure.as_ref().filter(check_start) {
            if let Some((t3_start, t3)) = t3_tenure.as_ref().filter(check_end) {
                let (t2_start, t2) = t2_tenure.as_ref().unwrap(); // (TODO) maybe explain why this can't fail
                let t1_reward = t1.get_reward(start_from, *t2_start)?;
                let t2_reward = t2.get_reward(*t2_start, *t3_start)?;
                let t3_reward = t3.get_reward(*t3_start, end_at)?;
                t1_reward.try_add(t2_reward)?.try_add(t3_reward)
            } else if let Some((t2_start, t2)) = t2_tenure.as_ref().filter(check_end) {
                let t1_reward = t1.get_reward(start_from, *t2_start)?;
                let t2_reward = t2.get_reward(*t2_start, end_at)?;
                t1_reward.try_add(t2_reward)
            } else {
                // simplest case - only t1 rate is applicable
                t1.get_reward(start_from, end_at)
            }
        } else {
            if let Some((t3_start, t3)) = t3_tenure.as_ref().filter(check_end) {
                let (t1_start, t1) = t1_tenure.as_ref().unwrap(); // (TODO) maybe explain why this can't fail
                let (t2_start, t2) = t2_tenure.as_ref().unwrap(); // (TODO) maybe explain why this can't fail
                let base_reward = self.get_base_reward(start_from, *t1_start)?;
                let t1_reward = t1.get_reward(*t1_start, *t2_start)?;
                let t2_reward = t2.get_reward(*t2_start, *t3_start)?;
                let t3_reward = t3.get_reward(*t3_start, end_at)?;
                base_reward
                    .try_add(t1_reward)?
                    .try_add(t2_reward)?
                    .try_add(t3_reward)
            } else if let Some((t2_start, t2)) = t2_tenure.as_ref().filter(check_end) {
                let (t1_start, t1) = t1_tenure.as_ref().unwrap(); // (TODO) maybe explain why this can't fail
                let base_reward = self.get_base_reward(start_from, *t1_start)?;
                let t1_reward = t1.get_reward(*t1_start, *t2_start)?;
                let t2_reward = t2.get_reward(*t2_start, end_at)?;
                base_reward.try_add(t1_reward)?.try_add(t2_reward)
            } else if let Some((t1_start, t1)) = t1_tenure.as_ref().filter(check_start) {
                let base_reward = self.get_base_reward(start_from, *t1_start)?;
                let t1_reward = t1.get_reward(*t1_start, end_at)?;
                base_reward.try_add(t1_reward)
            } else {
                // simplest case - only base rate is applicable
                self.get_base_reward(start_from, end_at)
            }
        }?;

        gems.try_mul(per_gem)
    }
}

(I hope I got all the start vs end correct)

This is still rather lengthy. It seems like there’s a pattern to this whole thing, but I don't really understand the logic. Maybe it can, somehow, be made more concise and less repetitive? Feel free to explain the logic of these checks and we might be able to help.

[elsuizo]

maybe you could put all the tiers in a array or Vec

pub struct FixedRateSchedule {
    pub base_rate: u64,

    // pub tier1: Option<TierConfig>,
    //
    // pub tier2: Option<TierConfig>,
    //
    // pub tier3: Option<TierConfig>,
    pub tiers: [Option<TierConfig>; 3],
}

And later in the code iterate only over the Somes like this:

fn main() {
    let a = [Some(1), None, Some(2), None, None];
    for element in a.iter().flatten() {
        println!("element: {:?}", element);
    }
}

[quinedot]

This is going to be a pretty sloppy reply on my part.

Here I have modified @steffahn's mock with a few different ideas/observations. The first was to separate out the cases of how many tiers you have, which I do in the method fn per_gem_for_reward. Then each case can be handled separately, which I do in the implementations of PerGem. The pattern for each implementation of PerGem is the same:

• If the last range isn't used, defer to a shorter implementation
• Otherwise figure out how many tailing ranges are needed and if the base case is needed

This is done in an if-else block, but once in this form, it's easy to see that it could be an iterative or folding process.

The approach just described is utilized by reward_amount, as given. The next two approaches are just sketches that I didn't wire all the way up to the rest of the mock.

---

The method per_gem_for_reward investigates how the PerGem implementations would look if combined into one length-agnostic approach. It's still pretty messy, for a few reasons:

• The base range is treated special compared to the others (has it's own method to call)
• The ranges don't know their end positions (the following range's start position)
  • For this you would need some end position value that was a maximum, for the tailing range
• You have to check to be sure there is overlap before calling get_reward

---

The method per_gem_iterator_concept examines what it might look like if the last bullet point above was partially resolved -- that is, if you could call get_reward with an end < start and get something equivalent to 0.

If it were fully resolved, and ranges knew their start and end points, you could not calculate the min and maxes and just pass every range start_from and end_at. If you had the "maximum end position" for the tailing range, you wouldn't need to special case last either. Then it might look something like

    fn per_gem_iterator_concept(/* ... */) -> Result<u64, ProgramError> {
        let base = self.get_base_reward(start_from, end_at);
        tenures.iter().fold(base, |last, this| {
            let this = this.bar.get_reward(start_from, end_at)?;
            last?.try_add(this)
        })
    }

Since we need a initial value for fold anyway, the base range being special isn't really a problem with this approach.

---

I didn't try at all to maximize performance / minimize cost with any of these thought experiments, and instead concentrated on what might be cleaner.

[quinedot]

Here's a slightly less messy version. First we have this guy:

/// A tenure that we can definitely call `get_reward` on
struct HeldTenure {
    start: u64,
    end: u64,
    bar: Bar,
}

impl HeldTenure {
    /// Given a `Bar`, a span `start_from` to `end_at`, and the lower and upper
    /// limits of a `Bar`, return
    /// * `None` if the span is outside the limits, else
    /// * Some(Self) with a start and end corresponding to the span capped by
    ///   the given limits
    fn new(bar: Bar, start_from: u64, end_at: u64, lower: u64, upper: u64) -> Option<Self> {
        let start = std::cmp::max(start_from, lower);
        let end = std::cmp::min(end_at, upper);
        match end < start {
            false => Some(Self { start, end, bar }),
            true  => None,
        }
    }

    //// Apply `Self` by calling `get_reward`    
    fn get_reward(self) -> Result<u64, ProgramError> {
        self.bar.get_reward(self.start, self.end)
    }
}

The purpose is to mostly encapsulate the logic of "should I call this tenure or not". (Note: this is handling logic that could instead be part of get_reward instead.) We pair this with a method on Foo:

    /// Given a tenure key, a span, and the current maximum end of span, attempt
    /// to return an applicable tenure.  If the tenure exists, the maximum end is
    /// adjusted to the beginning of the tenure, even if the tenure is not
    /// applicable to the span.
    fn extract_held_tenure(&self, key: &str, start_from: u64, end_at: u64, max_end: &mut u64) -> Option<HeldTenure> {
        match self.extract_tenure(key) {
            (Some(begin), Some(bar)) => {
                let ht = HeldTenure::new(bar, start_from, end_at, begin, *max_end);
                *max_end = begin;
                ht
            }
            _ => None
        }
    }

The idea is that we call this on our potential tenures from highest to lowest, adjusting the maximum end position as we go (via the mutable reference to max_end).

You can see this in action in the per_gem_for_reward method:

    fn per_gem_for_reward(&self, start_from: u64, end_at: u64) -> Result<u64, ProgramError> {
        // Find applicable tenures from maximum to minimum
        let mut cap = u64::MAX;
        let t3 = self.extract_held_tenure("t3", start_from, end_at, &mut cap);
        let t2 = self.extract_held_tenure("t2", start_from, end_at, &mut cap);
        let t1 = self.extract_held_tenure("t1", start_from, end_at, &mut cap);

        let mut iter = [t1, t2, t3].into_iter().flatten().map(|t| t.get_reward());

        // Base case is either base_reward or first applicable tenure, depending
        // on the start position relative to the first applicable tenure
        let init = match start_from < cap {
            // If iter is empty, cap is MAX.  So this can only panic if start_from
            // is also MAX.  Optionally, check for start_from == MAX and return
            // `Err(ProgramError)` instead.
            false => iter.next().unwrap(),
            true => self.get_base_reward(start_from, std::cmp::min(cap, end_at)),
        };
        
        iter.fold(init, |last, this| last?.try_add(this?))
    }

A capturing closure could replace extract_held_tenure for this first half of this method, but I thought this was cleaner, though it does involve passing a lot of parameters.

As you can see in the second half of the method, we still have to special-case the base reward. If the base reward could act like a tenure, it could instead be

let init = iter.next().unwrap(); // Or optionally return `Err(...)`

Arguably it would be better to defer calling get_reward() to the initialization and fold steps, rather than as part of iter. But the eager call only pessimizes the unhappy path, assuming no side effects.

---

Thinking about how this made things cleaner,

• The return-tuple-of-Options interface is pretty awkward, and the returned values don't even fully encapsulate the information needed to know if the tenure is applicable (as there is no end value). HeldTenure consolidates the multiple Options and the applicability into a single Option.
• The if tree was turned into an iterative process by
  • holding some state (max end position) and trying each tenure in order (updating the state)
  • and recognizing that calculating the result was iterative too (fold)
• The remaining Options were handled by using flatten
  • If get_reward could be called with end < start and behave, these Options could be eliminated. Basically the check is deferred.

As a bonus, it's easy to extend with a "t4", say. (Or an arbitrary amount, if you key on something like an integer instead of a str.)

Things would be cleaner yet if extract_tenure knew the upper bound of the tenures and returned something like Option<HeldTenure> (which may or may not be applicable) itself, and if the base reward could be a tenure too. Playground. So if the set of tenures changes less often than rewards are calculated, I recommend investigating that approach.

[ilmoi]

@quinedot

Here's a slightly less messy version.

Thank you SO MUCH for writing this up! I just can't help being surprised by this magical place called internet where someone you never met is willing to give up their time and energy to teach you stuff:D I really appreciate it.

Now the code.

I think it's a beautiful solution - and I was able to glue it in, and further simplify given the rest of the codebase.

You sort of put into words (code?) something that was sitting at the back of my head - I should be using min/max to estimate "actual" aka definitive periods and applying the rate to each one.

I definitely didn't have the rust-fu you showed in the end with flattens and iters and folds - so that was an added learning.

Thanks again - this goes to everyone who contributed here.