2
\$\begingroup\$

I've made a simple implementation of a bank ATM machine in pure Ruby. I'd like to hear some recommendations to make my code more idiomatic.

atm.rb

This is the startup file. I didn't use a class and I'm getting the user input using gets.chomp and a case statement for the flow control.

require_relative 'account'

account = Account.new

loop do
  puts "What you want to do?\n1- Deposit\n2- Withdraw\n3- Check balance\n4- Check statement\n5- Exit"
  option = gets.chomp
  case option
  when '1'
    puts "How many dollars do you want to deposit?"
    amount = gets.chomp.to_f
    account.deposit(amount)
  when '2'
    puts "How many dollars do you want to withdraw?"
    amount = gets.chomp.to_f
    account.withdraw(amount)
  when '3'
    puts "You have #{account.balance} USD."
  when '4'
    puts account.statement.join("\n")
  when '5'
    break
  else
    "Wrong option. Try again."
  end
end

account.rb

I used an array to store the transactions. To check the balance, I'm iterating through the array using the inject function.

require_relative 'transaction'

class Account
  def initialize
    @transactions = []
  end

  def balance
    @transactions.inject(0) { |sum, transaction| sum + transaction.value }
  end

  def deposit(amount)
    add_transaction(amount, :deposit)
  end

  def withdraw(amount)
    add_transaction(amount, :withdraw)
  end

  def statement
    @transactions.map { |transaction| transaction.to_s }
  end

  private
  attr_accessor :transactions

  def add_transaction(amount, type)
    @transactions << Transaction.new(amount, type)
  end
end

transaction.rb

This is the transaction class. I don't know if it's ok to validate the constructor parameters the way I did.

class Transaction
  attr_accessor :amount, :type, :timestamp

  def initialize(amount, type)
    raise 'Invalid type' unless POSSIBLE_TYPES.include? type
    @amount = amount
    @type = type
    @timestamp = Time.now.getutc
  end

  def value
    return @amount if type == :deposit
    -@amount
  end

  def to_s
    "#{@timestamp} - #{type} - #{amount} USD"
  end

  private
  POSSIBLE_TYPES = [:deposit, :withdraw]
end

I'm aware that I'm not validating the user input type or value, I'm ok with that.

See it running in repl.it.

\$\endgroup\$
  • 1
    \$\begingroup\$ By the way: it is really great that you recognized the Transaction abstraction. In almost all OO tutorials, bank accounts are used, but almost always it is taught that a transaction should be an operation and the balance should be data. When you do this, you run into a lot of trouble with synchronization, once you have concurrent access to your bank accounts. By modeling a transaction as data and the balance as an operation (a fold of the transaction log), you side-step a lot of those problems later on. Kudos for that! \$\endgroup\$ – Jörg W Mittag Dec 25 '17 at 18:34
5
\$\begingroup\$

This is my first review, so please bear with me. I'll start with atm.rb:

require_relative 'account'

account = Account.new

loop do
  # This is totally opinion, but I think doing this by joining a string array looks neater.
  # puts "What you want to do?\n1- Deposit\n2- Withdraw\n3- Check balance\n4- Check statement\n5- Exit"
  puts "What you want to do?"
  options = [
    "1- Deposit",
    "2- Withdraw",
    "3- Check balance",
    "4- Check statement",
    "5- Exit"
  ]
  puts options.join("\n")
  # These two lines can be condensed into one
  # option = gets.chomp
  # case option
  case gets.chomp
  when '1'
    puts "How many dollars do you want to deposit?"
    # You could merge these lines into account.deposit(get.chomp.to_f)
    amount = gets.chomp.to_f
    account.deposit(amount)
  when '2'
    puts "How many dollars do you want to withdraw?"
    # You could merge these lines into account.withdraw(get.chomp.to_f)
    amount = gets.chomp.to_f
    account.withdraw(amount)
  when '3'
    puts "You have #{account.balance} USD."
  when '4'
    puts account.statement.join("\n")
  when '5'
    break
  else
    "Wrong option. Try again."
  end
