rust icon indicating copy to clipboard operation
rust copied to clipboard

CustomSet exercise allows creating new `CustomSet`s with duplicated members

Open mnquintana opened this issue 4 years ago • 1 comments

👋🏽 I came across an edge case that (I think) currently isn't being caught by the test cases for CustomSet – it's possible to write a CustomSet::new constructor that internally stores set members with duplicates, e.g.:

#[derive(Clone, Debug)]
pub struct CustomSet<T> {
    members: Vec<T>,
}

impl<T: Clone + PartialEq> CustomSet<T> {
    pub fn new(input: &[T]) -> Self {
        let members = input.to_vec();
        Self { members }
    }
}

On the one hand, I don't think the internal representation matters that much as long as the CustomSets methods account for duplicates.

But, after a cursory glance at the test cases, it looks like there aren't any test cases that instantiate a CustomSet from a slice with duplicate elements, which if I'm reading them right means a big chunk of a Set's functionality is currently untested (and the test suite passes solutions that don't actually dedupe members).

Example test case:

#[test]
fn intersection_of_two_sets_with_no_shared_elements_is_an_empty_set() {
    let set1 = CustomSet::new(&[1, 3, 2, 3, 3]);
    let set2 = CustomSet::new(&[4, 5, 6]);
    assert_eq!(set1.intersection(&set2), CustomSet::new(&[]));
    assert_eq!(set2.intersection(&set1), CustomSet::new(&[]));
}

Would y'all be open to a PR that adds test cases that test for CustomSet's deduping ability by passing in slices with duplicate elements?

mnquintana avatar Jun 05 '21 16:06 mnquintana

I think that a deduplication test is a great idea! I'd be very open to a pr adding one.

On Sat, Jun 5, 2021, 18:06 Machisté N. Quintana @.***> wrote:

👋🏽 I came across an edge case that (I think) currently isn't being caught by the test cases for CustomSet https://exercism.io/tracks/rust/exercises/custom-set – it's possible to write a CustomSet::new constructor that internally stores set members with duplicates, e.g.:

#[derive(Clone, Debug)] pub struct CustomSet<T> {

members: Vec<T>,

}

impl<T: Clone + PartialEq> CustomSet<T> {

pub fn new(input: &[T]) -> Self {

    let members = input.to_vec();

    Self { members }

}

}

On the one hand, I don't think the internal representation matters that much as long as the CustomSets methods account for duplicates.

But, after a cursory glance at the test cases https://github.com/exercism/rust/blob/main/exercises/practice/custom-set/tests/custom-set.rs, it looks like there aren't any test cases that instantiate a CustomSet from a slice with duplicate elements, which if I'm reading them right means a big chunk of a Set's functionality is currently untested (and the test suite passes solutions that don't actually dedupe members).

Would y'all be open to a PR that adds test cases that test for CustomSet's deduping ability by passing in slices with duplicate elements?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/exercism/rust/issues/1275, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB3V4TQTAJADFWZK3YBXGZLTRJDSDANCNFSM46EW7FYA .

coriolinus avatar Jun 05 '21 17:06 coriolinus

should be fixed by https://github.com/exercism/problem-specifications/pull/2324

senekor avatar Sep 16 '23 20:09 senekor