0

Having a little issue with my code to see if a credit card number adheres to the Luhn Algorithm. The code is returning true when the Credit Card is divisible by 10, but also is returning true when the CC number is not divisible by 10. I have printed out the final sum to make sure the numbers were actually adding to the sum variable, and they seem to be.. Below is my code. I know it can be cleaner, but at this stage I would like to see it work first.

def check_card

   c_num= []

   sum=0

   s_numbers=@card_numbers.to_s.reverse.split("")

   s_numbers.each_slice(2) do |x| 
   c_num << (x.last.to_s.to_i*2)
   c_num << (x.first.to_s.to_i)
     end

  c_num.each do |num|
    if num.to_i > 9
      sum+= (num.to_i % 10)+1
    else 
      sum += num.to_i
    end
  end

sum % 10==0 

end

Here is how it is being called:

it 'returns false for a bad card' do
  card = CreditCard.new(4408041234567892)
  card.check_card.should eq false
end
11
  • 1
    You code works for me. What is the input you're using? Sep 25, 2013 at 18:26
  • hmm..interesting.I am using the following for @card_numbers (4408041234567892), but it produces true. Sep 25, 2013 at 21:54
  • Especially interesting, your code gives me false for that number as well. Sep 25, 2013 at 21:57
  • Are you sure this is exactly the code you're using? Sep 25, 2013 at 21:57
  • Positive. I have an initialize method above it(for the class), but all it is doing is defining the local variable and sending an ArguementError if the credit card number is not long enough. The sum for the number I sent you keeps coming out to 69, so it should be false.. Sep 25, 2013 at 22:19

4 Answers 4

1

Now that another answer has appeared I will offer a suggested coding. This does not answer your question, but I thought it might be of interest and couldn't very well put it in a comment, because of formatting limitations.

def valid?(card)
  return false unless card  =~ /^\d+$/ # Ensure card contains only digits
  arr = card.split('').reverse.each_with_index.map {|d, index| (index.odd? ? 2*(d.to_i) : d.to_i)}
  (arr.join.split('').inject(0) {|tot, d| tot + d.to_i}) % 10 == 0
end 

valid?("1234567890123456") => false
3
  • +1 Good Ruby-ish answer. I might suggest that, since you're validating the credit card number, check that it has sixteen digits (or fifteen if it begins with 3, for AmEx). Sep 26, 2013 at 14:24
  • Yeah, I suppose I was just assuming the four major ones use Luhn (and that there'd be no need to check for any that use different lengths). Sep 26, 2013 at 17:26
  • @iamnotmaynard Your suggesion is well-taken. It prompted me to do a minimum of investigation. This Wiki told me more than I ever wanted to know about cc numbers. It turns out they range from 12 to 19 digits and most, but not all, use Luhn. (This comment replaces an earlier one with that had a bad link.) Sep 26, 2013 at 17:34
0

This is your code, commented and adapted to behave like the wikipedia description.

def check_card(str) #creditcardnumber as argument

   c_num = []

   sum = 0

   s_numbers = str.split("") #no reversing. str.split("").map(&:to_i) would save a lot of to_i's later on...
   checksum = s_numbers.pop.to_i #chop off last digit, store as checksum

   s_numbers.each_slice(2) do |x| 
     c_num << (x.last.to_s.to_i*2)
     c_num << (x.first.to_s.to_i)
   end

  c_num.each do |num|
    if num.to_i > 9
      sum+= (num.to_i % 10)+1
    else 
      sum += num.to_i
    end
  end

  (sum * 9) % 10 == checksum 

end

p check_card("79927398713") #=> true
3
  • Thanks for your help, however the code now says that the argument (4408041234567893) is false-should be true. Also, I see .map(&:to_i) a lot for this problem. Whenever I have seen & it is calling in a Proc or Lambda. What exactly are we doing with the code here. Sep 25, 2013 at 22:03
  • Try it with check_card("4408041234567893") - it expects a string. Don't worry about Lambda's and Procs, map.to_i would apply the to_i method to each element of the array which results from split(""): just nice integers to operate on..
    – steenslag
    Sep 25, 2013 at 22:20
  • @user2201289 Are you asking about "4408041234567893" or "440804l234567893"? That cc number is familiar. Sep 26, 2013 at 3:48
0

It seems you're confusing the output of the test with the output of the check_card method. You've confirmed that sum == 69, so sum % 10 == 0 should return false (unless Ruby's math is broken), so your check_card method should also return false — add the line puts card.check_card after the line card = CreditCard.new(4408041234567892) (but before the next line) in your testing block to show the value returned.

The next line, card.check_card.should eq false, asserts that the returned value "should equal" false — that is, it expects check_card to be false for that value of @card_numbers, and will return true if that's the case. I suspect that you're seeing the test come up true and taking that to mean the method is returning the incorrect value of true.

Without seeing the output of the test, this is only speculation, of course. However, your code seems correct and, for me, gives the correct result.

0

posting my answer because I thought it was clean:

def valid?(card_number)
  return false unless card_number !~ /\D/

  card_numbers = card_number.reverse.split("").map(&:to_i)
  sum = 0
  card_numbers.each_with_index do |num, i|
    if i.even?
      sum += num
    else
      sum += (num *= 2) > 9 ? num.divmod(10).inject(:+) : num
    end
  end
  sum % 10 == 0
end

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.