ATM program with loan section

Question

I have spent a few days coding an ATM. What I'm looking for is some critique of my work. Is there anything I can do better here?

The entire project has 9 files and one class, a checking account and savings accounts. It also has a section for loans (thought it was different). Any advice, or input would be greatly appreciated.

To copy this from github the link is: https://github.com/13aal/ATM/tree/master

To make it run use run.rb

main.rb source:

#!/usr/bin/env ruby

################
#ATM Rewrite 
#                                
#Creator Lost Bam      
#                        
#11/19/15                
##################

require_relative 'checking.rb'
require_relative 'savings.rb'
require_relative 'transfer.rb'
require_relative 'loan.rb'
require_relative './ready/exit.rb'
require_relative './ready/redirect.rb'

class ATM
    attr_accessor :name, :checking_account, :savings_account, :pin_number, :transfer, :loan
        def initialize( name, checking_account, savings_account, pin_number, credit_score )
            @name = name
            @checking_account = checking_account
            @savings_account = savings_account
            @pin_number = pin_number
            @credit_score = credit_score
        end

        def pin 
            x = 3
            while (x > 0) do
                puts "Enter PIN(#{x} attempts left):"
                pin_num = gets.chomp
                case pin_num.to_i
                when @pin_number
                    menu
                else
                    puts "Invalid PIN"
                    x -= 1
                    bad_pin if x == 0
                end
            end
        end

        def menu 
            puts <<-END.gsub(/^\s*>/, ' ')
                >
                >Welcome #{name} thank you for choosing Bank of Bam.
                >You may choose from the list below of what you would like to do
                >For checking inquiries press '1'
                >For savings account information press '2'
                >To transfer funds press '3'
                >To either apply for a loan, or get information on a loan press '4'
                >To exit the system press '5'
                >
            END
            input = gets.chomp
            case input.to_i
            when 1
                checking_information
            when 2
                savings_information
            when 3
                transfer_funds
            when 4
                loan_info
            when 5
                exit_screen
            else
                puts "Error Invalid option please try again"
                menu
            end
        end

        def bad_pin
            abort('Exiting system, all attempts failed...')
            exit
        end
    end

checking.rb source:

private

    def checking_information
        puts <<-END.gsub(/^\s*>/, ' ')
            >
            >Your current balance is: #{@checking_account}
            >Deposit funds '1'
            >Make a withdrawl '2'
            >Transfer funds '3'
            >Go back '4'
            >
        END
        input = gets.chomp
        case input.to_i
        when 1
            deposit_checking
        when 2
            withdrawl_checking
        when 3
            transfer_funds
        when 4
            redirect
        else
            puts "Invalid option"
            checking_information
        end
    end

    def deposit_checking
        puts "Enter amount to deposit:"
        amount = gets.chomp.to_i
        @checking_account += amount.to_f
        puts "Your new balance is #{@checking_account}"
        puts "Would you like make another deposit?"
        input = gets.chomp
        if input == /yes/i
            deposit
        else
            redirect
        end
    end

    def withdrawl_checking
        puts "Your current balance is #{@checking_account}. Please enter amount to withdrawl:"
        input = gets.chomp.to_i
        @checking_account -= input.to_f
        puts "Your new balance is #{@checking_account}"
        puts "Would you like to make another withdrawl?"
        if input =~ /yes/i
            withdrawl_checking
        else
            redirect
        end
    end

savings.rb source:

private

def savings_information
    puts <<-END1.gsub(/^\s*>/,' ')
        >
        >Your current savings balance is #{@savings_account}
        >Deposit funds '1'
        >Make a Withdrawl '2'
        >Transfer funds '3'
        >Go back '4'
        >
    END1
    input = gets.chomp
    case input.to_i
    when 1
        deposit_savings
    when 2
        withdraw_savings
    when 3
        transfer_funds
    when 4
        redirect
    else
        puts "Invalid option"
        savings_information
    end
end

def deposit_savings
    puts "Your current balance is #{@savings_account}, how much would you like to deposit?"
    input = gets.chomp.to_i
    @savings_account += input
    puts "Your new balance is #{@savings_account}, would you like to make another deposit?"
    anthr_deposit = gets.chomp
    if anthr_deposit =~ /yes/i
        deposit_savings
    elsif anthr_deposit =~ /no/i
        redirect
    else
        puts "Invalid option, exiting."
        exit_screen
    end
