Set each option exactly once, exit early if twice

I want to iterate over many items and find a few specific values. Each value I'm looking for should be found exactly once. If any item is missing or is found multiple times, the function should return None. If possible, I'd like to only iterate over the items once instead of once per value I'm looking for. I expect there to be other items that don't correspond to the values I'm looking for, which is fine.

Right now, my code looks something like this:

fn parse(items: &[Item]) -> Option<Stuff> {
    let mut foo = None;
    let mut bar = None;
    let mut baz = None;
    for item in items {
        match item.id.as_ref() {
            "foo" => {
                if foo.replace(item.stuff).is_some() {
                    return None;
                }
            },
            "bar" => {
                if bar.replace(item.stuff).is_some() {
                    return None;
                }
            },
            "baz" => {
                if baz.replace(item.stuff).is_some() {
                    return None;
                }
            },
            _ => return None,
        }
    }
    Some(Stuff {
        foo: foo?,
        bar: bar?,
        baz: baz?,
    })
}

There's a ton of repeated code in here, but I can't think of a good way to reduce it. Since I have an if statement that exits early with None, I feel like I should be able to use the ? operator. However, my use case is kind of backward from normal—instead of returning early if Option::replace returns None, I want to return early if Option::replace returns Some. I guess I could do something like this?

"foo" => foo.replace(item.stuff).xor(Some(Stuff::default()))?

But that requires creating a dummy value just for the ease of using the ? operator, which probably isn't worth it.

I'm sure I could do this with a macro, but this doesn't seem like it's worth a macro. Plus, that reduces readability as well.

Any suggestions for a clean, idiomatic, readable version of this code? Thanks! :smiley:

1 Like

Looks like what you want is a map, with a check at insert whether the key already exists.

1 Like

I have another attempt. Now, I'm creating the variables like I was before, but then stuffing them into an array that I can loop over for every possible variable for each item. If the check matches, I can use the mutable reference to store the value and also check if it's already been set.

fn parse(items: &[Item]) -> Option<Stuff> {
    let mut foo = None;
    let mut bar = None;
    let mut baz = None;
    let item_infos = [
        (&mut foo, "foo"/*, more validation parameters */),
        (&mut bar, "bar"/*, more validation parameters */),
        (&mut baz, "baz"/*, more validation parameters */),
    ];
    for item in items {
        for (item_ref, id) in item_infos {
            if item.id != id {
                continue;
            }
            if item_ref.replace(item.stuff).is_some() /* More validation */ {
                return None;
            }
            break;
        }
    }
    Some(Stuff {
        foo: foo?,
        bar: bar?,
        baz: baz?,
    })
}

@jdahlstrom That's not a bad idea, although I'd prefer to keep things as separate variables, though, because I'm planning on putting them into a struct (see the construction of the Stuff struct at the end of the function). Thanks for your input!

You could just straight-up assign the value and keep track of how many times you do so...

    for item in items {
        match item.id.as_ref() {
            "foo" => { foo = Some(item.stuff); count += 1; }
            "bar" => { bar = Some(item.stuff); count += 1; }
            "baz" => { baz = Some(item.stuff); count += 1; }
            _ => {}
        }
        if count == 3 { break; }
    }

...and when you reach this point, if there have been duplicates, at least one of the variables will be None.

    Some(Stuff {
        foo: foo?,
        bar: bar?,
        baz: baz?,
    })

Your latest post makes it sound like there's more complicated checks going on though, which makes me wonder if a builder pattern would be more appropriate.

#[derive(Default)]
struct StuffBuilder { /* ... */ }

impl StuffBuilder {
    // Returns an error if
    // - Already hydrated
    // - ...
    fn hydrate(stuff: Stuff) -> Result<(), &'static str> {
        // ...
    }
    
    // Returns an error if
    // - Not hydrated
    // - ...
    fn into_stuff() -> Result<Stuff, &'static str> {
        // ...
    }
}

fn parse(items: &[Item]) -> Option<Stuff2> {
    let mut foo = StuffBuilder::default();
    for item in items {
        match item.id.as_ref() {
            "foo" => foo.hydrate(item.stuff).ok()?,
        }
    }
    Some(Stuff2 {
        foo: foo.into_stuff().ok()?,
    })
}

Your array approach is basically a variation on a HashMap<&'static str, &mut Option<_>> approach.

1 Like

You could also write an extension trait with a method that contains the repeated code.

I'll be that guy: &[Item] is an unfitting data structure for your problem,it would become a lot easier it it were a HashMap<&'static str, Item>. But, if for whatever reason you can't change it, I propose a solution using OnceCell:

fn parse(items: &[Item]) -> Option<Stuff> {
    let foo = OnceCell::new();
    let bar = OnceCell::new();
    let baz = OnceCell::new();
    for item in items {
        match item.id.as_ref() {
            "foo" => foo.set(item.stuff).ok()?,
            "bar" => foo.set(item.stuff).ok()?,
            "baz" => foo.set(item.stuff).ok()?,
            _ => return None,
        }
    }
    Some(Stuff {
        foo: foo.into_inner()?,
        bar: bar.into_inner()?,
        baz: baz.into_inner()?,
    })
}

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.