Implement Luhn #247

IanWhitney · 2017-01-14T23:20:42Z

Closes #237

This mostly follows the current test suite
(exercism/problem-specifications#491), with one change.

The final canonical test adds an alphabetical character to a known
invalid string and then asserts that it's still invalid.

Instead what the test should do is alter a known-valid string and show
that it is now invalid.

That's what I've done here. I'll also propose the change to canonical
test suite.

Closes exercism#237 This mostly follows the current test suite (exercism/problem-specifications#491), with one change. The final canonical test adds an alphabetical character to a known invalid string and then asserts that it's still invalid. Instead what the test should do is alter a known-valid string and show that it is now invalid. That's what I've done here. I'll also propose the change to canonical test suite.

petertseng · 2017-01-14T23:33:23Z

exercises/luhn/example.rs

+    }
+
+    candidate.chars()
+        .filter(|c| c.is_numeric())


can I suggest filter_map here? It seems perfect for this use.

IanWhitney · 2017-01-14T23:43:42Z

Thinking about placement. I don't think my solution is that far from what people will do. The existing crate uses another crate to handle digits > 10 after doubling. If people go down that path their solutions will vary a bit from mine, but not by a bunch.

So the solution mostly requires a good handle on

Converting a string to digits
Working with iteration

I would put it between Sum of Multiples and Scrabble Score.

Update:

The conversion of a string to digits is also part of Largest Series Product, so we could move the exercise to right before LSP to reinforce that concept.

https://github.com/exercism/xrust/pull/247/files#r96126729

I don't think my solution is that far from what people will do. The [existing crate](https://lunemec.github.io/rust-luhn/src/luhn2/src/lib.rs.html) uses another crate to handle digits > 10 after doubling. If people go down that path their solutions will vary a bit from mine, but not by a bunch. So the solution mostly requires a good handle on - Converting a string to digits - Working with iteration The conversion of a string to digits is also part of Largest Series Product, so we could put the exercise to right before LSP to reinforce that concept.

While implementing these tests for Rust I found that the final test was not well written (my fault!) exercism/rust#247 I added the alphabetical character to an already-invalid string. So the test doesn't show anything useful. Instead the test should show that a known-valid string is made invalid by the inclusion of a non-digit

IanWhitney · 2017-01-15T02:30:46Z

This works, but I've been thinking about some other, more Rusty ways of approaching it.

Option 1: Use `From`

This way we could validate strings and different types of unsized ints. The test API would look like:

let luhn = Luhn::from("012345");
assert!(!luhn.is_valid());
let luhn = Luhn::from(12345);
assert(!luhn.is_valid());

Option 2: Use Traits

Students would create a LuhnValidator trait and then implement it for different types. Test API would look like:

let string_luhn = String::from("012345");
assert!(!string_luhn.valid_luhn());

let u8_luhn: u8 = 8; 
assert!(!u8_luhn.valid_luhn());
//and so on

IanWhitney · 2017-01-15T02:34:08Z

Option 3

Do all of them. Have 3 Luhn exercises that show different ways of solving the problem, so that students can see each approach.

petertseng · 2017-01-15T11:23:43Z

I know you already placed, but I'm going to make my placement comment without looking at where you placed to avoid bias, and if our ideas match up then great.

Place no later than robot-simulator.
Place no earlier than hamming.

Fantastic, we agree.

petertseng

we could validate strings and different types of unsized ints

A bit of a shame that strings with leading zeroes lose their leading zeroes, but fortunately this doesn't affect correcntess.

Students would create a LuhnValidator trait and then implement it for different types.

Yeah, interesting way to add functions to existing types. Do we have any exercise that already does this? I think it's a good thing to show off at some point.
Looking at our git grep "impl.*for", I guess not.

Hmm, https://doc.rust-lang.org/book/traits.html#rules-for-implementing-traits says "It is considered poor style to implement methods on such primitive types, even though it is possible.", I wonder if we should still show it anyway.

petertseng · 2017-01-15T11:38:10Z

exercises/luhn/example.rs

+        .rev()
+        .enumerate()
+        .map(|(index, digit)| if index % 2 == 0 { digit } else { digit * 2 })
+        .map(|digit| if digit > 10 { digit - 9 } else { digit })


sorry, but this code says !is_valid("59") and is_valid("50"), when instead it should say is_valid("59") and !is_valid("50").

Ah, good catch. Should be digit > 9.

petertseng · 2017-01-15T11:40:08Z

exercises/luhn/tests/luhn.rs

