# Luhn algorithm in Ruby

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 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") # => false


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 map operation.

1. 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.

2. Don't bother with adding digits when a number is greater than 9; just subtract 9. The result is the same.

3. Don't use split("") when there's the String#chars method.

4. Don't chain off of #each_with_index, when you can use the Enumerator#with_index "modifier" on #map, i.e. .map.with_index { ... }

5. 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]

6. 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
end


A 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
end


Oh and remember to also check the length of the card number, and that it is, in fact, all-numeric.

• Minor notes: 1) most of the comments add no useful information (that's the cool thing of FP code, it's declarative) 2) You can use method chaining all the way to the end: .modulo(10).zero?, 3) double of what? 4) If it's so important to validate the input, it should be done in your code :) – tokland Jul 16 '16 at 12:03
• @tokland 1) True - the comments were a hold-over from OP's code. I figured they didn't add much, but didn't really detract either, so I kept 'em. Helps point out the differences from the original, if anything. Didn't add comments for the 2nd code block, because, as you say, they're trivial. 2) Good point - totally forgot about #modulo! 3) Lazy naming, I admit. 4) This was on purpose; the review - and OP's code - is for the Luhn algorithm only, not full card no. validation. I mention it, sure, but I left the extra functionality for OP to figure out. – Flambino Jul 16 '16 at 16:26

As a small improvement on top the other answer, you could avoid declaring a double variable by using .divmod to split the digits, like this:

def luhn card_no
return false if card_no.empty?
card_no.chars.reverse.each_slice(2).flat_map do |a, b|
[a.to_i, *(b.to_i * 2).divmod(10)]
end.inject(:+) % 10 == 0
end


This way it's slightly more compact.
Also we now need .flat_map (not .map) since we return an array from the block, not a scalar.