end

This is generally fine, just a couple things that I'd prefer for readability. I have larger ideas for structure for the other two files.

For the Transaction class, you really don't need the @type instance variable if it can only be :deposit or :withdraw. You can really just make that information be held in the sign of @amount. If @amount > 0, it's a deposit, and if @amount < 0 it's a withdraw. So, with that in mind, here's my revised version:

class Transaction
  # This was an attr_accessor. These attributes don't need to be editable, only readable.
  attr_reader :amount, :timestamp

  def initialize(amount)
    @amount = amount.to_f
    @timestamp = Time.now.getutc
  end

  def to_s
    # @amount is an instance variable, and so I added the "@" before it
    # You don't really need type here, but if you want it, use the next line:
    type = @amount > 0 ? "deposit" : "withdraw"
    "#{@timestamp} - #{type} - #{@amount} USD"
  end
end

And now, we move to the final file, account.rb:

require_relative 'transaction'

class Account
  def initialize
    @transactions = []
  end

  def balance
    # With our modifications to Transaction, we can do this more simply
    # @transactions.inject(0) { |sum, transaction| sum + transaction.value }
    @transactions.map(&:amount).sum
  end

  # In the next 2 methods, we don't to pass type anymore.
  # We can also remove the private add_transaction method, because it doesn't really serve any purpose. If you wanted to modify the transaction creation process, you should do it in Transaction#initialize.
  # I've also added optional type handling, for fun

  def deposit(amount)
    # add_transaction(amount, :deposit)
    @transactions << Transaction.new(amount)
  end

  def withdraw(amount)
    # add_transaction(amount, :withdraw)
    @transactions << Transaction.new(-amount)
  end

  def statement
    # This can be simplified with the map(&:to_s) syntax
    # @transactions.map { |transaction| transaction.to_s }
    @transactions.map(&:to_s)
    # You may also want to make the call to #join here, instead of in the main loop, because a statement sounds like it should be a string.
  end
end

Edit:

You could also get rid of the Transaction#to_s method and instead modify the Account#statement method to be:

class Account
  def statement
    @transactions.map do |t|
      type = t.amount > 0 ? "deposit" : "withdraw"
      "#{t.timestamp} - #{type} - #{t.amount} USD"
      # Personally, I like the extra line for readability, but
      # you could also write it as:
      # "#{t.timestamp} - #{t.amount > 0 ? 'deposit' : 'withdraw'} - #{t.amount} USD"
    end
  end
end
\$\endgroup\$
  • \$\begingroup\$ No problem! If you have any questions about ruby, feel free to ping me in the ruby chat room \$\endgroup\$ – thesecretmaster Dec 18 '17 at 2:49
  • \$\begingroup\$ I like the way you changed the Transaction#initialize. In my implementation I was raising an exception when the argument passed was invalid. Is that the way to go? \$\endgroup\$ – Vini Brasil Dec 18 '17 at 2:56
  • 1
    \$\begingroup\$ That would also work. By calling #to_f on amount, it will throw an error for anything that does not respond to the #to_f method. For example, if you passed an array, you'd get an error. But anything that can be coerced to be a float will be. \$\endgroup\$ – thesecretmaster Dec 18 '17 at 2:58
  • \$\begingroup\$ Really great improvements. I'll note that the only place you have a local variable is in transaction.to_s. The perfectionist in me would extract type to a private method, or nix it altogether. \$\endgroup\$ – Kori John Roys Dec 28 '17 at 22:38
  • \$\begingroup\$ @KoriJohnRoys Hmmm... maybe. I feel like since it's only used in one place, it's better to just use a local variable. And if you're looking at that method anyways, I feel like the formatting should be mixed into Account#statement instead of Transaction#to_s, like it is. Or at least renamed. Really, a Transaction can't be coerced into a string, it has to be formatted to be one. \$\endgroup\$ – thesecretmaster Dec 29 '17 at 0:40

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.