Luhn algorithm in Ruby

Question

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.

Answer 1

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.

Answer 2

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.