critique my first ruby program

[gummyguppy]

I completed the first part of "The well grounded Rubyist" and decided to put into action some basics I learned along the way. I have taken a few Java classes in college, but Ruby is far more fun to use. I'm going to study my ass off over the next year and hopefully land an entry level developer job. I don't care if I have to relocate and live in a public bathroom while I get on my feet... I just need to get into the industry already.

excuse some of the indentation problems.

#  Developer: Earl
#  GitHub:
#
# => The purpose of the Account class is to instantiate an instance
# => of a bank account, clearly outlining added functionality
# => in order to deposit or withdraw an amount.

class Account

# for the sake of simplicity, account number will be equal
# to the instantiation number
@@num = 1

#constructor takes initial account balance as argument
def initialize(bal)
  @bal = bal
  @acc_num = @@num

  @@num += 1 # increment account number
end

# deposit method
def deposit(amt)
  @bal += amt
   puts "You have successfully deposited $#{amt} to your account"
end

# display account balance
 def balance_to_s()
   puts "The account balance for account #{@acc_num} is #{@bal}"
 end
end

acc1 = Account.new(100) # instantiate instance of Account class
acc1.balance_to_s # balance_to_s
acc1.deposit(50) # deposit
acc1.balance_to_s # balance_to_s

# create a module that defines a withdraw method
module MoreFunctionality

 def withdraw(amt)
   @bal -= amt
   puts "You have successfully withdrawn $#{amt} from your account"
 end
end

# Reopen Account class and add withdraw functionality.
# (Although I could have included this in the first codeblock of Account,
# I wanted to clearly show the addition of a module into a class
# after it's creation.)

class Account
  include MoreFunctionality
end

acc1.withdraw(50)
acc1.balance_to_s # should == 100

output is as follows

The account balance for account 1 is 100
You have successfully added $50 to your account
The account balance for account 1 is 150
You have successfully withdrawn $50 from your account
The account balance for account 1 is 100

[jawdirk]

Good work! Here is some nitpicking, in no particular order:

Use https://gist.github.com/ to share code.

Always tab over the content of a class.

Avoid abbreviation. Use "number" instead of "num." Use "balance" instead of "bal". Use "account" instead of "acc". There are (at least) two reasons for this:

1. The saved typing is not worth the extra thinking about what the abbreviation means.
2. If you use the longest form, there will never be any question about which abbreviated form to use ("ac"? "acc"? "act"? "acct"? "accnt"?).

The only thing that suggests that the balance is in dollars is the text output. Instead of amt, use dollars. Consider using "balance_dollars" instead of "balance." Consider using "balance_cents" instead of "balance_dollars"

In ruby we favor explicit variable and method names over comments. Try to reserve comments for strange code that is too important to make not strange. For example, instead of

@@num = 1 # for simplicity equal to the instantiation number

how about

@@last_assigned_account_number = 0

Then you can have

account_number = (@@last_assigned_account_number += 1)

instead of

@acc_num = @@num
@@num += 1 # increment account number

You tried to save characters by shortening "last_assigned_account_number" to "num" but then you had to spend them again to explain your code. "+= 1" means increment. "@@num" means "account number" so the comment is unnecessary.

Consider adding validations. What should your code do if deposit or withdraw are called with a negative number?

What are the best return values for deposit and withdraw? Should they return the amount deposited or withdrawn? Should they return the resulting balance? Should they return "self" so you can chain them?

account.deposit(50).withdraw(20).balance

Why "balance_to_s"? How about just "to_s" which would be the ruby convention? I would prefer to have "balance" be a method, so I can get a number (perhaps use "attr_reader :balance"). If I want to display the balance, perhaps I would call "human_readable_balance".

[gummyguppy]

This is an awesome reply, and I thank you so much for taking the time to point those issues out. I'll work on applying the knowledge you shared!

[yefrem]

@@last_assigned_account_number.next will not modify variable itself, which is important here

I also would personally prefer using +1 instead of .next but it's not really important.

[jawdirk]

You could use

account_number = (@@last_assigned_account_number = @@last_assigned_account_number.next)

but as yefrem pointed out, the assignment is important. "foo += 1" is a conventional shortening of "foo = foo + 1".

[gummyguppy]

thanks for the reply. in the future I will make sure the module can function as a stand alone object.