end

def withdraw_savings
    puts "You current balance is #{@savings_account}, how much would you like to withdraw?"
    input = gets.chomp.to_i
    @savings_account -= input
    puts "Your new balance is #{@savings_account}, would you like to make another withdrawl?"
    anthr_withdrawl = gets.chomp
    if anthr_withdrawl =~ /yes/i
        withdraw_savings
    elsif anthr_withdraw =~ /no/i
        redirect
    else
        puts "Invalid option, exiting."
        exit_screen
    end
end

transfer.rb source:

private

require_relative 'error.rb'

def transfer_funds
    puts <<-START.gsub(/^\s*>/, ' ')
        >
        >Transfer from Savings to Checking '1'
        >Transfer from Checking to Savings '2'
        >
    START
    input = gets.chomp
    case input.to_i
    when 1
        puts <<-START1.gsub(/^\s*>/, ' ')
            >
            >Your current balance in your Savings account is: #{@savings_account}
            >Your current balance in your Checking account is: #{@checking_account}
            >How much would you like to transfer from your Savings to your Checking?
            >
        START1
        transfer = gets.chomp.to_i if @savings_account > 0
        if transfer > @savings_account
            error
        else
            @savings_account -= "#{transfer}".to_f
            @checking_account += "#{transfer}".to_f
            puts <<-START2.gsub(/^\s*>/, ' ')
                >
                >Your new balance for your Savings account is #{@savings_account}
                >Your new balance for your Checking account is: #{@checking_account}
                >Would you like to make another transfer?
                >
            START2
            anthr_transfer = gets.chomp
            if anthr_transfer =~ /yes/i
                transfer_funds
            elsif anthr_transfer =~ /no/i
                puts "System will now exit for security"
                exit_screen
            else
                puts "Invalid options"
                redirect
            end
        end
    when 2
        puts <<-START3.gsub(/^\s*>/, ' ')
            >
            >Your current balance in your Checking account is: #{@checking_account}
            >Your current balance in your Savings account is: #{@savings_account}
            >How much would you like to transfer from your Checking to your Savings?
            >
        START3
        transfer = gets.chomp.to_i
        if transfer > @checking_account
            error
        else
            @checking_account -= "#{transfer}".to_f
            @savings_account += "#{transfer}".to_f
            puts <<-START4.gsub(/^\s*>/, ' ')
                >
                >Your new balance for your Checking account is #{@checking_account}
                >Your new balance for your Savings account is: #{@savings_account}
                >Would you like to make another transfer?
                >
            START4
            anthr_transfer = gets.chomp
            if anthr_transfer =~ /yes/i
                transfer_funds
            elsif anthr_transfer =~ /no/i
                puts "System will now exit for security"
                exit_screen
            else
                puts "Invalid options"
                redirect
            end
        end
    end
end

loan.rb source:

private

def loan_info
    puts <<-END.gsub(/^\s*>/, ' ')
        >
        >Hello and welcome to the credit station
        >Please choose from the list below
        >Would you like to Apply for a loan '1'
        >Check credit score '2'
        >Go back '3'
        >
    END
    input = gets.chomp
    case input.to_i
    when 1
        apply_credit
    when 2
        check_score
    else
        redirect
    end
end

def apply_credit
    if @credit_score >= 640
        accepted
    else
        denied_loan
    end
end

