Bank Account #32
Bank Account #32
Conversation
Market account
There was a problem hiding this comment.
Overall well done, there are some methods that are too long and complicated (finding owners for example). You should have taken some advantage of inheritance (super) and you're missing some things in your testing.
Despite this you did a very good job on testing and good work on the extras. Nice work!
| @open_date = DateTime.parse(open_date) | ||
|
|
||
| # Assumes that required csv file is accesible | ||
| CSV.read("support/account_owners.csv").each do |line| |
There was a problem hiding this comment.
This loop is problematic. It's reading the file and each find is reading another file each time. The find methods should probably be scanning through the Arrays provided by .all so you don't have to repeat things over and over again.
| describe "#initialize" do | ||
| # Check that a CheckingAccount is in fact a kind of account | ||
| it "Is a kind of Account" do | ||
| account = Bank::CheckingAccount.new(12345, 100.0) | ||
| account = Bank::CheckingAccount.new(12345, 10000, "1999-03-27 11:30:09 -0800") |
|
|
||
| # Balance does not change, returns amount | ||
| it "Doesn't modify the balance if the fee would put it below $0.00" do | ||
| Bank::CheckingAccount.new(12345, 10000, "1999-03-27 11:30:09 -0800").withdraw(100000).must_equal 10000 | ||
| end |
There was a problem hiding this comment.
The above two tests should check to ensure the balance doesn't get modified like:
my_checking_acct = Bank::CheckingAccount.new(12345, 10000, "1999-03-27 11:30:09 -0800")
my_checking_acct.withdraw(100000)
my_checking_acct.balance.must_equal 10000You're only checking the return value of withdraw.
| end | ||
|
|
||
| # Method that handles withdraw | ||
| def withdraw(withdraw_amount) |
There was a problem hiding this comment.
Shouldn't you be able to use super in this method to avoid replicating existing functionality.
| end | ||
|
|
||
| # Method that handles withdraw_using_check | ||
| def withdraw_using_check(withdraw_amount) |
There was a problem hiding this comment.
You should be able to use withdraw in this method to save code.
| end | ||
|
|
||
| # Resets the number of checks used to zero | ||
| def reset_checks() |
There was a problem hiding this comment.
The new month functionality wasn't in the requirements and it's making your reset_checks method more cumbersome than it needs to be.
Bank AccountWhat We're Looking For
SummaryOverall well done, there are some methods that are too long and complicated (finding owners for example). You should have taken some advantage of inheritance ( Despite this you did a very good job on testing and good work on the extras. Nice work! |
|
How to withdraw from bank account? |
Bank Account
Congratulations! You're submitting your assignment.
Comprehension Questions
raise ArgumentError? What do you think it's doing?.all&.findmethods class methods? Why not instance methods?