11
\$\begingroup\$

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.

\$\endgroup\$
11
\$\begingroup\$
  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.

\$\endgroup\$
  • 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 Jul 16 '16 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 Jul 16 '16 at 16:26
5
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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