Ruby banking system program

Question

As a beginner I'd like to get any kind of feedback. The more the better. Any optimization and style mistakes?

class Account
      attr_reader :name, :balance

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

      private

      def pin
      @pin = 789 
      end  

      def pin_check
        puts "Welcome to the banking system, #{@name}!\n" + "To access your account, input PIN please"
        @input_pin = gets.chomp.to_i
      end      

     def pin_error 
        return "Access denied: incorrect PIN."
     end

    public

      def access
        if pin_check == pin
          puts "Input d: to deposit money, s: to show balance or w: to withdraw money"
          action = gets.chomp.downcase
            case action
            when "d"
            deposit
            when "s"
            display_balance
            when "w"
           withdraw
            else
        puts "Try again"
          end
        else puts pin_error
        end    
      end

      def amount
        puts "Input the amount"
        @money = gets.to_i
      end

      def over_error
        print "You don't own that kind of money, dude! Your balance: $#{@balance}"
      end

      def deposit
       @balance += amount
        puts "Deposited: $#{@money}. Updated balance: $#{@balance}."
      end

      def display_balance
        puts "Balance: $#{@balance}."   
      end

      def withdraw
        if  amount <= @balance
          @balance -= @money
           puts "Withdrew: $#{@money}. Updated balance: $#{@balance}."  
        else
            puts over_error
        end
      end
    end  

    checking_account = Account.new("James Bond", 520_000)
    checking_account.access

this is quite lovely. i really don't have much to add to it. i guess you only want the access method to be publicly available. the rest you would probably leave as private methods - because they might change. but this is pretty sweet IMO and i can't comment unless there are further changes to the requirements. — BenKoshy
– BenKoshy, Commented May 3, 2017 at 14:06

Flambino · Accepted Answer · 2017-05-03 23:38:09Z

Nice implementation. The comments I have are mostly about overall structure.