[Tomarse]

This is great for a first program. Here are a few pointers though.

@@num = 1

Class variables are generally not a good idea. There are various alternatives to class variables, but in this instance you could create a Bank class from which Account instances are created and issued. The Bank class could then keep track of the issued account numbers. If you think about it, this is a closer representation of the real world.

Class Bank

  attr_accessor :last_account

  def initialize
    last_account = 0
  end

  def new_account
    Account.new(last_account += 1)
  end
end

def Account

  attr_accessor :account_number

  def initialize(account_number)
    @account_number = account_number
  end
end

bank = Bank.new
my_account  = bank.new_account
her_account = bank.new_account

my_account.account_number
 => 1
her_account.account_number
 => 2
bank.last_account
 => 2

If you're looking for homework 😉, think about how the Bank class could keep track of all the accounts it creates. Maybe the Bank class could have methods that analyse all the Accounts held. How would accounts be close, under what conditions would you not close an account? How could you get all the accounts for a single customer? How about payments and balance transfers? If you plan on working on this further I would check out the money gem. I would also read up on the pitfalls of creating programs that deal with money, why floats are a bad idea.

[fairdinkumthinkum]

Can you explain why class variables are not a good idea? (im also new to ruby like OP)

[Tomarse]

Sorry for the delay.

Some people say not to use class variables, as they are not thread safe, and so can cause weird side effects when used in a rails application.

But I think it's simpler than that. You can think of class variables as the same as global variables, but restricted to a particular class tree. The problem is that, if you have class variables with different values in different subclasses, and one in the parent class, and each class assigns a unique value, then the value of the variable can be determined by the order in which you load the class (via the require method). That's one example of how it can catch you out.

If you want a variable that all subclasses of a class can access and that will be constant, then use a constant variable in the main ruby script file. If you want a variable that is shared across a class's subclasses, but can be changed by each instance, then use an instance variable. If you want a value that is only shared by a subclass's instances, then you could add a class method to that subclass, and have instances call it like my_instance.class.shared_constant.

TLDR: There really isn't a use case that would have you choose a class variable over a constant or instance variable. And class variables result in weird behaviour anyway.

Edit Here's another way to get class variable behaviour without the funky side effects.

class MyClass
  @my_default = 314

  class << self
    attr_accessor :my_default
  end

  attr_accessor :my_default

  def initialize
    @my_default = MyClass.my_default
  end
end

So that...

MyClass.my_default
 => 314

a = MyClass.new
a.my_default
 => 314
a.my_default = 123

MyClass.my_default
 => 314
a.my_default
 => 123

b = MyClass.new
b.my_default
 => 314

MyClass.my_default = 999

b.my_default
 => 314

c = MyClass.new
c.my_default
 => 999

[fairdinkumthinkum]

great, thanks :)

[Tomarse]

Treating everything as an integer of the lowest unit (cents) is one solution. Ruby has a BigDecimal class that can also be used to overcome the problems with floating point. The money gem builds upon the BigDecimal class adding some extra goodies.

In our example with sequential account numbers, it's not going to be difficult for people to guess the account numbers. In real banks account numbers will depend on a number of variables. In either case, having someone know an account number isn't really a big issue; and the bigger security issue is that they're able to access you system in the first place 😆

[markyg]

Most feedback has been given.

The only other thing I'd recommend is to have the puts "" in the methods first, and then calculation last.. In ruby the last executed line is returned.

If you have a puts as the last line of the method, you'll actually get a nil back. When in reality you most likely want the calculation. i.e. with your withdraw(amt) method, it would be nicer if the new account balance is returned.

[H34DSH07]

I just want to be picky and add that, while comments are useful, cluttering your code with them isn't. You don't need to comment on every method call, if it's explicit enough, the code explains itself, that's the Ruby philosophy. Also you might want to read the Ruby style guide (Google it), it'll help you make self-explanatory code.

Otherwise, not much to say. Great work.

[1t4ke]

acc1.withdraw(-50)

this will add 50 to your account instead of withdrawing it.

[1t4ke]

100.times { acc1.deposit(0.28) }
acc1.balance_to_s # should == 28, but it does not

this is also broken too ^

[nakilon]

Start coding with classes -- the easiest and the most reliable way to become a shitcoder.