6
\$\begingroup\$

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"--------------Menu-----------------"
        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"
        puts"Choose from the menu above:".red
        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"-----------------------------------"
            puts"What is your first name?:".yellow
                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"Please enter your address? :".yellow
                address = {}
                info[:address]= address
            puts"-----------------------------------"
            puts"what is the street? :".yellow
                street = gets.chomp
                address[:street] = street
            puts"-----------------------------------"
            puts"what is the city?:".yellow
                city = gets.chomp
                address[:city] = city
            puts"-----------------------------------"
            puts"what is the zipcode?:".yellow
                zip_code = gets.chomp.to_i
                address[:zip_code] = zip_code

                if (11410..12000).cover?(zip_code)
                    address[:county] = "New York"
                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"Your Account number is #{account_number}".blue
                    puts"your balance is #{balance}".red
                    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"
                print"your choice :"
                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")
          @@account_list = YAML.load_file("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
\$\endgroup\$
4
\$\begingroup\$
  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.

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

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)
                address[:county] = "New York"
            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.

\$\endgroup\$
  • \$\begingroup\$ 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 \$\endgroup\$ – junior stephane Jun 3 '15 at 20:30
  • \$\begingroup\$ @Devon Parsons the full name function can you explain more \$\endgroup\$ – junior stephane Jun 3 '15 at 20:32
  • \$\begingroup\$ @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". \$\endgroup\$ – Devon Parsons Jun 4 '15 at 12:25
  • 1
    \$\begingroup\$ @DevonParsons i did some research with the ruby documentation about the map method , and play with it and finally got it. \$\endgroup\$ – junior stephane Jun 4 '15 at 17:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.