2
\$\begingroup\$

When I was working in Rust this week, I missed 's fun <T> Sequence<T>.single(): T. Basically, calling this function means that you are asserting that the sequence contains only one element and that you want that one element back. The most common use case is you've filtered a sequence down to one element. It's also clearer than using first() for the same task as it is clear that there is only supposed to be one thing and that if there is more than one you need to crash because your assumptions don't hold anymore.

I wanted it in Rust, so I implemented it and put it on crates.io. Before I go 1.0.0, however, I'd like to ask for review, mostly focusing on usability of the crate (thus design) and style of the code.

//! This crate exposes a `Single` trait for extracting the element from a
//! single-element iterator (or `panic!`ing if that precondition is false).

use std::{fmt, error, result};

type Result<T> = result::Result<T, self::Error>;

/// Trait to extract the element from a single-element iterator.
pub trait Single {
    /// The item type of the wrapped iterator.
    type Item;

    /// Get the single element from a single-element iterator.
    ///
    /// Note that many iterators return references to the elements,
    /// so this method will as well if the backing iterator does.
    ///
    /// # Examples
    ///
    /// ```
    /// # use single::{ self, Single };
    /// # use std::iter;
    /// assert_eq!(iter::empty::<i32>().single(), Err(single::Error::NoElements));
    /// assert_eq!(iter::once(0).single(), Ok(0));
    /// assert_eq!(iter::repeat(0).single(), Err(single::Error::MultipleElements));
    /// ```
    fn single(self) -> Result<Self::Item>;
}

/// An error in the execution of [`single::Single::single`](trait.Single.html#tymethod.single).
#[derive(Clone, Copy, PartialEq, Eq)]
pub enum Error {
    /// Asked empty iterator for single element.
    NoElements,
    /// Asked iterator with multiple elements for single element.
    MultipleElements,
}

impl fmt::Debug for Error {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        fmt::Display::fmt(self, f)
    }
}

impl fmt::Display for Error {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        match *self {
            Error::NoElements => write!(
                f,
                "SingleError::NoElements: {}",
                error::Error::description(self),
            ),
            Error::MultipleElements => write!(
                f,
                "SingleError::MultipleElements: {}",
                error::Error::description(self),
            ),
        }
    }
}

impl error::Error for Error {
    fn description(&self) -> &str {
        match *self {
            Error::NoElements => "Asked empty iterator for single element",
            Error::MultipleElements => "Asked iterator with multiple elements for single element",
        }
    }
    fn cause(&self) -> Option<&error::Error> { None }
}

impl<I> Single for I where I: Iterator {
    type Item = <Self as Iterator>::Item;

    fn single(mut self) -> Result<Self::Item> {
        match self.next() {
            None => Err(Error::NoElements),
            Some(element) => if self.next().is_none() {
                Ok(element)
            } else {
                Err(Error::MultipleElements)
            }
        }
    }
}

#[cfg(test)]
mod test {
    use std::iter;
    use super::Single;

    #[test]
    #[should_panic(expected = "Asked empty iterator for single element")]
    fn panic_empty() { let _: i32 = iter::empty().single().unwrap(); }

    #[test]
    #[should_panic(expected = "Asked iterator with multiple elements for single element")]
    fn panic_multiple() { let _ = iter::repeat(0).single().unwrap(); }
}

Future growth by optimization(?) with std::iter::ExactSizeIterator and/or std::iter::TrustedLen may be possible in the future, but would require benchmarking to see if it is worthwhile.

