nucleotide-count: Error on invalid nucleotide#238
nucleotide-count: Error on invalid nucleotide#238ijanos merged 3 commits intoexercism:masterfrom petertseng:nucleotide-invalid
Conversation
| let mut map: HashMap<char, usize> = VALID_NUCLEOTIDES.chars().map(|c| (c, 0)).collect(); | ||
| for nucleotide in input.chars() { | ||
| *map.entry(nucleotide).or_insert(0) += 1; | ||
| match map.entry(nucleotide) { |
There was a problem hiding this comment.
you know, maybe instead of all this I could just check valid(nucleotide) (I would make it a top-level function), so that I wouldn't have to deal with Entry:: cases
There was a problem hiding this comment.
I feel that you are not supposed to deal with entry cases directly. I suggest using if let here:
if let Some(n) = map.get_mut(nucleotide) {
*n += 1;
} else {
return Err(nucleotide);
}There was a problem hiding this comment.
Fantastic. Did exactly that (though &nucleotide needed)
| #[test] | ||
| fn test_count_empty() { | ||
| assert_eq!(dna::count('A', ""), 0); | ||
| assert_eq!(dna::count('A', "").unwrap(), 0); |
There was a problem hiding this comment.
should we do anything specific here to make abundandly clear that it's a result? see https://github.com/exercism/xrust/blob/master/exercises/largest-series-product/tests/largest-series-product.rs#L6
There was a problem hiding this comment.
I think using is_err is enough.
There was a problem hiding this comment.
Agreed. For implementations that expect Result or Option, I usually start with some is_err and is_ok tests just to make it clear.
There was a problem hiding this comment.
Added. One for each function.
`count` and `nucleotide_counts` both return a Result now. Closes #149
| scrabble-score | chaining higher-order functions, HashMap (optional) | ||
| pangram | filter, ascii (optional) | ||
| nucleotide-count | filter, entry api, mutablity, match | ||
| nucleotide-count | Result, filter, entry api, mutablity, match |
| if valid(nucleotide) && input.chars().all(valid) { | ||
| Ok(input.chars().filter(|&c| c == nucleotide).count()) | ||
| } else { | ||
| Err(nucleotide) |
There was a problem hiding this comment.
for count('A', "AX") this returns A instead of X, but we don't check it in the tests so I'm not motivated.
|
Squashing is recommended. I'll do it soon, just giving y'all a last chance to look. |
IanWhitney
left a comment
IanWhitney
left a comment
There was a problem hiding this comment.
Other than adding the is_err test, which is not a blocker, I think this is fine.
3 participants
countandnucleotide_countsboth return a Result now.Closes #149
Too many other language implementations to look through them all and see what they did. There is no canonical-data.json for this problem yet and I do not have the time to make one.