def accepted
    loan = %w(100 200 300 400 500 600 700 800 900 1000 1100 1200 1300 1400 1500 1600 1700 1800 1900 2000 2100 2200 2300 2400 2500 2600 2700 2800 2900 3000 3100 3200 3300 3400 3500 3600 3700 3800 3900 4000 4100 4200 4300 4400 4500 4600 4700 4800 4900 5000 5100 5200 5300 5400 5500 5600 5700 5800 5900 6000 6100 6200 6300 6400 6500 6600 6700 6800 6900 7000 7100 7200 7300 7400 7500 7600 7700 7800 7900 8000 8100 8200 8300 8400 8500 8600 8700 8800 8900 9000 9100 9200 9300 9400 9500 9600 9700 9800 9900 10000)
    loan_amount = loan.sample
    puts "You have been accepted for a #{loan_amount} loan which will be added to your bank account"
    puts <<-END.gsub(/^\s*>/, ' ')
        >
        >Which account would you like to add that to?
        >Checking Account '1'
        >Savings Account '2'
        >
    END
    input = gets.chomp
    case input.to_i
    when 1
        @checking_account += loan_amount.to_f
        puts "#{loan_amount} has been added to your checking account, your new balance is #{@checking_account}"
        puts "Your card will now be returned for security purposes."
        exit_screen
    when 2
        @savings_account += loan_amount.to_f
        puts "#{loan_amount} has been added to your savings account, your new balance is #{@savings_account}"
        puts "Your card will now be returned for security purposes"
        exit_screen
    end
end

def check_score
    puts "In order to check your score $5.99 will be charged to your account. Do you accept the charges?"
    input = gets.chomp
    if input =~ /yes/i
        @checking_account -= 5.99
        puts "Your credit score is #{@credit_score}. Would you like to apply for a loan?"
        apply = gets.chomp
        if apply =~ /yes/i
            apply_credit
        else
            return
        end
    else
        puts "You will be taken back to the main menu."
        menu
    end
end

def denied_loan
    puts <<-REASON.gsub(/^\s*>/, ' ')
        >
        >We regret to inform you, but you have been denied
        >for the loan. This is do to either you owing money, or
        >credit score being to low. In order to be accepted you
        >must have a credit score of 640 or higher. Your credit
        >score came back to us as #{@credit_score}. We 
        >apologize for any inconvienence this may have caused 
        >you. If you feel this was denied in error, please contact
        >your bank.
        >
        >Your card will now be returned to you..
        >
    REASON
    exit_screen
end

exit.rb source:

def exit_screen
    puts "Returning card.."
    puts "Thank you for choosing Bank of Bam, have a nice day!"
    exit
end

redirect.rb source:

def redirect
    puts <<-END.gsub(/^\s*>/, ' ')
            >
            >Please choose from the list below
            >Main menu '1'
            >Checking inquiries  '2'
            >Savings account information '3'
            >Transfer funds '4'
            >Apply or view a loan '5'
            >Return card and exit system '6'
            >
        END
    input = gets.chomp
    case input.to_i
    when 1
        menu
    when 2
        checking_information
    when 3
        savings_information
    when 4
        transfer_funds
    when 5
        loan_info
    when 6
        exit_screen
    else
        puts "Invalid option"
        redirect
    end
end

error.rb source:

def error
    puts <<-EOT.gsub(/^\s*>/, ' ')
        >
        >Cannot complete transaction.
        >Either your account is overdrawn, you
        >do not have access to this section, or
        >you cannot transfer that amount. If you
        >feel you have reached this in error please
        >contact your bank.
        >
        >Error #28-971-9087 has been logged#<-Random error I made up
        >
        >Exiting system for security
        >
    EOT
    exit_screen
end

Run using run.rb source of run.rb:

require_relative 'main.rb' #<=Always require in this file

my_acc = ATM.new("Thomas Park", 5000, 10000, 7856, 640)
my_acc.pin                      #Checking account
                                      #savings account
                                             #PIN 
                                                   #credit score

Answer 1

User friendliness

    >Deposit funds '1'
    >Make a Withdrawl '2'
    >Transfer funds '3'
    >Go back '4'

Entering numbers does not feel user-friendly, allowing letters is much more intuitive and just as easy to implement:

    >[D]eposit funds 'D'
    >[M]ake a Withdrawl 'M'
    >[T]ransfer funds 'T'
    >[B]ack 'B'

and then:

case gets.chomp.downcase.first
when 'd'
  deposit
when 'm'
  ...

first is there to allow deposit or make to be valid inputs.

Avoid while overuse

Ruby is a high-level language, where while has a limited use. Any-time you use while think twice.

    x = 3
    while (x > 0) do
        ...
        x -= 1

Should be written as:

3.downto(1) do |x|
     ...

The second version is not only shorter, but also easier to read.