\$\endgroup\$
1
\$\begingroup\$
  1. "`panic!`ing" would normally be written as "panicking". Strangely, it doesn't appear your code actually... panics though? Seems your docs are misleading or outdated.

  2. I dislike the common repetition in documentation for an X that starts with an equivalent of "The X is". Try removing those and improving the information density of the docs. We can already see it's an X from the doc tool itself.

  3. Since the crate is so narrowly focused, I'd recommend moving at least one (if not all) the examples from the method to the crate level. This allows users to immediately see what it does and how to use it.

  4. There's no reason to use self:: in the custom Result.

  5. There's never a time where you'd implement this trait for anything but an iterator, so your trait should require Iterator as a supertrait. This has the benefit of unifying the Item types.

  6. The note about "many iterators return references to the elements", seems out-of-place; that's just how iterators work. If someone doesn't understand that, it seems like they should re-read The Rust Programming Language. For similar reasons, I wouldn't explain how to use a trait or how to add dependencies to the Cargo.toml in my crate docs.

  7. Avoid referencing your crate name in the doc prose. A user can choose to import it with whatever name they like.

  8. Consider a nested match in the iterator implementation.

  9. The majority of your code appears to be dealing with the error type. Perhaps a crate like quick-error would help tip the balance back towards the interesting code.

  10. It's strange to have Display and Debug be the same value. Recall that Debug is for programmers; we'd expect to see the variant name. Display is for humans that see the error.

  11. I'd delegate your Display implementation to Error::description as adding the names of the variants doesn't help the humans reading it.

  12. I'm not a fan of multi-line expressions that aren't blocks in a match (like your write! or if). Would promote them to blocks.

//! Provides the `Single` trait for extracting the element from a
//! single-element iterator.

use std::{fmt, error, result};

type Result<T> = result::Result<T, Error>;

/// Extract the element from a single-element iterator.
pub trait Single: Iterator {
    /// Get the single element from a single-element iterator.
    ///
    /// # Examples
    ///
    /// ```
    /// # use single::{ self, Single };
    /// # use std::iter;
    /// assert_eq!(iter::empty::<i32>().single(), Err(single::Error::NoElements));
    /// assert_eq!(iter::once(0).single(), Ok(0));
    /// assert_eq!(iter::repeat(0).single(), Err(single::Error::MultipleElements));
    /// ```
    fn single(self) -> Result<Self::Item>;
}

/// An error in the execution of [`Single::single`](trait.Single.html#tymethod.single).
#[derive(Clone, Copy, PartialEq, Eq)]
pub enum Error {
    /// Asked empty iterator for single element.
    NoElements,
    /// Asked iterator with multiple elements for single element.
    MultipleElements,
}

impl fmt::Debug for Error {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        fmt::Display::fmt(self, f)
    }
}

impl fmt::Display for Error {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        match *self {
            Error::NoElements => {
                write!(
                    f, "SingleError::NoElements: {}",
                    error::Error::description(self),
                )
            }
            Error::MultipleElements => {
                write!(
                    f, "SingleError::MultipleElements: {}",
                    error::Error::description(self),
                )
            }
        }
    }
}

impl error::Error for Error {
    fn description(&self) -> &str {
        match *self {
            Error::NoElements =>
                "Asked empty iterator for single element",
            Error::MultipleElements =>
                "Asked iterator with multiple elements for single element",
        }
    }
    fn cause(&self) -> Option<&error::Error> { None }
}

impl<I> Single for I where I: Iterator {
    fn single(mut self) -> Result<Self::Item> {
        match self.next() {
            None => Err(Error::NoElements),
            Some(element) => {
                match self.next() {
                    None => Ok(element),
                    Some(_) => Err(Error::MultipleElements),
                }
            }
        }
    }
}

#[cfg(test)]
mod test {
    use std::iter;
    use super::Single;

    #[test]
    #[should_panic(expected = "Asked empty iterator for single element")]
    fn panic_empty() { let _: i32 = iter::empty().single().unwrap(); }

    #[test]
    #[should_panic(expected = "Asked iterator with multiple elements for single element")]
    fn panic_multiple() { let _ = iter::repeat(0).single().unwrap(); }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Outdated. The original version had a more aggressive contract which replaced Kotlin's exception with a panic instead of using the Result wrapper. Obviously the move to using the result type leads to much more composable code :D (Thanks for the notes; I agree with everything you've said and crated up version 0.2.1 with what I think are better error messages as well. 1.0.0 will come when I've had some use-time with the crate and I'm sure I like it.) \$\endgroup\$ – CAD97 Jun 11 '17 at 2:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.