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

Stacks- Kelly Souza Bank Account #38

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

Conversation

kellysouza
Copy link

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? So you can contain the class behavior within a module and not interfere with other classes
What is accomplished with raise ArgumentError? What do you think it's doing? it out puts an error, and pops you out of the method without breaking the program
Why do you think we made the .all & .find methods class methods? Why not instance methods? Because the methods are being called on the class as a whole not on individual instances of the class.
Why does it make sense to use inheritance for the different types of accounts? less typing! but seriously, if each individual type of account can inherit the behavior, we have dryer code.
Did the presence of automated tests change the way you thought about the problem? How? yes, I learned to take the code writing one step at time and address each step alone. It also helped me be sure I was meeting each requirement before building more code on top of a shaky foundation.

@kariabancroft
Copy link

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. I think you do have extra code in the before block in the find tests. It doesn't look like you are using the @test_array variable in your tests.
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. Nice job. You have some opportunity to DRY up the tests using a before block for the instantiation of the account objects. In one test, you are creating a boolean variable using a < and then checked the equality on that. It would be better to use must_be :< because you'll get a more specific assertion when/if this fails. Also some opportunity for DRYing up the code by using a times loop in the check tests.
Used inheritance in the initialize for both Checking and Savings Accounts (min balance) Yes - nice job creating a method that would utilize the variable set for the minimum balance. It will call this method twice (once from super) but overall, it is good.
Used inheritance for the withdraw in checking and savings accounts Yes - I like the step-by-step approach you created by utilizing some methods.
Baseline
Used Git Regularly Really nice job with frequency and messages
Answer comprehension questions Yes! I particularly liked the way you described your experience with TDD
Overall Nice job overall, you did a nice job using methods and using inheritance to your advantage. Your tests look nice & thorough.

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