# Bank ATM program in Ruby

After reading a bunch of Ruby tutorials, I decide to write fake ATM bank software. I've tried to use OOP design and just want to know if I'm on the right road.

 require "yaml"
require 'colorize'
class Person
attr_accessor :info

def initialize(info={})
@info = info
end

def change_info(info={})
self.info = info
end

def full_name
full_name =info[:first_name]
if !@middle_name.nil?
full_name += " "
full_name += info[:middle_name]
end
full_name += ' '
full_name += info[:last_name]
full_name
end

end

module Transaction
#contains all the operation need to manage a bank
def balance
# return balance  for each new object
puts"-----------------------------------"
puts"      Accounts balance - #{@account_number}"
puts"-----------------------------------"
puts "Your actual balance is $#{info[:balance]}" puts"-----------------------------------" end def deposit(amount) #add amount to the balance and return the aggregated amount depo_trans = @info[:balance] += amount @deposit.push(depo_trans) end def withdrawal(amount) depo_withdrawal = @info[:balance] -= amount @withdrawal.push(amount) end def transac_info count = 0 transac_counter = count += 1 puts"--------------------------------" puts" Transaction Lists-#{@info[:first_name]} " puts"---------------------------------" puts "Debit | Credit | No-Trans " puts"-----------Deposit---------------" all=@deposit.each{|trans| puts "$#{trans}"}
puts"Total deposit"
total=all.sum
puts"$#{total}" puts"-------------withdrawal----------" less=@withdrawal.each{|withd| puts "$#{withd}"}
diff =less.sum
puts "Total withdrawal"
puts"$#{diff}" puts"-----------Bank Balance----------" bank_bal = total + diff puts "$#{bank_bal}"
end

def transfer( account_number,pin, amount)
origin = @info[:balance]
lists=BankAccount.account_list
lists.each do |list|
if account_number == list[:account_number] && list[:pin] == pin
if amount > origin
puts "Low balance "
exit
else origin > amount
trans = list[:balance] += amount
@deposit.push(amount)
self.withdrawal(amount)
puts"Transfer complete"
end
end
end
end

end

module Run
include Transaction
loop do
puts"-----------------------------------"
puts"   Welcome to Bank of Trust        ".blue
puts"-----------------------------------"
puts"\n"
puts"1)Open an Account"
puts"2)View Balance "
puts"3)Make Transaction"
puts"4)View Transaction"
puts"5)Change Pin"
puts"6)Exit"
puts"-----------------------------------"
puts"\n"
ans = gets.chomp.to_i
if (1..6).include?(ans)
else
puts"Incorrect selection"
break
end

case ans
when 1
info = {}
puts"\n"
puts"-----------------------------------"
puts"Let's gets started".yellow
puts"-----------------------------------"
puts"   Account Opening Questions       ".blue
puts"-----------------------------------"
puts"-----------------------------------"
first_name = gets.chomp
info[:first_name] = first_name
puts"-----------------------------------"
puts"What is your last name? :".yellow
last_name = gets.chomp
info[:last_name] = last_name
puts"-----------------------------------"
puts"-----------------------------------"
puts"what is the street? :".yellow
street = gets.chomp
puts"-----------------------------------"
puts"what is the city?:".yellow
city = gets.chomp
puts"-----------------------------------"
puts"what is the zipcode?:".yellow
zip_code = gets.chomp.to_i

if (11410..12000).cover?(zip_code)
else
puts "Unfortunately we don't serve your community yet"
exit
end

if !info.nil?
@account = BankAccount.new(info)
puts"-----------------------------------"
puts"\n"
puts"Account has been successfully created".yellow
puts"\n"
@account.save()
else
puts "unable to create account at this time"
break
end
when 2
puts "in construction"
when 3
loop do
puts"\n"
puts "What do you want to do today?"
puts"-----------------------------------"
puts"1)Deposit  2)Withdrawal 3)Transfer 4)Balance 5)Exit"
ans = gets.chomp.to_i
case ans
when 1
puts"\n"

when 5
break
end

end
when 4
puts BankAccount.account_list
when 5
puts "in construction"
when 6
break
end

end

end

class BankAccount < Person
include Run

attr_accessor :balance, :pin
@@account_list = []
@@count = 0

def initialize(balance)
super
@info[:transaction]={}
@info[:transaction][:deposit]=[]
@info[:transaction][:withdrawal]=[]
@account_number = @@count += 1
@balance=0
@pin=0
@@account_list << @account
open()
end

def open
if File.exist?("bank_account.yml")
end
end

def save
File.open("bank_account.yml", "w") do |file|
file.write(@@account_list.to_yaml)
end
end

def set_pin(number)
self.info[:pin] = number
puts"-----------------------------------"
puts "Your pin was successfully changed "
puts"-----------------------------------"
end

def self.account_list
puts"---------------------------------"
puts"       Accounts Lists            "
puts"---------------------------------"
@@account_list.each do |account|
puts "#{account.full_name} | #{account[:account_number]} | \$#{account[:balance]}"
puts"---------------------------------"
end
end

end


1. Separate user interface from business logic

Each part of your code does a lot of printing. Like, really a lot of printing.

