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

Janice's Bank Accounts. At long last. #48

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

Janice's Bank Accounts. At long last. #48

wants to merge 20 commits into from

Conversation

janicewilson
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? Organization. Ability to simplify naming conventions.
What is accomplished with raise ArgumentError? What do you think it's doing? A notification is raised when the boolean value is false. The object is coupled with an inherent method that puts forth the relevant notification.
Why do you think we made the .all & .find methods class methods? Why not instance methods? .all and .find is not particular to a specific instance, but to the class itself.
Why does it make sense to use inheritance for the different types of accounts? Shortcut. Automatically replicates shared attributes or actions.
Did the presence of automated tests change the way you thought about the problem? How? Forced me to think more incrementally.

So sorry this is late! Alena showed me how to right my wrongward ways!

@kariabancroft
Copy link

kariabancroft commented Feb 28, 2017

Bank Account

What We're Looking For

Feature Feedback
Wave 1
All provided tests pass Yes. A couple of style notes: (1) you are using too much whitespace between lines of code. You should use a new line only after method definitions, separate blocks (like using an each) or other large blocks of code. Right now this is difficult to read. (2) Your attrs and initialize should be at the top of the class definition, even if you have some class methods inside. (Right now they're pushed down below the class methods)
Using the appropriate attr_ for instance variables Using attr_accessors - though I think that these might make more sense as readers.
Wave 2
All stubbed tests are implemented fully and pass Yes. The find_with_index method is cool!
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. The only problem with including a parameter in the all method to provide the file is that then when finding the accounts you have to provide the file name. I might recommend a constant next time for the file name if you want to make your all method more extensible (for this example, at least).
Wave 3
All stubbed tests are implemented fully and pass Yes. Tests look good. You could DRY things up a bit by using a before block for the instantiation of the account objects. Sometimes you are using a local variable start_balance unnecessarily. It's good when it needs to be compared against, but it seems like sometimes it was a copy/paste issue.
Used inheritance in the initialize for both Checking and Savings Accounts (min balance) Using inheritance in the initialize, but it is duplicating the savings account minimum balance logic.
Used inheritance for the withdraw in checking and savings accounts No - the withdraw logic is duplicated across each account class. You should be able to re-use at least some of this logic from the original Account class.
Baseline
Used Git Regularly Yes - regular commits and meaningful commit messages
Answer comprehension questions Yes - i'd like to gain even more insight into your thoughts about TDD and the new concepts you're learning in your next set of comprehension questions.
Overall Nice job. I think there are a few more opportunities to utilize the inheritance concept on this one. Your tests look really good! The formatting note about whitespace is important so I'd like to see this adjusted in your next submission.

@kariabancroft
Copy link

Bank Account

What We're Looking For

Feature Feedback
Wave 1
All provided tests pass Yes. A couple of style notes: (1) you are using too much whitespace between lines of code. You should use a new line only after method definitions, separate blocks (like using an each) or other large blocks of code. Right now this is difficult to read. (2) Your attrs and initialize should be at the top of the class definition, even if you have some class methods inside. (Right now they're pushed down below the class methods)
Using the appropriate attr_ for instance variables Using attr_accessors - though I think that these might make more sense as readers.
Wave 2
All stubbed tests are implemented fully and pass Yes. The `find_with_index
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. The only problem with including a parameter in the all method to provide the file is that then when finding the accounts you have to provide the file name. I might recommend a constant next time for the file name if you want to make your all method more extensible (for this example, at least).
Wave 3
All stubbed tests are implemented fully and pass
Used inheritance in the initialize for both Checking and Savings Accounts (min balance)
Used inheritance for the withdraw in checking and savings accounts
Baseline
Used Git Regularly Yes - regular commits and meaningful commit messages
Answer comprehension questions Yes - i'd like to gain even more insight into your thoughts about TDD and the new concepts you're learning in your next set of comprehension questions.
Extras

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