From afe320f0a6aaef284339b1b83c32f59b2ff6244a Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Tue, 12 Dec 2017 17:40:53 +0100 Subject: [PATCH] Update most_common* functions to return the intermediate Vec The most common operation in testing was to immediately collect it into a new vector, and there's no plausible way to sort a list without consuming all items from the list anyway, so we may as well just return the Vec and call it good. --- Cargo.toml | 2 +- README.md | 5 ++--- src/lib.rs | 36 ++++++++---------------------------- 3 files changed, 11 insertions(+), 32 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 73ee1bd..bf10508 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "counter" -version = "0.2.1" +version = "0.3.0" authors = ["Peter Goodspeed-Niklaus "] description = "Simple package to count generic iterables" repository = "https://github.com/coriolinus/counter-rs" diff --git a/README.md b/README.md index b592624..b878754 100644 --- a/README.md +++ b/README.md @@ -28,8 +28,7 @@ let difference = counts - other_counts; ```rust let by_common = Counter::init("eaddbbccc".chars()) - .most_common_ordered() - .collect::>(); + .most_common_ordered(); let expected = vec![('c', 3), ('b', 2), ('d', 2), ('a', 1), ('e', 1)]; assert!(by_common == expected); ``` @@ -40,7 +39,7 @@ For example, here we break ties reverse alphabetically. ```rust let counter = Counter::init("eaddbbccc".chars()); -let by_common = counter.most_common_tiebreaker(|&a, &b| b.cmp(&a)).collect::>(); +let by_common = counter.most_common_tiebreaker(|&a, &b| b.cmp(&a)); let expected = vec![('c', 3), ('d', 2), ('b', 2), ('e', 1), ('a', 1)]; assert!(by_common == expected); ``` diff --git a/src/lib.rs b/src/lib.rs index 69351ac..e7b9dfe 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -5,7 +5,6 @@ use std::collections::HashMap; use std::hash::Hash; - use std::ops::{Add, Sub, AddAssign, SubAssign, BitAnd, BitOr, Deref, DerefMut}; type CounterMap = HashMap; @@ -72,12 +71,7 @@ where T: Hash + Eq + Clone, { /// Create an iterator over `(frequency, elem)` pairs, sorted most to least common. - /// - /// FIXME: This is pretty inefficient: it copies everything into a vector, sorts - /// the vector, and returns an iterator over the vector. It would be much better - /// to create some kind of MostCommon struct which implements `Iterator` which - /// does all the necessary work on demand. PRs appreciated here! - pub fn most_common(&self) -> ::std::vec::IntoIter<(T, usize)> { + pub fn most_common(&self) -> Vec<(T, usize)> { use std::cmp::Ordering; self.most_common_tiebreaker(|ref _a, ref _b| Ordering::Equal) } @@ -87,12 +81,7 @@ where /// /// In the event that two keys have an equal frequency, use the supplied ordering function /// to further arrange the results. - /// - /// FIXME: This is pretty inefficient: it copies everything into a vector, sorts - /// the vector, and returns an iterator over the vector. It would be much better - /// to create some kind of MostCommon struct which implements `Iterator` which - /// does all the necessary work on demand. PRs appreciated here! - pub fn most_common_tiebreaker(&self, tiebreaker: F) -> ::std::vec::IntoIter<(T, usize)> + pub fn most_common_tiebreaker(&self, tiebreaker: F) -> Vec<(T, usize)> where F: Fn(&T, &T) -> ::std::cmp::Ordering, { @@ -108,7 +97,7 @@ where unequal @ _ => unequal, } }); - items.into_iter() + items } } @@ -120,12 +109,7 @@ where /// /// In the event that two keys have an equal frequency, use the natural ordering of the keys /// to further sort the results. - /// - /// FIXME: This is pretty inefficient: it copies everything into a vector, sorts - /// the vector, and returns an iterator over the vector. It would be much better - /// to create some kind of MostCommon struct which implements `Iterator` which - /// does all the necessary work on demand. PRs appreciated here! - pub fn most_common_ordered(&self) -> ::std::vec::IntoIter<(T, usize)> { + pub fn most_common_ordered(&self) -> Vec<(T, usize)> { self.most_common_tiebreaker(|ref a, ref b| a.cmp(&b)) } } @@ -422,7 +406,7 @@ mod tests { #[test] fn test_most_common() { let counter = Counter::init("abbccc".chars()); - let by_common = counter.most_common().collect::>(); + let by_common = counter.most_common(); let expected = vec![('c', 3), ('b', 2), ('a', 1)]; assert!(by_common == expected); } @@ -430,9 +414,7 @@ mod tests { #[test] fn test_most_common_tiebreaker() { let counter = Counter::init("eaddbbccc".chars()); - let by_common = counter - .most_common_tiebreaker(|&a, &b| a.cmp(&b)) - .collect::>(); + let by_common = counter.most_common_tiebreaker(|&a, &b| a.cmp(&b)); let expected = vec![('c', 3), ('b', 2), ('d', 2), ('a', 1), ('e', 1)]; assert!(by_common == expected); } @@ -440,9 +422,7 @@ mod tests { #[test] fn test_most_common_tiebreaker_reversed() { let counter = Counter::init("eaddbbccc".chars()); - let by_common = counter - .most_common_tiebreaker(|&a, &b| b.cmp(&a)) - .collect::>(); + let by_common = counter.most_common_tiebreaker(|&a, &b| b.cmp(&a)); let expected = vec![('c', 3), ('d', 2), ('b', 2), ('e', 1), ('a', 1)]; assert!(by_common == expected); } @@ -450,7 +430,7 @@ mod tests { #[test] fn test_most_common_ordered() { let counter = Counter::init("eaddbbccc".chars()); - let by_common = counter.most_common_ordered().collect::>(); + let by_common = counter.most_common_ordered(); let expected = vec![('c', 3), ('b', 2), ('d', 2), ('a', 1), ('e', 1)]; assert!(by_common == expected); }