Iterator extension AllTheSame

code review
3

Here’s an alternative way of doing the implementation that I could come up with

let first = self.next().ok_or_else(HashSet::new)?;
match self.find(|other| *other != first) {
    None => Ok(first),
    Some(first_other) => Err(HashSet::from_iter(
        [first, first_other].into_iter().chain(self),
    )),
}

I went for a more ”functional” approach here, which defines the return values in one step in each case, not build them up in multiple mutations.

Beyond a different style (which is just a matter of taste), a small advantage I could find is that you don't loop over the whole list doing for every item 2 steps, first checking other != first and then doing the HashSet::insert, but instead the hashset-lookup can handle both at once more efficiently if first was already part of the set.

Some things I like about your code already: It makes sure to have consistent behavior for choosing the representative of equal items (it seems HashSet always keeps the first value if multiple are equal, and your code also keeps the first value of all the ones equal to first). It also effectively makes sure that no hashset is allocated unless there’s at least 2 different items.

A general possible issue with this function of yours is that if one isn’t interested in the Err variant, the function does additional work beyond determining whether all items are the same (in which case it could return earlier). That being said, that’s mainly a naming and/or documentation issue, and usage of Result where the Err variant is considered important enough to be fed with additional “expensive”-to-compute information is completely fine, as long as the user is aware and consider the cost insignificant or are aware that they should use different API if they aren’t interested in the Err value.

Some more variation on my code: With a free-standing chain function, like perhaps eventually part of std and currently part of itertools, the expression of [first, first_other].into_iter().chain(self) can become a bit nicer as chain([first, first_other], self). The usage of HashSet::from_iter can also be replaced with a call to .collect(), the latter is shorter, the former more explicitly makes the type visible.

3 Likes