Update luhn.rs #1486
Update luhn.rs #1486
Conversation
Made it easier to read and also got rid of unwrap
| let cc_number = cc_number.replace(" ", ""); | ||
| let mut sum = 0; | ||
| let mut double = false; | ||
| let mut digit_seen = 0; | ||
| let mut even = false; |
There was a problem hiding this comment.
I think what you're essentially doing here is trading an allocation (cc_number.replace(" ", "") will typically have to allocate) for the digit_seen variable which tracks the number of characters seen.
I would prefer to avoid the allocation and thus remove the call to replace — allocation is not necessary when we can just filter out the spaces.
Your PR is useful since it can refactor the nested matching and if-statements into something that uses an early return.
There was a problem hiding this comment.
Can you take out this part since the allocation is ultimately unnecessary?
| num = num * 2; | ||
|
|
||
| if num > 9 { | ||
| num = (num / 10) + (num % 10); |
There was a problem hiding this comment.
We have this code in the past 🙂 See the history for this exercise.
People said in the past that the if sum > 9 { sum = sum - 9 } logic is easier to understand.
| double = !double; | ||
| digit_seen += 1; | ||
| } else { | ||
| return false; |
There was a problem hiding this comment.
You should be able to turn this return into an early return.
| match c.to_digit(10) { | ||
| Some(n) => num = n, | ||
| None => return false, | ||
| } |
There was a problem hiding this comment.
Please use the if let Some(num) = c.to_digit(10) else return false pattern here.
There was a problem hiding this comment.
Hi @AadeeWasTaken, could you merge in the latest main and see if there are still things to improve here?
Please also take out the parts that I commented on. Thanks!
| let cc_number = cc_number.replace(" ", ""); | ||
| let mut sum = 0; | ||
| let mut double = false; | ||
| let mut digit_seen = 0; | ||
| let mut even = false; |
There was a problem hiding this comment.
Can you take out this part since the allocation is ultimately unnecessary?
Made it easier to read and also got rid of unwrap