# ATM program with loan section

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'

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
else
puts "Invalid PIN"
x -= 1
end
end
end

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"
end
end

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


checking.rb source:

private

def checking_information
puts <<-END.gsub(/^\s*>/, ' ')
>
>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
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*>/, ' ')
>
>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*>/, ' ')
>
>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*>/, ' ')
>
>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*>/, ' ')
>
>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 "Your card will now be returned for security purposes."
exit_screen
when 2
@savings_account += loan_amount.to_f
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  • Fixed, added error.rb and a image of usage Nov 24, 2015 at 17:34 • Using run.rb... Nov 24, 2015 at 17:54 • Neat that you placed a bounty! Still, there's a lot of code here, which is a little intimidating to review - even if there are additional fake internet points to be had. Do you have any specific worries, or something you'd like to have reviewers focus on? Everything's fair game for review, but if there's something specific you want feedback on, it might further encourage reviews Nov 29, 2015 at 0:33 • You have a bounty and still less than a hundred views :(. Maybe you should put each file in a different question and post the links to the other questions with each question. This will probably get you better reviews! :) Nov 29, 2015 at 19:45 • @DeliriousSyntax Added github repo Nov 30, 2015 at 12:48 ## 3 Answers 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. • Very nice answer, thank you. Only question I have is, why would using letters be more "user friendly" then numbers? Nov 29, 2015 at 13:46 • @LostBam because lettere are mnemonics. If a want to deposit it feels natural to write d Nov 29, 2015 at 13:48 • I really don't think I'm going to get a better answer for this, so I'm going to accept it congrats. Nov 30, 2015 at 20:35 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
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.

• Awesome answer thank you, I will be encrypting the PIN numbers, I just wanted to get it running first, I do like the idea of turning the accounts into classes, and the prompt method that was established. Thanks for the ideas! I do have a question for the options.each idea what would I implement as the key and label? Just the user menu? Nov 29, 2015 at 20:15
• @LostBam Yeah, I was thinking that you just pass it a hash, like { "d" => "Deposit", "w" => "Withdraw" } or something - that'd be your key and label for the menu items. As for encrypting PINs, that's fun challenge, but I'd say your code would benefit more from restructuring and a more object-oriented design. In the end, it's not a real ATM, so encrypting the PIN isn't really required. The take-away from the project should probably be more about overall design methodology than the encryption of 4 digits/characters. Nov 29, 2015 at 20:34
• Awh I see, I like it! I could probably work it into a YAML file, or just throw a hash in. I figured encryption would be a fun challenge, might as well learn as much as I possibly can from writing it. Nov 29, 2015 at 20:40
• @LostBam There's not much point encrypting PIN numbers in storage; you can brute-force 10k options trivially regardless of what scheme you implement. (Communication, though, should of course be encrypted.) Nov 30, 2015 at 13:50
• @Veedrac I know you can bruteforce just about anything to be honest. But encryption seems like a fun challenge, never actually encrypted anything so it's something I'd be excited to learn. Nov 30, 2015 at 16:42

        >Please choose from the list below
>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.

• Good to know, I understand what you're saying and will defiantly implement it into a rewrite, eventually. Nov 30, 2015 at 16:41