Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Brenna's BankAcccounts #20

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open

Brenna's BankAcccounts #20

wants to merge 25 commits into from

Conversation

bcmdarroch
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 are an efficient way to relate classes to one another.
What is accomplished with raise ArgumentError? What do you think it's doing? ArgumentErrors can be used to let a user know what exactly is going wrong. Testing that we 'raised an ArgumentError' verifies that the code can correctly recognize specific kinds of errors.
Why do you think we made the .all & .find methods class methods? Why not instance methods? We needed to keep track of all the instances of Accounts created. If .all and .find were instance methods, they would reset with the creation of each new Account, thus rendering them useless for tracking other Accounts.
Why does it make sense to use inheritance for the different types of accounts? Since you can deposit and withdraw in both Savings and Checking accounts, it makes sense that they can inherit those methods, with some added specifications for each class according to its rules.
Did the presence of automated tests change the way you thought about the problem? How? Automated testing made it difficult for me to get started initially, as I thought I had to write all my tests at once. It became easier once I realized I can just write the simplest tests first and then start moving back and forth between testing and coding. Using automated tests forced me to be more particular about my code and its output. It also made me more systematic in writing my code.

Copy link

@CheezItMan CheezItMan left a comment

Choose a reason for hiding this comment

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

Nice work overall. Just a few comments in the code here.

return @all_accounts if @all_accounts
@all_accounts = []
CSV.open("/Users/brenna/ada/week3/BankAccounts/support/accounts.csv").each do | line |
@all_accounts << Bank::Account.new(line[0].to_i, line[1].to_f, line[2])

Choose a reason for hiding this comment

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

You can get this to work with: support/accounts.csv instead, that way it will work if you move the project to another folder.

return @all_owners if @all_owners
@all_owners = []
CSV.open("/Users/brenna/ada/week3/BankAccounts/support/account_owners.csv").each do | line |
owner_hash = {

Choose a reason for hiding this comment

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

Similar to the above, you can use support/account_owners.csv.

@@ -0,0 +1,30 @@

Choose a reason for hiding this comment

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

The file should probably be named savings_account.rb instead, but not a big deal.

@check_count = 0
end

def withdraw(withdrawal_amount)

Choose a reason for hiding this comment

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

Could you use super in this method?

print "There must always be at least $10 in your savings."
return @balance
end
super

Choose a reason for hiding this comment

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

Good use of super.

@CheezItMan
Copy link

Bank Account

What We're Looking For

Feature Feedback
Wave 1
All provided tests pass Yes, although I had to change the path of your CSV.read commands.
Using the appropriate attr_ for instance variables Well done
Wave 2
All stubbed tests are implemented fully and pass Check
Created instances (by calling new) in Account.all Check
Used CSV library only in Account.all (not in Account.find) Check
Used Account.all to get account list in Account.find Well done
Wave 3
All stubbed tests are implemented fully and pass Check
Used inheritance in the initialize for both Checking and Savings Accounts (min balance) Well done
Used inheritance for the withdraw in checking and savings accounts You could have used super in the CheckingAccount withdrawal method. I left a comment in the code in your PR.
Baseline
Used Git Regularly It looks like as time went by you were committing less and less, you started strong, but maybe got tired. You did a pretty good job of committing as you want.
Answer comprehension questions Modules: modules are more than a way to group classes together they also let you namespace things so that you don't have naming conflicts. .all and .find methods are class methods because they don't deal with a specific instance of an Account. They deal with the collection of Accounts in the file. I'm glad testing helped you be specific in your coding.

Summary

Nicely done overall. Some of your methods most notably, withdraw_using_check are a bit long and could use some refactoring, but you did a pretty good job overall.

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