Avoid writing repetitive things by hand

You write:

loan = %w(100 200 300 400 500 600 700 800 900 1000 1100 1200 1300 1400 1500 1600 1700 1800 1900 2000 2100 2200 2300 2400 2500 2600 2700 2800 2900 3000 3100 3200 3300 3400 3500 3600 3700 3800 3900 4000 4100 4200 4300 4400 4500 4600 4700 4800 4900 5000 5100 5200 5300 5400 5500 5600 5700 5800 5900 6000 6100 6200 6300 6400 6500 6600 6700 6800 6900 7000 7100 7200 7300 7400 7500 7600 7700 7800 7900 8000 8100 8200 8300 8400 8500 8600 8700 8800 8900 9000 9100 9200 9300 9400 9500 9600 9700 9800 9900 10000)

But you are a programmer! Programming was invented to avoid repetitive tasks!

loan = (1..100).map{|x| (x*100).to_s}

This way you can be sure that no value is being left out and can easily modify the loan list.

Bug: decimal loss

    amount = gets.chomp.to_i
    @checking_account += amount.to_f

If you convert amount to integer you will lose all the decimals!

>> "4141.3".to_i
=> 4141

This is not what you want as you then call to_f

Magic numbers

A number is named magic when it appears randomly in the code with no specification as to why it has the value it has.

You should give them a name to allow easier code reading and future changes:

CHECK_COST = 5.99
MINIMAL_MONEY_FOR_LOAN = 640
PIN_ENTER_ATTEMPTS = 3

Names of constants are ALL_CAPITAL by convention.

prompt function for user inter interaction

You give a lot of output and ask for a lot of input, I suggest a function:

def prompt(text)
   puts(text)
   gets.chomp
end

You can then simplify your code, for example:

def deposit_checking
    @checking_account += prompt("Enter amount to deposit:").to_f
    puts "Your new balance is #{@checking_account}"

    if prompt("Would you like make another deposit?") == /yes/i
        deposit
    else
        redirect
    end
end

Other functions may be similarly cleaned up.

Replace comments with code

my_acc = ATM.new("Thomas Park", 5000, 10000, 7856, 640)
my_acc.pin                      #Checking account
                                      #savings account
                                             #PIN 
                                                   #credit score

May be written as:

my_acc = ATM.new( name = "Thomas Park",
                  checking_account = 5000,
                  savings_account = 10000,
                  pin_number = 7856,
                  credit_score = 640 )

Because the compiler / interpreter can automatically check code for correctness but not comments.

Answer 2

First thing I notice, is that user input/output is mixed in with the account manipulation logic. It's all tangled up. You've split things into files, but haven't split them by responsibility, which the key thing.

Second, and this is related to the first, I notice that your savings and checking accounts implementations are basically identical. Only the name is different.

At its core, an account has a balance, and said balance can be queried and increased (deposit) or decreased (withdrawal).

In a more object-oriented manner, this could be modelled as:

class Account
  attr_reader :name, :balance

  def initialize(name, balance)
    @name = name
    @balance = balance
  end

  def deposit(amount)
    @balance += amount
  end

  def withdraw(amount)
    @balance += amount
  end
end

Of course, there's a little more to it. You'll want to check that the amount values passed to the #deposit/#withdraw methods are indeed numbers - and that they're positive numbers so you can't withdraw a negative amount and end up with more money. And, depending on the account it might or might not allow for overdraw. Stuff like that.

Note that this class doesn't do any input/output. The point of the Account class is to act like a bank account; not to act like an ATM in and of itself. That's left up to other code.

Said other code could be an ATM class. It'd be its responsibility to handle user interaction and manipulate the account(s) in some way. Or, depending on how far you want to go, you could have a separate controller class that implement the account manipulation logic, but again leaves the interface up to yet another bit of code.

Anyway, we still have more data modelling: The second set of data (or model) we're working with is the customer. The customer could be modelled like so:

class Account
  attr_reader :name, :credit_score, :accounts

  def initialize(name, pin, credit_score)
    @name = name
    @pin = pin
    @credit_rating = credit_score
    @accounts = []
  end

  def verify_pin(pin)
    pin == @pin
  end
end

You could then do something like:

customer = Customer.new("Someone", 640, "1243")
customer.accounts << Account.new("Savings account", 10000)
customer.accounts << Account.new("Checking account", 5000)

Now, you have a customer against which you can check the PIN, and the customer has a list of accounts.

Again, this can be embellished is many ways. One thing to note, though, is that the PIN isn't readable by the outside - and certainly not writable. It's not really a concern for a test project like this, but conceptually the PIN should be kept secret. You can check if an entered PIN is correct, but you can't just ask for the real PIN once it's been set. That'd be too easy.

Now, for the interface/controller. You've got at least 2 common interactions:

• Picking something from a list/menu
• Entering an amount

In both cases, you'll want a way to re-prompt the user if the input doesn't make sense.

Whereever you place the interface logic, it'd be nice to make these parts into generic methods. Such methods could look like:

def menu_prompt(prompt, options)
  puts prompt
  options.each { |key, label| puts "[#{key}]: #{label}" }
  get_input("Invalid option. Try again") do |key|
    options.has_key?(key.downcase)
  end
end

def amount_prompt(prompt)
  puts prompt
  get_input("Invalid amount. Try again") do |amount|
    amount =~ /^\d+(\.\d\d?)?$/
  end
end

def get_input(error_message, &block)
  while true
    input = gets.chomp
    return input if yield input
  end
end

Both #menu_prompt and #amount_prompt use #get_input to actually get the user's input. However they pass along different blocks for checking said input (one requires that the input matches an item in the menu, and the other requires it to be a numeric string, possibly with 1-2 decimals). If the check fails, the user can try again.

You could use the same thing for the PIN entry:

def pin_prompt
  puts "Please enter your PIN"
  get_input("Invalid PIN") { |pin| some_customer.verify_pin(pin) }
end

Of course, this would just let someone try every combination, since the machine would just keep asking. But it's a start. You can either add the max-attempts logic to #get_input, or you write a wholly separate method specifically for prompting for a PIN.

Whether this specific way of doing things makes sense in all cases is up to you to find out. The point is that logic has been encapsulated and been made reusable.

Other stuff

As Caridorc pointed out you have a rounding error in your code because you switch between floats and integers in some places. A common way of avoiding this is to always treat amounts as integers, but in smallest unit of currency. So in the case of dollars, you'd use cents. It's not a perfect solution (e.g. interest rates that add fractional cents will pose problems), but for these purposes, it's safer. You do however have to divide or multiply by 100 to get the actual dollar amount whenever you're presenting it to the user or getting his/her input.

You also have another bug related to type/number conversions. Specifically this bit: pin_num.to_i. That won't work if the PIN starts with a zero. Technically, 0003 is a valid PIN, but once you convert it to an int, it becomes just 3 - not what the user entered, and not a proper 4-digit PIN. This is also why I used a string for the PIN in the Customer class example.

On another note, writing private doesn't do anything: All you methods are in the global scope (in effect, they're added as methods on the global object), so they're all at the same visibility level anyway.

Finally, the Ruby convention is 2 spaces of indentation. Not 4 spaces, not tabs. Always use 2 spaces.

Answer 3

This will be a very short answer you already have great answers that cover all the important stuff. Something not important about your code but about functionality is your menu choice. Something important to remember is user will have a muscle memory of your system.

        >Please choose from the list below
        >Main menu '1'
        >Checking inquiries  '2'
        >Savings account information '3'
        >Transfer funds '4'
        >Apply or view a loan '5'
        >Return card and exit system '6'

The problem with how you propose you option at the moment is that if you're adding an option, you first reflex will probably to increment the "exit" option to 7 and add the new feature as 6. Your users will probably have a hard time adjusting to that change if it's used.

An option is to leave space for future changes. You could have used 0 or 9 for the exit and that way it feels more natural if you add more features.

An other option is using letters like @Caridorc proposed in his answer. This will leave you with a lot more option without breaking previous choices. The thing to consider when using letters is to be consistent over all the menus and option. If you use E for exit, don't use E elsewhere to edit.

This is more important in bigger programs with a user-base, but I wanted to leave that critic as a thing you need to think sometimes.