Skip to content

Conversation

@marisol-lopez
Copy link

Bank Account

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Answer
Why is it useful to put classes inside modules? What do you think? Modules provide the ability to use the functionality of other classes and methods without having name confliction.
What is accomplished with raise ArgumentError? What do you think it's doing? I think that I'm telling my code to look out for this specific possible error, and then if it happens, it'll let raise the error to alert.
Why do you think we made the .all & .find methods class methods? Why not instance methods? Because we'd be able to access the different methods without having to create instances of the class
Why does it make sense to use inheritance for the different types of accounts? because you can use the functionality of the parent class without having to rewrite all of the code.
Did the presence of automated tests change the way you thought about the problem? How? Yes, I think it forced me to be more concise/focused in what I wanted each block of code to accomplish

@droberts-sea
Copy link

Bank Account

What We're Looking For

Feature Feedback
Wave 1
All provided tests pass yes
Using the appropriate attr_ for instance variables see inline comment
Wave 2
All stubbed tests are implemented fully and pass yes
Created instances (by calling new) in Account.all yes
Used CSV library only in Account.all (not in Account.find) yes
Used Account.all to get account list in Account.find yes
Wave 3
All stubbed tests are implemented fully and pass yes
Used inheritance in the initialize for both Checking and Savings Accounts yes
Used inheritance for the withdraw in checking and savings accounts no - see inline comment
Baseline
Used Git Regularly I like how frequently you've been committing, but in the future I would like to see more descriptive commit messages. Try and capture what you accomplished and why, and how the code is different than it was before.
Answer comprehension questions yes

Excellent work overall!


module Bank
class Account
attr_accessor :id, :balance, :date_created

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these be attr_accessor? Seems to me that attr_reader would be more appropriate - you don't want people trying to modify @balance directly, they should go through withdraw or deposit instead.

end

def withdraw(amount)
if amount < (@balance - 11)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to refactor your withdraw methods so that the versions in the subclasses could take advantage of Account.withdraw? You might need to add some extra information about fees or minimum balances, but it has the potential to make the code much DRYer.

It looks like you may have been headed way with the @fee instance variable, but I don't think it ever gets used.


module Bank
class CheckingAccount< Account
attr_accessor :id, :balance

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out the attr_accessor methods are one of the things we get for free via inheritance, so this line is not needed.

account = Bank::CheckingAccount.new(1337, start_balance)
new_balance = start_balance - ((withdrawal_amount * 7) + 4)

5.times do

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the idea of using a times loop here to avoid writing the same code many times. Good work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants