Generic iterator type as reference

Hey everyone :slight_smile:

I have written this code:

use std::collections::HashMap;

pub struct PairMatcher<T, I>
where
    T: PartialEq,
    I: IntoIterator<Item = T>,
{
    pairs: HashMap<T, T>,
    search_input: I,
}

impl<T, I> PairMatcher<T, I>
where
    T: PartialEq,
    I: IntoIterator<Item = T>,
{
    pub fn new(pairs: HashMap<T, T>, search_input: I) -> Self {
        Self {
            pairs,
            search_input,
        }
    }

    pub fn count_pairs(self) -> Option<usize> {
        let mut stack = vec![];
        let mut counter = 0;

        let left_elements = Vec::from_iter(self.pairs.keys());
        let right_elements = Vec::from_iter(self.pairs.values());

        for element in self.search_input {
            if left_elements.contains(&&element) {
                stack.push(element)
            } else if right_elements.contains(&&element) {
                match stack.pop() {
                    Some(_) => counter += 1,
                    None => return None,
                }
            }
        }

        if !stack.is_empty() {
            return None;
        }
        Some(counter)
    }

    pub fn has_matching_pairs(self) -> bool {
        match self.count_pairs() {
            Some(_) => true,
            None => false,
        }
    }
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_simple_str_match() {
        let pairs = HashMap::from([('{', '}')]);
        let pair_matcher = PairMatcher::new(pairs, "{}{}".chars());
        assert_eq!(pair_matcher.count_pairs(), Some(2));
    }

    #[test]
    fn test_simple_int_match() {
        let pairs = HashMap::from([(1, 2)]);
        let pair_matcher = PairMatcher::new(pairs, vec![1, 2, 3, 4, 5, 1, 2, 56, 1, 2].into_iter());
        assert_eq!(pair_matcher.count_pairs(), Some(3));
    }

    #[test]
    fn test_failed_match() {
        let pairs = HashMap::from([(1, 2)]);
        let pair_matcher = PairMatcher::new(pairs, vec![1, 2, 3, 4, 5, 1, 2, 56, 1].into_iter());
        assert!(!pair_matcher.has_matching_pairs())
    }

    #[test]
    fn test_complex_match() {
        let pairs = HashMap::from([('{', '}')]);
        let pair_matcher = PairMatcher::new(pairs, "{{{{{{{{}{}{}{}{}{{}}}}}}}}{}{}{}}".chars());
        assert!(pair_matcher.has_matching_pairs())
    }
}

and it works. but the code looks quite ugly in my opinion.

Also I would like to have count_pairs to have &self and not self, so it does not move search_input.
I don't seem to find a way for it.

Any help would be appreciated. I really want to get better at rust.

Making count_pairs is pretty simple, you were already passing an iterator directly in your tests anyway. Most of std's iterators are cheap to clone, so just making I: Iterator<Item = T> + Clone instead is an easy fix

use std::collections::HashMap;

// It's usually best to avoid placing trait bounds on the struct itself when you can avoid it.
pub struct PairMatcher<T, I> {
    pairs: HashMap<T, T>,
    search_input: I,
}

impl<T, I> PairMatcher<T, I>
where
    T: PartialEq,
    I: Iterator<Item = T> + Clone,
{
    pub fn new(pairs: HashMap<T, T>, search_input: I) -> Self {
        Self {
            pairs,
            search_input,
        }
    }

    pub fn count_pairs(&self) -> Option<usize> {
        let mut stack = vec![];
        let mut counter = 0;

        let left_elements = Vec::from_iter(self.pairs.keys());
        let right_elements = Vec::from_iter(self.pairs.values());

        for element in self.search_input.clone() {
            if left_elements.contains(&&element) {
                stack.push(element)
            } else if right_elements.contains(&&element) {
                match stack.pop() {
                    Some(_) => counter += 1,
                    None => return None,
                }
            }
        }

        if !stack.is_empty() {
            return None;
        }
        Some(counter)
    }

    pub fn has_matching_pairs(self) -> bool {
        match self.count_pairs() {
            Some(_) => true,
            None => false,
        }
    }
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_simple_str_match() {
        let pairs = HashMap::from([('{', '}')]);
        let pair_matcher = PairMatcher::new(pairs, "{}{}".chars());
        assert_eq!(pair_matcher.count_pairs(), Some(2));
    }

    #[test]
    fn test_simple_int_match() {
        let pairs = HashMap::from([(1, 2)]);
        let pair_matcher = PairMatcher::new(pairs, vec![1, 2, 3, 4, 5, 1, 2, 56, 1, 2].into_iter());
        assert_eq!(pair_matcher.count_pairs(), Some(3));
    }

    #[test]
    fn test_failed_match() {
        let pairs = HashMap::from([(1, 2)]);
        let pair_matcher = PairMatcher::new(pairs, vec![1, 2, 3, 4, 5, 1, 2, 56, 1].into_iter());
        assert!(!pair_matcher.has_matching_pairs())
    }

    #[test]
    fn test_complex_match() {
        let pairs = HashMap::from([('{', '}')]);
        let pair_matcher = PairMatcher::new(pairs, "{{{{{{{{}{}{}{}{}{{}}}}}}}}{}{}{}}".chars());
        assert!(pair_matcher.has_matching_pairs())
    }
}

What specifically seems ugly to you?

1 Like

Thanks for your help.

For me the double references look ugly:
image

Also I was looking for a way to not clone self.search_input in the the for loop.
But rather have for element in &self.search_input. But i couldn't make the compiler happy.

Also i think the whole problem could probably be solved in a more "rust" way.
But i am too much of a rust noob to see it :sweat_smile:. One day. But I need more practice

My code is cloning the iterator not the string, that means it's copying 2 pointers and not making a copy of the whole string

Note that for vec![1, 2, 3, 4, 5, 1, 2, 56, 1].into_iter(), the iterator will own the allocation, so cloning it will result in a new allocation.

Um, okay? Such low-level syntactic details certainly don't count towards code beauty according to the overwhelming majority of experienced programmers. You can basically just… live with the double reference.

For me its not ugly in the sense that i wouldn't write it. It's ugly because I think there could be a better way to solve this. I am just too much of a noob in rust

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.