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

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.


2 Answers 2

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

A different approach could be:

def luhn(card_no)
    .inject(0) do |sum, (a, b)|
      double = b.to_i * 2
      sum + a.to_i + (double > 9 ? double - 9 : double)
    end % 10 == 0

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

  • 1
    \$\begingroup\$ 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 :) \$\endgroup\$
    – tokland
    Commented Jul 16, 2016 at 12:03
  • 1
    \$\begingroup\$ @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. \$\endgroup\$
    – Flambino
    Commented Jul 16, 2016 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

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.

  • 1
    \$\begingroup\$ This discussion on this post has been moved to chat \$\endgroup\$
    – janos
    Commented Jul 11, 2016 at 12:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.