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

• BenKoshy: 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.

Answer 1

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.

• edvard_munch: 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: 2. I'll definitely make some structural changes, got your point.
• edvard_munch: 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: 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 :).
• Flambino: @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)

Answer 2

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!

• edvard_munch: 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.
• edvard_munch: 2. Yeah, thanks. That is obvious, I've never even paid attention to these methods since they worked properly from the very beginning.
• Flambino: Nice answer +1. Took the liberty of editing just to format the code block (it was being rendered as regular text)
• Marc Rohloff: 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.

Answer 3

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.