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 -- Tehut Getahun -- Bank Accounts #35

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

Conversation

tehut
Copy link

@tehut tehut 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? Putting classes inside modules allows us to group and use similar code without worrying about variable names overlapping.
What is accomplished with raise ArgumentError? What do you think it's doing? We are anticipating a specific, likely common, use case and creating instructions for users to help them use the program correctly.
Why do you think we made the .all & .find methods class methods? Why not instance methods? As class methods they were available to us in other portions of the program and would not have been as instance methods.
Why does it make sense to use inheritance for the different types of accounts? Inheritance allowed us to re-use and modify other classes.
Did the presence of automated tests change the way you thought about the problem? How?
It did! Writing the tests made me think about what might not work and figure out how to measure and code what 'breaking' looked like.

@droberts-sea
Copy link

droberts-sea commented Feb 27, 2017

Bank Account

What We're Looking For

Feature Feedback
Wave 1
All provided tests pass yes
Using the appropriate attr_ for instance variables yes
Wave 2
All stubbed tests are implemented fully and pass yes
Created instances (by calling new) in Account.all yes
Used CSV library only in Account.all (not in Account.find) yes
Used Account.all to get account list in Account.find yes
Wave 3
All stubbed tests are implemented fully and pass yes
Used inheritance in the initialize for both Checking and Savings Accounts (min balance) yes
Used inheritance for the withdraw in checking and savings accounts yes
Baseline
Used Git Regularly In the future I would like to see smaller, more frequent commits, with more descriptive commit messages. A good cadence is usually to commit whenever you've written a new test and gotten it passing.
Answer comprehension questions yes

Great work overall!

end
end
end

describe "Tests that the number of accounts is correct" do

Choose a reason for hiding this comment

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

Watch your indentation through here.

end
end

def withdraw(amount,limit= 10)

Choose a reason for hiding this comment

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

I like how your design of Account#withdraw makes this method and withdraw_using_check both very clean. Good work!

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