+
+#[test]
+#[ignore]
+fn valid_canadian_sin_is_valid() {


long cat is long.

(No change necessary)

IanWhitney · 2017-01-15T13:29:33Z

Just this week I was trying to figure out how to monkey-patch Ruby to add methods to the number 2. Not that this is a good idea, but it was interesting. For the same reason I think show traits on primitives could be interesting, though the documentation of the exercise should say that it is not recommended.

I'm going to experiment with Option 3 and will either update this PR or create a new one.

exercism#247 (comment)

* Luhn: Update test of non-digit While implementing these tests for Rust I found that the final test was not well written (my fault!) exercism/rust#247 I added the alphabetical character to an already-invalid string. So the test doesn't show anything useful. Instead the test should show that a known-valid string is made invalid by the inclusion of a non-digit

This is an implementation of the idea I discussed here exercism#247 (comment) After students do the original Luhn exercise, they can learn how to use the From trait to allow validation of multiple types. I've put this right after Luhn, which might be a bit early for introducing Traits.

This implements the Trait-based API that I proposed here exercism#247 (comment) As pointed out by Peter, using traits like this is probably an anti-pattern. I'm somewhat on the fence about demonstrating it.

This is an implementation of the idea I discussed here exercism#247 (comment) After students do the original Luhn exercise, they can learn how to use the From trait to allow validation of multiple types. I've put this right after Luhn, which might be a bit early for introducing Traits.

This implements the Trait-based API that I proposed here exercism#247 (comment) As pointed out by Peter, using traits like this is probably an anti-pattern. I'm somewhat on the fence about demonstrating it.

This implements the idea I discussed here exercism#247 (comment) After students do the original Luhn exercise, they can now do "Luhn: Using the From Trait" to allow validation of multiple types. And after that, they can do "Luhn: Using a Custom Trait" to develop their own trait.

This implements the idea I discussed here exercism#247 (comment) After students do the original Luhn exercise, they can now do "Luhn: Using the From Trait" to allow validation of multiple types. And after that, they can do "Luhn: Using a Custom Trait" to develop their own trait. "Luhn: Using the From Trait" is placed just after Bracket Push (which also uses `From`). In the Luhn implementation students will implement `From` for multiple types, so it makes sense to put it after Bracket Push where students only implement it for one type. "Luhn: Using a Custom Trait" moves to after Space Age. Space Age solutions use custom traits and `From`, so pairing these exercises made sense.

petertseng reviewed Jan 14, 2017

View reviewed changes

whit0694 added 3 commits January 14, 2017 17:47

Ignore all but first test

f280c5a

Filter + Map = filter_map

0bee7de

https://github.com/exercism/xrust/pull/247/files#r96126729

IanWhitney force-pushed the implement_luhn branch from 9581ad7 to 00715b5 Compare January 14, 2017 23:59

IanWhitney mentioned this pull request Jan 15, 2017

Luhn: Update test of non-digit exercism/problem-specifications#496

Merged

ijanos approved these changes Jan 15, 2017

View reviewed changes

petertseng reviewed Jan 15, 2017

View reviewed changes

Fix logical error

1c1e8b4

exercism#247 (comment)

petertseng approved these changes Jan 15, 2017

View reviewed changes

IanWhitney merged commit 2886848 into exercism:master Jan 15, 2017

IanWhitney deleted the implement_luhn branch January 15, 2017 21:06

IanWhitney mentioned this pull request Jan 16, 2017

Example of Luhn parts 2 & 3, From Trait and Custom Trait #248

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Luhn #247

Implement Luhn #247

IanWhitney commented Jan 14, 2017

petertseng Jan 14, 2017

IanWhitney Jan 14, 2017

IanWhitney commented Jan 14, 2017 •

edited

Loading

IanWhitney commented Jan 15, 2017 •

edited

Loading

IanWhitney commented Jan 15, 2017

petertseng commented Jan 15, 2017

petertseng left a comment •

edited

Loading

petertseng Jan 15, 2017

IanWhitney Jan 15, 2017

petertseng Jan 15, 2017

IanWhitney commented Jan 15, 2017

Implement Luhn #247

Implement Luhn #247

Conversation

IanWhitney commented Jan 14, 2017

petertseng Jan 14, 2017

Choose a reason for hiding this comment

IanWhitney Jan 14, 2017

Choose a reason for hiding this comment

IanWhitney commented Jan 14, 2017 • edited Loading

IanWhitney commented Jan 15, 2017 • edited Loading

Option 1: Use From

Option 2: Use Traits

IanWhitney commented Jan 15, 2017

Option 3

petertseng commented Jan 15, 2017

petertseng left a comment • edited Loading

Choose a reason for hiding this comment

petertseng Jan 15, 2017

Choose a reason for hiding this comment

IanWhitney Jan 15, 2017

Choose a reason for hiding this comment

petertseng Jan 15, 2017

Choose a reason for hiding this comment

IanWhitney commented Jan 15, 2017

IanWhitney commented Jan 14, 2017 •

edited

Loading

IanWhitney commented Jan 15, 2017 •

edited

Loading

Option 1: Use `From`

petertseng left a comment •

edited

Loading