0

Help me refactor implementing Luhn algorithm, which is described as follows:

The formula verifies a number against its included check digit, which is usually appended to a partial account number to generate the full account number. This account number must pass the following test:

  1. From the rightmost digit, which is the check digit, moving left, double the value of every second digit; if the product of this doubling operation is greater than 9 (e.g., 8 × 2 = 16), then sum the digits of the products (e.g., 16: 1 + 6 = 7, 18: 1 + 8 = 9).
  2. Take the sum of all the digits.
  3. If the total modulo 10 is equal to 0 (if the total ends in zero) then the number is valid according to the Luhn formula; else it is not valid.

Assume an example of an account number "7992739871" that will have a check digit added, making it of the form 7992739871x:

  • Account number 7 9 9 2 7 3 9 8 7 1 x
  • Double every other 7 18 9 4 7 6 9 16 7 2 -
  • Sum of digits 7 9 9 4 7 6 9 7 7 2 =67

The check digit (x) is obtained by computing the sum of digits then computing 9 times that value modulo 10 (in equation form, (67 × 9 mod 10)). In algorithm form:

  1. Compute the sum of the digits (67).
  2. Multiply by 9 (603).
  3. The last digit, 3, is the check digit. Thus, x=3.

Following is my implementation, it works but could be a lot better, I believe.

def credit_check(num)
verify = num.to_s.split('').map(&:to_i)

half1 = verify.reverse.select.each_with_index { |str, i| i.even? }
half1 = half1.inject(0) { |r,i| r + i }

# This implements rule 1
half2 = verify.reverse.select.each_with_index { |str, i| i.odd? }     
double = half2.map { |n| n * 2 }
double = double.map { |n| n.to_s.split('') }
double = double.flatten.map(&:to_i) 
double = double.inject(0) { |r,i| r + i }

final = double + half1   

puts final % 10 == 0 && (num.to_s.length > 12 && num.to_s.length < 17) ? "VALID" : "INVALID"
end

I'm a rank noob at all of this, obviously. But I appreciate any help, including proper style!

Improve this question
3
  • The first step is to use an editor that helps enforce indentation. That goes a long way toward writing correct code. Commented Feb 27, 2015 at 17:22
  • Thank you. I was using pico, which offers no help! Commented Feb 27, 2015 at 17:36
  • There are many good editors. The two mainstays are vim and emacs. They're a bit of a learning curve but they run on multiple platforms so once you learn one or the other you can use the same commands and configurations on other machines. I use vim on Mac OS, Windows and Linux, all with the same configurations. Also both Sublime Text Editor and Textmate are good; Sublime is in continuous development and Textmate seems to have become stale. Commented Feb 27, 2015 at 17:58

3 Answers 3

Reset to default
2

Suggestions:

  1. Try to encapsulate your code in a class and provide a intuitive public API. Hide the inner details of the algorithm in private methods.
  2. Break the rules into small methods in the class that has utmost 5 lines, break this rule sparingly. Follow Sandi Metz Rules.
  3. Study the problem and find domain names relevant to the problem; use it to name the small methods.
  4. Focus on readability. Remember this quote: "Programs must be written for people to read, and only incidentally for machines to execute." by Hal Abelson from SICP.
  5. Read Ruby style guide to improve code formatting; and yes get a better editor.
  6. Following these may seem like making the code more verbose. But it will improve readability and help for maintenance. Also, if you tend to follow it even in personal projects, this process will be etched into you and will soon become second nature.

With these in mind, go through the following attempt at the problem:

class CreditCard
  VALID_LENGTH_RANGE = 12..17

  def initialize(number)
    @number = number.to_s
  end

  def valid?
    valid_length? && check_sum_match?
  end

  private

  def valid_length?
    VALID_LENGTH_RANGE.include? @number.length
  end

  def check_sum_match?
    check_sum.end_with? check_digit
  end

  def check_sum
    digits = check_less_number
             .reverse
             .each_char
             .each_with_index
             .map do |character, index|
      digit = character.to_i
      index.even? ? double_and_sum(digit) : digit
    end

    digits.reduce(:+).to_s
  end

  def check_less_number
    @number[0..-2]
  end

  def check_digit
    @number[-1]
  end

  def double_and_sum(digit)
    double = digit * 2
    tens = double / 10
    units = double % 10

    tens + units
  end
end

Hence you can use it as follows:

CreditCard.new(222222222224).valid? # => true
CreditCard.new(222222222222).valid? # => false
Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

0

how about using nested inject method

 half2  = verify.reverse.select.each_with_index { |str, i| i.odd? }
 double = half2.map { |n| n * 2 }

 double = double.inject(0){|x,y| x + y.to_s.split("").inject(0){|sum, n| sum + n.to_i}}
Improve this answer

Comments

0

I would implement that algorithm like that:

def credit_card_valid?(num)
  digits = String(num).reverse.chars.map(&:to_i)
  digits.each_with_index.reduce(0) do |acc, (value, index)|
    acc + if index.even?
            value
          else
            double_value = value * 2
            if double_value > 9
              double_value.to_s.split('').map(&:to_i).reduce(&:+)
            else
              double_value
            end
          end
  end % 10 == 0
end

Well, that code works for those examples from Wikipedia :)

Here are some advices for you:

  • get rid of prints/puts to stdin in your function, just return a value. For this function boolean true/false is good.
  • ruby community uses '?' in method names that return false/true
  • don't forget about properly formatting your code, but maybe you might've not yet learnt how to do it on Stackoverflow (I haven't yet :)
  • use 2 spaces for indenting your code
Improve this answer

2 Comments

That is very straightforward and easily understood. And thanks for the tips. I don't understand the syntax of the penultimate line: end % 10 == 0 I understand what it does, but don't understand how it gets the value it's checking. Thanks!
Hey the whole construction before % 10 returns a number which is the result if that sophisticated multiplication/summation from wikipedia. So I could have assigned it to some variable like sum = digits.each_with_index...' and then use it like sum % 10 == 0`. The function returns true if a credit card number valid and false otherwise.

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.