I do have one comment on some low-level stuff: Use a string for the PIN instead of an int. When it's an integer, you can't have leading zeros, so if James Bond's PIN is 007, you could enter 7, 07, or 0000000007, and all would be equally valid. That's no good (and James Bond - already the world's worst "secret" agent is a fool for having that PIN anyway).

I'd also suggest making the PIN a constructor argument - otherwise everyone in your bank will have the same hard-coded PIN. Not exactly secure.

As for structure: Your Account class isn't only an account - it's a complete ATM, too. User interface (the command line in this case), is mixed in with the business logic of managing the balance.

I'd suggest separating those concerns. One would be the Account model/class that holds the information, and performs actions on said information, and the other would encapsulate the user interface.

Right now, if you decide that you want to do web banking too - not just command line banking - you'd run into trouble. You'd have to make a brand new Account class to handle it. But that wouldn't make sense, since mr Bond only has (or should only have) one account - not different ones depending on how he's accessing them.

I'd do something like this:

class Account
  attr_reader :name, :balance

  def initialize(name, pin, balance)
    @name = name
    @pin = pin # TODO: Check that it's a valid PIN and not just blank or something
    @balance = balance
  end

  def valid_pin?(pin)
    @pin == pin
  end

  def withdraw(amount)
    # subtract amount from balance or raise error if balance
    # isn't large enough (or amount is negative)
  end

  def deposit(amount)
    # add amount to balance or raise error if amount is negative
  end
end

class Atm
  def initialize(account)
    @account = account
  end

  # ...
end

The Atm class handles all the user interaction much like now, but delegates the actual withdrawal/depositing and PIN checking to the account. How you structure it from here is up to you, but the basic point is that the account is "just" an account.

1. Nice one, though it doesn't work right after i've replaced int with string for pin input. I'll continue trying. — edvard_munch
– edvard_munch, Commented May 4, 2017 at 13:45
2. I'll definitely make some structural changes, got your point. — edvard_munch
– edvard_munch, Commented May 4, 2017 at 13:52
hello, can you give me the hint please, how to make these two classes work together without inheritance or using "Account" as a module? In your example you are passing the amount as an argument for two different methods. It is supposed to get the actual amount from the user input in the Atm class? I didn't ask right away because I thought I could figure this out myself, with the help of mighty google. Found some examples of simple ATM programs in Ruby and Python (since it very similar and more popular now). — edvard_munch
– edvard_munch, Commented May 16, 2017 at 6:47
Got some interesting new ideas, but nothing to help me figure out how to do this in the way that you mean. And can you tell me more about problems I'd run into if i'll use my current structure for web app? What kind of problems? Sorry but as a beginner I don't see it clearly :). — edvard_munch
– edvard_munch, Commented May 16, 2017 at 7:07
@Flynn84 Apologies, but it seems I won't have time to add much to my answer for the next couple days. But to quickly answer you question about using it for a web app: Your current code expect input from a terminal (asking for the the PIN, and you typing it in, etc.), but if you're running things on a server, people would be using it through a browser: There's no terminal. Hence why I said to separate user interface (terminal in this case, website in another) from core functionality (the account) — Flambino
– Flambino, Commented May 18, 2017 at 8:13

Flambino · Accepted Answer · 2017-05-04 14:01:21Z

5

There are a few things that I would do differently and/or change:

I usually find it easier to put all the public methods at the top and the private methods at the bottom instead of mixing them. This is also what I see most people do.
Be consistent. In pin_error you return an error message, in over_error you print the message (elsewhere you use puts). I would make both methods just puts the error message.
I would not assign an instance variable in amount just return the value and then change withdraw to use a local variable. I would also call it get_amount or something since it does more than just return an amount.
```
def withdraw
  amt = get_amount
  if  amt <= @balance
    @balance -= amt
    puts "Withdrew: $#{amt}. Updated balance: $#{@balance}."  
  else
    over_error
  end
end       
```
The pin method is odd in that it returns the bin but also sets an instance variable unnecessarily. I would pass the pin into the constructor.
I would make pin_check actually check if the pin is correct and return true or false. I would also give it an active name, with a bang, like check_pin!

edited May 4, 2017 at 14:01

Flambino

33.3k2 gold badges46 silver badges90 bronze badges

answered May 3, 2017 at 23:42

Marc Rohloff

3,54710 silver badges7 bronze badges

\$\begingroup\$ 1. I've read on stackowerflow probably week or two ago that it's better to have all private methods at the top, so I have them there. Except initialize, that always should be at the top? I hope I understand it properly that there is need to put "private" before every method you want to be private? They all private before I put the "public" word? I've checked - it worked in this program. \$\endgroup\$

edvard_munch
– edvard_munch

2017-05-04 11:50:50 +00:00
Commented May 4, 2017 at 11:50
\$\begingroup\$ 2. Yeah, thanks. That is obvious, I've never even paid attention to these methods since they worked properly from the very beginning. \$\endgroup\$

edvard_munch
– edvard_munch

2017-05-04 11:53:24 +00:00
Commented May 4, 2017 at 11:53
\$\begingroup\$ Nice answer +1. Took the liberty of editing just to format the code block (it was being rendered as regular text) \$\endgroup\$

Flambino
– Flambino

2017-05-04 14:02:35 +00:00
Commented May 4, 2017 at 14:02
\$\begingroup\$ wrt To private positioning, all the projects I have worked on have them at the bottom and that is my preference but it doesn't really matter as long as you are consistent within a project. \$\endgroup\$

Marc Rohloff
– Marc Rohloff

2017-05-04 16:09:22 +00:00
Commented May 4, 2017 at 16:09

Add a comment |

Toby Speight · Accepted Answer · 2019-03-11 09:47:04Z

There are few guidelines which I would follow while coding in Ruby:

Indentation in Ruby is 2 spaces.
In any program or project, improve understanding of the project by using proper documentation. Like Java has Javadoc for documentation, Ruby has Tomdoc, which helps in providing documentation to project.
Rubocop is a gem in Ruby, which can be installed on the local system, and from project location, when we run rubocop gives warning on style.
attr_reader method is used to read variables; attr_writer method is used to write variables. To both read and write, we use attr_accessor. In your case balance is read and written, so we can use attr_accessor.
Usually in Banking sector, the menu option repeats after transaction. Even in your case that can be achieved, by providing wcase statement in the while loop and the menu can have another option: exit :e.
Pin is unique for each user; we can prompt user to enter pin rather than hardcoding it.
We can use proper exception Handling. Instead of just printing the error message we can raise the error and rescue the error message from calling application.

Stack Exchange Network

Ruby banking system program

3 Answers 3

You must log in to answer this question.

Hot Network Questions

Ruby banking system program

3 Answers 3

You must log in to answer this question.

Related

Hot Network Questions