From 32e76e974aed515ebdae9caae69a3a4a623f6d45 Mon Sep 17 00:00:00 2001 From: Peter Tseng Date: Fri, 20 Nov 2020 05:28:07 +0000 Subject: [PATCH 1/2] simple-linked-list, doubly-linked-list: Add is_empty https://rust-lang.github.io/rust-clippy/master/index.html#len_without_is_empty This alleges that for certain types, len() may be expensive, but is_empty() is likely to always be cheap, and therefore it's good custom to have both. I find this sufficiently persuasive. On the principle that the track instill good habits in students, we will add and test such a method. I understand that for the example solutions, len() is actually cheap. In addition, I understand that for a list it's always possible to be able to cheaply determine the `len()` simply by keeping an extra field that gets updated on every insert. Nevertheless I think it's a good idea anyway. Helps address https://github.com/exercism/rust/pull/1011 Helps address https://github.com/exercism/rust/pull/1012 --- .../doubly-linked-list/.meta/description.md | 2 +- exercises/doubly-linked-list/README.md | 2 +- exercises/doubly-linked-list/example.rs | 4 +++ exercises/doubly-linked-list/src/lib.rs | 9 ++++++ .../tests/doubly-linked-list.rs | 23 +++++++++++-- exercises/simple-linked-list/example.rs | 4 +++ exercises/simple-linked-list/src/lib.rs | 9 ++++++ .../tests/simple-linked-list.rs | 32 +++++++++++++++++++ 8 files changed, 81 insertions(+), 4 deletions(-) diff --git a/exercises/doubly-linked-list/.meta/description.md b/exercises/doubly-linked-list/.meta/description.md index 7313bac60..f5de1952f 100644 --- a/exercises/doubly-linked-list/.meta/description.md +++ b/exercises/doubly-linked-list/.meta/description.md @@ -33,7 +33,7 @@ private functions. Implement the functionality for adding and removing elements (pushing and popping) at the front and back. This is enough to use the list as a double-ended queue. -Also implement the `len` function. +Also implement the `len` and `is_empty` functions. In the finished implementation, all modifications of the list should be done through the cursor struct to minimize duplication. The `push_*` and `pop_*` methods on `LinkedList` diff --git a/exercises/doubly-linked-list/README.md b/exercises/doubly-linked-list/README.md index 865c967ae..343a0a5ea 100644 --- a/exercises/doubly-linked-list/README.md +++ b/exercises/doubly-linked-list/README.md @@ -35,7 +35,7 @@ private functions. Implement the functionality for adding and removing elements (pushing and popping) at the front and back. This is enough to use the list as a double-ended queue. -Also implement the `len` function. +Also implement the `len` and `is_empty` functions. In the finished implementation, all modifications of the list should be done through the cursor struct to minimize duplication. The `push_*` and `pop_*` methods on `LinkedList` diff --git a/exercises/doubly-linked-list/example.rs b/exercises/doubly-linked-list/example.rs index a46dd40ea..a9d83f9b2 100644 --- a/exercises/doubly-linked-list/example.rs +++ b/exercises/doubly-linked-list/example.rs @@ -77,6 +77,10 @@ impl LinkedList { } } + pub fn is_empty(&self) -> bool { + self.len == 0 + } + pub fn len(&self) -> usize { self.len } diff --git a/exercises/doubly-linked-list/src/lib.rs b/exercises/doubly-linked-list/src/lib.rs index 68a82afc5..f9e01820e 100644 --- a/exercises/doubly-linked-list/src/lib.rs +++ b/exercises/doubly-linked-list/src/lib.rs @@ -14,6 +14,15 @@ impl LinkedList { unimplemented!() } + // You may be wondering why it's necessary to have is_empty() + // when it can easily be determined from len(). + // It's good custom to have both because len() can be expensive for some types, + // whereas is_empty() is almost always cheap. + // (Also ask yourself whether len() is expensive for LinkedList) + pub fn is_empty(&self) -> bool { + unimplemented!() + } + pub fn len(&self) -> usize { unimplemented!() } diff --git a/exercises/doubly-linked-list/tests/doubly-linked-list.rs b/exercises/doubly-linked-list/tests/doubly-linked-list.rs index 9993c78ae..366c76b8e 100644 --- a/exercises/doubly-linked-list/tests/doubly-linked-list.rs +++ b/exercises/doubly-linked-list/tests/doubly-linked-list.rs @@ -15,6 +15,7 @@ fn is_generic() { fn basics_empty_list() { let list: LinkedList = LinkedList::new(); assert_eq!(list.len(), 0); + assert!(list.is_empty()); } // push / pop at back ———————————————————————————————————————— @@ -25,7 +26,11 @@ fn basics_single_element_back() { list.push_back(5); assert_eq!(list.len(), 1); + assert!(!list.is_empty()); + assert_eq!(list.pop_back(), Some(5)); + + assert!(list.is_empty()); } #[test] @@ -34,13 +39,15 @@ fn basics_push_pop_at_back() { let mut list: LinkedList = LinkedList::new(); for i in 0..10 { list.push_back(i); + assert!(!list.is_empty()); } assert_eq!(list.len(), 10); - for i in (0..10).rev() { + assert!(!list.is_empty()); assert_eq!(i, list.pop_back().unwrap()); } assert_eq!(list.len(), 0); + assert!(list.is_empty()); } // push / pop at front ——————————————————————————————————————— @@ -51,7 +58,11 @@ fn basics_single_element_front() { list.push_front(5); assert_eq!(list.len(), 1); + assert!(!list.is_empty()); + assert_eq!(list.pop_front(), Some(5)); + + assert!(list.is_empty()); } #[test] @@ -60,13 +71,15 @@ fn basics_push_pop_at_front() { let mut list: LinkedList = LinkedList::new(); for i in 0..10 { list.push_front(i); + assert!(!list.is_empty()); } assert_eq!(list.len(), 10); - for i in (0..10).rev() { + assert!(!list.is_empty()); assert_eq!(i, list.pop_front().unwrap()); } assert_eq!(list.len(), 0); + assert!(list.is_empty()); } // push / pop at mixed sides ————————————————————————————————— @@ -76,10 +89,13 @@ fn basics_push_front_pop_back() { let mut list: LinkedList = LinkedList::new(); for i in 0..10 { list.push_front(i); + assert!(!list.is_empty()); } for i in 0..10 { + assert!(!list.is_empty()); assert_eq!(i, list.pop_back().unwrap()); } + assert!(list.is_empty()); } #[test] @@ -88,10 +104,13 @@ fn basics_push_back_pop_front() { let mut list: LinkedList = LinkedList::new(); for i in 0..10 { list.push_back(i); + assert!(!list.is_empty()); } for i in 0..10 { + assert!(!list.is_empty()); assert_eq!(i, list.pop_front().unwrap()); } + assert!(list.is_empty()); } // ——————————————————————————————————————————————————————————— diff --git a/exercises/simple-linked-list/example.rs b/exercises/simple-linked-list/example.rs index 878d0c01a..9c3433fea 100644 --- a/exercises/simple-linked-list/example.rs +++ b/exercises/simple-linked-list/example.rs @@ -15,6 +15,10 @@ impl SimpleLinkedList { SimpleLinkedList { head: None, len: 0 } } + pub fn is_empty(&self) -> bool { + self.len == 0 + } + pub fn len(&self) -> usize { self.len } diff --git a/exercises/simple-linked-list/src/lib.rs b/exercises/simple-linked-list/src/lib.rs index ebd7a1e34..8131c1529 100644 --- a/exercises/simple-linked-list/src/lib.rs +++ b/exercises/simple-linked-list/src/lib.rs @@ -11,6 +11,15 @@ impl SimpleLinkedList { unimplemented!() } + // You may be wondering why it's necessary to have is_empty() + // when it can easily be determined from len(). + // It's good custom to have both because len() can be expensive for some types, + // whereas is_empty() is almost always cheap. + // (Also ask yourself whether len() is expensive for SimpleLinkedList) + pub fn is_empty(&self) -> bool { + unimplemented!() + } + pub fn len(&self) -> usize { unimplemented!() } diff --git a/exercises/simple-linked-list/tests/simple-linked-list.rs b/exercises/simple-linked-list/tests/simple-linked-list.rs index 28141a5c1..f190ea678 100644 --- a/exercises/simple-linked-list/tests/simple-linked-list.rs +++ b/exercises/simple-linked-list/tests/simple-linked-list.rs @@ -28,6 +28,38 @@ fn test_pop_decrements_length() { assert_eq!(list.len(), 0, "list's length must be 0"); } +#[test] +#[ignore] +fn test_is_empty() { + let mut list: SimpleLinkedList = SimpleLinkedList::new(); + assert!(list.is_empty(), "List wasn't empty on creation"); + for inserts in 0..100 { + for i in 0..inserts { + list.push(i); + assert!( + !list.is_empty(), + "List was empty after having inserted {}/{} elements", + i, + inserts + ); + } + for i in 0..inserts { + assert!( + !list.is_empty(), + "List was empty before removing {}/{} elements", + i, + inserts + ); + list.pop(); + } + assert!( + list.is_empty(), + "List wasn't empty after having removed {} elements", + inserts + ); + } +} + #[test] #[ignore] fn test_pop_returns_head_element_and_removes_it() { From 553d2c151396a7d4678e0dae1f0deb885ae4e865 Mon Sep 17 00:00:00 2001 From: Peter Tseng Date: Fri, 20 Nov 2020 05:30:16 +0000 Subject: [PATCH 2/2] doubly-linked-list: Test len in more places I simply thought we could increase the thoroughness without being an undue burden on readability or student completion. This is limited to testing len in additional places in tests that were already testing len or is_empty. It hasn't added len in tests that didn't already have either of those two. --- .../doubly-linked-list/tests/doubly-linked-list.rs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/exercises/doubly-linked-list/tests/doubly-linked-list.rs b/exercises/doubly-linked-list/tests/doubly-linked-list.rs index 366c76b8e..101ef6c21 100644 --- a/exercises/doubly-linked-list/tests/doubly-linked-list.rs +++ b/exercises/doubly-linked-list/tests/doubly-linked-list.rs @@ -30,6 +30,7 @@ fn basics_single_element_back() { assert_eq!(list.pop_back(), Some(5)); + assert_eq!(list.len(), 0); assert!(list.is_empty()); } @@ -39,10 +40,11 @@ fn basics_push_pop_at_back() { let mut list: LinkedList = LinkedList::new(); for i in 0..10 { list.push_back(i); + assert_eq!(list.len(), i as usize + 1); assert!(!list.is_empty()); } - assert_eq!(list.len(), 10); for i in (0..10).rev() { + assert_eq!(list.len(), i as usize + 1); assert!(!list.is_empty()); assert_eq!(i, list.pop_back().unwrap()); } @@ -62,6 +64,7 @@ fn basics_single_element_front() { assert_eq!(list.pop_front(), Some(5)); + assert_eq!(list.len(), 0); assert!(list.is_empty()); } @@ -71,10 +74,11 @@ fn basics_push_pop_at_front() { let mut list: LinkedList = LinkedList::new(); for i in 0..10 { list.push_front(i); + assert_eq!(list.len(), i as usize + 1); assert!(!list.is_empty()); } - assert_eq!(list.len(), 10); for i in (0..10).rev() { + assert_eq!(list.len(), i as usize + 1); assert!(!list.is_empty()); assert_eq!(i, list.pop_front().unwrap()); } @@ -89,12 +93,15 @@ fn basics_push_front_pop_back() { let mut list: LinkedList = LinkedList::new(); for i in 0..10 { list.push_front(i); + assert_eq!(list.len(), i as usize + 1); assert!(!list.is_empty()); } for i in 0..10 { + assert_eq!(list.len(), 10 - i as usize); assert!(!list.is_empty()); assert_eq!(i, list.pop_back().unwrap()); } + assert_eq!(list.len(), 0); assert!(list.is_empty()); } @@ -104,12 +111,15 @@ fn basics_push_back_pop_front() { let mut list: LinkedList = LinkedList::new(); for i in 0..10 { list.push_back(i); + assert_eq!(list.len(), i as usize + 1); assert!(!list.is_empty()); } for i in 0..10 { + assert_eq!(list.len(), 10 - i as usize); assert!(!list.is_empty()); assert_eq!(i, list.pop_front().unwrap()); } + assert_eq!(list.len(), 0); assert!(list.is_empty()); }