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

Alison's Bank Accounts #47

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

Alison's Bank Accounts #47

wants to merge 33 commits into from

Conversation

AlisonZ
Copy link

@AlisonZ AlisonZ 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? For clarity, to deal with methods in different modules that may share a name, such as all. in the Bank module it can only be accessed in a Bank instance and so another module would not have access to that method
What is accomplished with raise ArgumentError? What do you think it's doing? Lets user know that their input is not correct, can not be handled by the program. It seems like it also breaks out of the method
Why do you think we made the .all & .find methods class methods? Why not instance methods? I am still unclear as to why class methods are useful. Is it because they can be used in the savings and checking??
Why does it make sense to use inheritance for the different types of accounts? So that shared methods can just occur in the superclass and get passed on, share the load, avoid duplication
Did the presence of automated tests change the way you thought about the problem? How? Not really. They were all things that I would test while building the program, but test them below the code. This is just a different/better way to do that and keep it separate from the coding file.

…he big brother accounts class and tests are clean; using methods for minimum and fee
…les in savings account spec that need to be dealt with
@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 Check
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 Check, well done!
Wave 3
All stubbed tests are implemented fully and pass Missing at least one test, left comments in the PR.
Used inheritance in the initialize for both Checking and Savings Accounts (min balance) Cleverly done!
Used inheritance for the withdraw in checking and savings accounts Check
Baseline
Used Git Regularly Pretty regular commit history, nicely done.
Answer comprehension questions Class Methods: We use class methods for actions that are tied to the entire class and not a specific instance. The .all and .find were methods used to process all the Accounts and not just a single instance.

Summary

Overall nicely done, the use of methods for fee, etc were a nice way to handle withdrawal using inheritance. You did have some things missing in your tests. Overall nicely done.

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.

Nicely done overall, a few comments here and there.



#method which returns 1, so fee for any object in checking account is now 1
def fee

Choose a reason for hiding this comment

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

Interesting how you used this for the inheritance in Checking & Savings.

raise ArgumentError.new("You need some positive cash flow to open an account") if balance < 0
@id = id
@balance = balance
@opendate = opendate

Choose a reason for hiding this comment

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

a more standard name would be @open_date


# Because a CheckingAccount is a kind
# of Account, and we've already tested a bunch of functionality
# on Account, we effectively get all that testing for free!
# Here we'll only test things that are different.

# TODO: change 'xdescribe' to 'describe' to run these tests
xdescribe "CheckingAccount" do
describe "CheckingAccount" do
describe "#initialize" do
# Check that a CheckingAccount is in fact a kind of account
it "Is a kind of Account" do
Copy link

@CheezItMan CheezItMan Feb 28, 2017

Choose a reason for hiding this comment

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

test for it being an Account?


account = Bank::CheckingAccount.new(id, balance)
account.withdraw_using_check(amount)
account.balance.must_be :<, balance

Choose a reason for hiding this comment

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

good use of must_be

end

it "Requires a positive withdrawal amount" do
# TODO: Your test code here!
id = 116

Choose a reason for hiding this comment

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

Where's the attempt to do a negative withdrawal?

amount = 10

account = Bank::CheckingAccount.new(id, balance)
4.times do account.withdraw_using_check(amount) end

Choose a reason for hiding this comment

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

Good use of a loop!

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