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

Queues - Anna Barklund BankAccounts #37

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

Conversation

amb54
Copy link

@amb54 amb54 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? The classes can in this way communicate with each other.
What is accomplished with raise ArgumentError? What do you think it's doing? I think it prints the message given, throws an error and exit.
Why do you think we made the .all & .find methods class methods? Why not instance methods? Instance methods is for a unique instance of the class. The class methods deal with all instances for the class. .all and .find concerned all instances of the class.
Why does it make sense to use inheritance for the different types of accounts? The variables and the methods in the account class, could be "reused" in the svaings- and credit-account. Rewriting code does not make sence.
Did the presence of automated tests change the way you thought about the problem? How? Yes, I am starting to take one step at a time, instead of trying to grasp everything at once.

@PilgrimMemoirs
Copy link

Bank Account

What We're Looking For

Feature Feedback
Wave 1
All provided tests pass Well Done
Using the appropriate attr_ for instance variables Savings account has one for fee, but there isn't a fee instance variable. Same for check_withdraw_fee in checking account. We probably don't want check_count to be a writable, so that shouldn't be an accessor. Otherwise, the rest look good.
Wave 2
All stubbed tests are implemented fully and pass "Returns an array of all accounts" block is testing a lot of things at once, should be split into separate tests to give us more context and to know what specifically fails when we run the tests.
Created instances (by calling new) in Account.all Well Done
Used CSV library only in Account.all (not in Account.find) Well Done
Used Account.all to get account list in Account.find Well done
Wave 3
All stubbed tests are implemented fully and pass Well Done
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 Used in checking, not in savings.
Baseline
Used Git Regularly Should commit more often, with every new thing (like a method) added, changed or removed.
Answer comprehension questions First question could use more explanation - why/how is it easier for them to communicate? The other questions look good.
Extras Overall looks good, need to focus on when to (or not to) use attr_ methods.

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.

None yet

2 participants