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

Ashton's Bank Account #31

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

Ashton's Bank Account #31

wants to merge 48 commits into from

Conversation

ashtn
Copy link

@ashtn ashtn commented Feb 27, 2017

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? It's a good way to contain your Class elements, if you are using multiple classes.
What is accomplished with raise ArgumentError? What do you think it's doing? I think raise ArgumentError is some kind of conditional string method, that prints a message to the screen if specific criteria is or isn't met.
Why do you think we made the .all & .find methods class methods? Why not instance methods? Because the information we were looking for was not about 1 specific instance of a class, also it was a great way to pass those methods on to the sub classes.
Why does it make sense to use inheritance for the different types of accounts? Because all accounts in this example inherently had similar functionality, and with was more efficient to make methods for a parent, and pass them on to the child class, rather than writing all new methods for each type of account.
Did the presence of automated tests change the way you thought about the problem? How? While the tests were pre populated, it was very restrictive in how i could think about bank accounts. I also think in theory it would help catch mistakes before they happen, but that didn't happen for me this time.

ashtn added 30 commits February 21, 2017 15:19
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.

Overall nicely done, I put a few comments into the code here and there, but you did well.

end

it "Doesn't modify the balance if the account would go below -$10" do
# TODO: Your test code here!
@account.check_withdraw(200).must_equal 100
end

it "Requires a positive withdrawal amount" do

Choose a reason for hiding this comment

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

This should instead be triggering an ArgumentError

end


def check_withdraw(amount)

Choose a reason for hiding this comment

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

You should be able to call the withdraw method from within check_withdraw.

@CheezItMan
Copy link

Bank Account

What We're Looking For

Feature Feedback
Wave 1
All provided tests pass Check
Using the appropriate attr_ for instance variables Done, nice use of the hash in initialize
Wave 2
All stubbed tests are implemented fully and pass Done, good use of the CSV in testing
Created instances (by calling new) in Account.all Done
Used CSV library only in Account.all (not in Account.find) Done, interesting use of the class variable
Used Account.all to get account list in Account.find Used a class variable instead, good idea!
Wave 3
All stubbed tests are implemented fully and pass Done
Used inheritance in the initialize for both Checking and Savings Accounts (min balance) done
Used inheritance for the withdraw in checking and savings accounts Done with super great!
Baseline
Used Git Regularly Lots of regular commits
Answer comprehension questions Modules: It also will namespace your code to prevent clashes from classes with the same name. ArgumentError causes the method to exit with an error that the calling code will need to handle. It keeps your code from executing with invalid arguments. Otherwise good answers.

Summary

Overall nicely done, I put a few comments into the code here and there, but you did well.

@ashtn
Copy link
Author

ashtn commented Mar 1, 2017 via email

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