2 Replies - 626 Views - Last Post: 23 January 2014 - 11:04 PM Rate Topic: -----

#1 heaphyg  Icon User is offline

  • D.I.C Head

Reputation: 4
  • View blog
  • Posts: 85
  • Joined: 30-August 13

Refactoring question

Posted 23 January 2014 - 07:49 PM

This program checks to see if a credit card number is valid or not. Should I look into splitting my check_card method into multiple methods or is it fine in it's current state. I'm trying to consider the "single responsibility" principal but I guess I'm a little unconfident about what that means exactly. The method does achieve a single task (checking the validation of a number) however, it takes several steps to achieve this end. Should I divide each step into it's own method and figure out how to link those methods together so that a single method call on the CreditcCard object runs the whole program? Thanks for any guidance.

class CreditCard 
  def initialize(card_number)   
    card_number = card_number.to_s
    raise ArgumentError.new("Please input a 16 digit number.") if card_number.length != 16  # clean up the input data
 
    @number_array = card_number.split("").map { |x| x.to_i }                  # split our card number into an array of 16 integers (the meat)
  end

  def check_card                                                              # starting from the penultimate digit in @number_array double every other digit until you reach the first digit
    new_array = []
    n = -2
    8.times do 
      new_array << @number_array[n]                                           # create a new_array that has every other value from penultimate to beginning
      n -=2 
    end
    doubled_array = new_array.collect { |n| n*2 }                             # creates a new array with the values from new_array doubled
    split_array = doubled_array.join("").split("").map { |x| x.to_i }         # this will split all of our multi digit numbers into 2 elements


    # create an array with the other values from our origional array of 16 numbers 
    other_digits_array = []
    n = -1
    8.times do
      other_digits_array << @number_array[n]
      n -=2
    end


    master_array = split_array + other_digits_array 
    num = master_array.inject { |acc, n| acc + n } # sum up all the elements and assign the value to num
    if num % 10 == 0 # i should probably create a method here that calls the truth
      return true
    else 
      return false
    end
  end
end

card_number = CreditCard.new(4563960122001999)
puts card_number.check_card



Is This A Good Question/Topic? 0
  • +

Replies To: Refactoring question

#2 Lemur  Icon User is offline

  • Pragmatism over Dogma
  • member icon


Reputation: 1381
  • View blog
  • Posts: 3,511
  • Joined: 28-November 09

Re: Refactoring question

Posted 23 January 2014 - 10:23 PM

A tinge verbose on that one. Let's clean it up, shall we?

Now, to explain a refactoring here:
class CreditCard 
  def initialize(card_number) 
    @number_array = card_number.to_s.tap { |s| # Change the card number into a string, tap the results
      # Since tap ALWAYS returns self, we're just taking its value from after the to_s call,
      raise 'Please input a 16 digit number' unless s.length == 16 # doing some stuff with it,
    }.chars.map(&:to_i) # ...and letting it go on its' merry way.
                        # chars returns an array of characters, 
                        # which we map with the method to_i.
  end

  def check_card                                                              
    @number_array # Take the number array
      .map                   # Map over each digit
      .with_index { |x, i|   # ..with its' index
        i.even? ? x * 2 : x  # If the index is even, return the value doubled, else leave it be 
      }.reduce(:+) % 10 == 0 # Reduce the array using the + function, and mod 10 it.

    # Since Ruby implies returns, it'll return whether or not its' mod
    # 10 is 0. No need to be explicit on something like that.
  end
end

puts CreditCard.new(4563960122001999).check_card # => false
puts CreditCard.new(4563960122001992).check_card # => true



Without comments, that's 13 lines of (admittedly slightly dense) code from the original 40. Ruby lets you be incredibly concise with things if you treat it like Ruby instead of like a C language.

Challenge

Now here's a followup question: How many valid credit card numbers are there from 0 to 9999_9999_9999_9999?

Oh, and a conditional to that, you have to pass credit card digits that are less than 16 digits like 0, and make them run. Any more digits and it fails.

0 is valid
0000_0000_0000_0000 is valid (ruby ignores underscores and assumes you're making the number more readable.)
0000_0000_0000_00001 is not valid

Bonus

You'll notice real quickly that takes a heck of a long time. Multi thread the process by breaking up the process into smaller pieces.

This post has been edited by Lemur: 23 January 2014 - 10:39 PM

Was This Post Helpful? 1
  • +
  • -

#3 heaphyg  Icon User is offline

  • D.I.C Head

Reputation: 4
  • View blog
  • Posts: 85
  • Joined: 30-August 13

Re: Refactoring question

Posted 23 January 2014 - 11:04 PM

Wow...that's some cool stuff and thanks for showing me some new methods. I really appreciate it.
Was This Post Helpful? 0
  • +
  • -

Page 1 of 1