For one, it makes the code less maintainable; you have to mess with the basic classes just to fix a typo. And a lot of very different classes have to agree on how to format their print output, so it all looks nice. But the point should be that the classes are independent. But in this case, it also just makes the code really hard to read. I have to skip over a bunch of puts to see the actual logic.

Go for the Single Responsibility Principle. E.g. a bank account shouldn't worry about how to print itself; it should just worry about its balance and transactions.

2. Structure

Your class and module structure is... strange. A bank account is a type of person that runs? That just sounds weird; a bank account isn't a person. Sure, its owner may be a person (or may not in the case of company accounts), but that's not inheritance; that's relationships and object composition. And a bank account doesn't "run"; it's just a data model being acted upon by others.

It just seems like you've added some classes and modules just to add classes and modules. The Run module is especially scary as it's just a big loop. That doesn't really match the point of modules as "reusable code". It's so specific to what you're doing - and you're only using it once - that it makes little sense as a module.

Lastly, considering this is an "ATM simulator", I'm surprised there isn't an ATM class anywhere. Not saying there has to be, but it'd be an obvious place to start. Yet it feels like you've taken so many steps back from the very concrete and thing you're modelling that it's become much, much too abstract.

3. Using hashes for everything

Hashes are neat when you have highly dynamic content. But you don't use it for that. Hashes are informal, since they can contain anything or nothing. But you're dealing with pretty strict and formal data models (like bank accounts) that must have very specific properties and attributes.

For instance, it'd make more sense for Person to have actual first_name, middle_name, and last_name attributes instead of keeping all that in a hash.

Point is, your other code should be able to just say person.first_name, instead of having to know that a) a person has a thing named info (which is a very vague name - it could be anything) that b) is a hash, and c) has a :first_name key. If anything, the info hash belongs to a person instance - other code should not "reach into it" to get answers (cf Law of Demeter).

Besides, the way it works right now, I could create a person with anything. For instance: Person.new(pi: 3.14, north: "up", random: rand). That'll create a person - except none of it makes any sense.

In short, there's a lot of work to do here. Focus on the logic first - printing can come later. And separately.

Edit: Well, this answer got accepted more quickly than I expected (and perhaps too quickly in general; would've liked to see more answers). I added a bit here and there after the fact.

• I also like seeing multiple answers per question; I answered below if you want to take a look. – Devon Parsons Jun 3 '15 at 19:10

Flambino gave you some design points to ponder, so I'll critique your style.

...
if (1..6).include?(ans)
else
...


You should never have an empty if or else block. Replace this with unless(1..6).include?(ans)

if !@middle_name.nil?
...
if !info.nil?


This is needlessly complicated, just use

if @middle_name
...
if info


def full_name
full_name =info[:first_name]
if !@middle_name.nil?
full_name += " "
full_name += info[:middle_name]
end
full_name += ' '
full_name += info[:last_name]
full_name
end


Your indentation is messed up here, this should be

def full_name
full_name =info[:first_name]
if !@middle_name.nil?
full_name += " "
full_name += info[:middle_name]
end
full_name += ' '
full_name += info[:last_name]
full_name
end


Ruby style dictates we use 2 spaces per level of indentation. The style guide is here, if you're interested.

Moreover this is needlessly wordy. You can replace the entire thing with this:

def full_name
[:first_name, :middle_name, :last_name].map do |part|
info[part].to_s
end.join(" ")
end


If you don't understand what this is doing just comment and I'd be happy to explain. We're taking advantage of the fact that Ruby handles collections of things incredibly gracefully.

count = 0
transac_counter = count += 1


You don't even use these two variables, delete them.

puts"--------------Menu-----------------"
puts"1)Open an Account"
puts"2)View Balance "
puts"3)Make Transaction"
puts"4)View Transaction"
puts"5)Change Pin"
...


First of all, put a space after puts, but more importantly, either use a heredoc for large multiline text or read the text from a file menu.txt.

total=all.sum


Again, use spaces. This should be

total = all.sum


            if (11410..12000).cover?(zip_code)
else
puts "Unfortunately we don't serve your community yet"
exit
end

...

if amount > origin
puts "Low balance "
exit


Your code does a whole lot of exiting. Why not replace that exit with a call to the main menu?

You can't do that at the moment because your Run module doesn't have any methods in it. Aside from the include Transaction, throw all of that into a start or run method or something so you can reuse it.

@@account_list = []
@@count = 0


A final thing to consider when you get more familiar with Ruby - do not use Class variables, use Class instance variables. Class variables have wonky behaviour with inheritence.

• that's the sort of critiques that i wanted . Thanks a lot i 'll work on the pointer and see if i can better my code – junior stephane Jun 3 '15 at 20:30
• @Devon Parsons the full name function can you explain more – junior stephane Jun 3 '15 at 20:32
• @juniorstephane The map function changes one array into another. We want to map the different parts of a generic name (the first, middle, and last parts) to the actual name the person has. Everything between [:first_name... to end will evaluate to something like ["John","G","Smith"]. We do a look up once per part to get the "correct" value. Then we take that array and join it with spaces to get "John G Smith". – Devon Parsons Jun 4 '15 at 12:25
• @DevonParsons i did some research with the ruby documentation about the map method , and play with it and finally got it. – junior stephane Jun 4 '15 at 17:37