patternrubyModerate
Luhn algorithm in Ruby
Viewed 0 times
algorithmluhnruby
Problem
I'm learning Ruby 2.3 and I've tried to implement a function which performs the Luhn credit card verification algorithm on an input string, returning
Is there any way to make this code shorter or more "Ruby-like"? I'm sure there must be a better way to perform the second
true if it matches and false if it doesn't.def luhn(card_no)
card_no.split("") # Break into individual digits
.reverse # Start from the end
.each_with_index
.map {|x, i| i.odd? and x.to_i * 2 or x.to_i} # Double every other digit
.map {|x| if x.to_s.length > 1; x.to_s.split("").map(&:to_i).inject(:+); else x end} # If any numbers are now 2-digit, add their digits
.inject(:+) % 10 == 0 # Check if multiple of 10
end
p luhn("4070590981311") # => true
p luhn("3103138183910") # => falseIs there any way to make this code shorter or more "Ruby-like"? I'm sure there must be a better way to perform the second
map operation.Solution
-
Don't flip back and forth between string manipulation and arithmetic. When is a number 2 digits? When it's greater than 9. Yes, you can convert to a string, and check the string length, but why bother? Just convert everything to a number immediately and stick to it.
-
Don't bother with adding digits when a number is greater than 9; just subtract 9. The result is the same.
-
Don't use
-
Don't chain off of
-
Don't do this
-
A slightly more descriptive method name would help too. Right now it's hard to tell if the method calculates the check digits, or checks the credit card number. Something like
That said there are many, many ways to implement the Luhn algorithm. A simple refactoring could be
A different approach could be:
Oh and remember to also check the length of the card number, and that it is, in fact, all-numeric.
Don't flip back and forth between string manipulation and arithmetic. When is a number 2 digits? When it's greater than 9. Yes, you can convert to a string, and check the string length, but why bother? Just convert everything to a number immediately and stick to it.
-
Don't bother with adding digits when a number is greater than 9; just subtract 9. The result is the same.
-
Don't use
split("") when there's the String#chars method.-
Don't chain off of
#each_with_index, when you can use the Enumerator#with_index "modifier" on #map, i.e. .map.with_index { ... }-
Don't do this
i.odd? and x.to_i * 2 or x.to_i when you have ternary branching: [condition] ? [true branch] : [else branch]-
A slightly more descriptive method name would help too. Right now it's hard to tell if the method calculates the check digits, or checks the credit card number. Something like
#valid_credit_card_number? is an option. That it's a Luhn algorithm is an implementation detail; external code just want to know if it's valid or not.That said there are many, many ways to implement the Luhn algorithm. A simple refactoring could be
def luhn(card_no)
card_no
.chars # Break into individual digits
.map(&:to_i) # map each character by calling #to_i on it
.reverse # Start from the end
.map.with_index { |x, i| i.odd? ? x * 2 : x } # Double every other digit
.map { |x| x > 9 ? x - 9 : x } # If > 9, subtract 9 (same as adding the digits)
.inject(0, :+) % 10 == 0 # Check if multiple of 10
endA different approach could be:
def luhn(card_no)
card_no
.chars
.reverse
.each_slice(2)
.inject(0) do |sum, (a, b)|
double = b.to_i * 2
sum + a.to_i + (double > 9 ? double - 9 : double)
end % 10 == 0
endOh and remember to also check the length of the card number, and that it is, in fact, all-numeric.
Code Snippets
def luhn(card_no)
card_no
.chars # Break into individual digits
.map(&:to_i) # map each character by calling #to_i on it
.reverse # Start from the end
.map.with_index { |x, i| i.odd? ? x * 2 : x } # Double every other digit
.map { |x| x > 9 ? x - 9 : x } # If > 9, subtract 9 (same as adding the digits)
.inject(0, :+) % 10 == 0 # Check if multiple of 10
enddef luhn(card_no)
card_no
.chars
.reverse
.each_slice(2)
.inject(0) do |sum, (a, b)|
double = b.to_i * 2
sum + a.to_i + (double > 9 ? double - 9 : double)
end % 10 == 0
endContext
StackExchange Code Review Q#134439, answer score: 12
Revisions (0)
No revisions yet.