Luhn Algorithm- Returning True for a False Credit Card

Question

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

hmm..interesting.I am using the following for @card_numbers (4408041234567892), but it produces true. — user2201289, Sep 25 '13 at 21:54
Especially interesting, your code gives me false for that number as well. — Reinstate Monica -- notmaynard, Sep 25 '13 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.. — user2201289, Sep 25 '13 at 22:19
You have the sum being printed from within the code? Which line? How are you calling this method? — Reinstate Monica -- notmaynard, Sep 25 '13 at 22:24
The 'sum % 10==0' if I were to change it to 'print sum' I end up getting 69. This means that the argument and code does what it is supposed to do, but for some reason the specs are not coming back correct. 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' — user2201289, Sep 25 '13 at 23:03
I suspect I know the cause of your problem. In my answer below, look at the first statement in `def valid?(card)`. Try using it with your data. — Cary Swoveland, Sep 26 '13 at 04:07
For future reference, it's often helpful (as it is in this case) to post the code that calls the method, as well as the code of the method itself. Also, if you could post the output of your test (using rspec, I take it?), that would also be helpful. — Reinstate Monica -- notmaynard, Sep 26 '13 at 13:55
4408041234567893 => true, 440804l234567893 => false. Those strings are not the same! Look carefully! Above you have '2', rather than '3' as the last digit, but in a comment below you have '3'. I assume you mean '3' (and that you're working your way through Rubeque). — Cary Swoveland, Sep 28 '13 at 04:54
@CarySwoveland The card ending in "2" should be an invalid card, while the one ending in "3" should be valid. That's why those two different numbers are being used. — Reinstate Monica -- notmaynard, Sep 30 '13 at 16:55

score 1 · Answer 1 · answered Sep 25 '13 at 21:35

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

answered Sep 25 '13 at 21:35

Cary Swoveland

106,649
6
63
100

+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). – Reinstate Monica -- notmaynard Sep 26 '13 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). – Reinstate Monica -- notmaynard Sep 26 '13 at 17:26
@iamnotmaynard Your suggesion is well-taken. It prompted me to do a minimum of investigation. This [Wiki](http://wikipedia.org/wiki/Credit_card_number) 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.) – Cary Swoveland Sep 26 '13 at 17:34

score 0 · Accepted Answer · answered Sep 25 '13 at 20:25

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

answered Sep 25 '13 at 20:25

steenslag

79,051
16
138
171

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. – user2201289 Sep 25 '13 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 '13 at 22:20
@user2201289 Are you asking about "4408041234567893" or "440804l234567893"? That cc number is familiar. – Cary Swoveland Sep 26 '13 at 03:48

score 0 · Answer 3 · answered Sep 26 '13 at 14:07

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.

score 0 · Answer 4 · answered Jun 27 '16 at 04:16

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

Luhn Algorithm- Returning True for a False Credit Card

4 Answers4

Linked