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

Monalisa's Bank Accounts #39

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

Monalisa's Bank Accounts #39

wants to merge 23 commits into from

Conversation

MonalisaC
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? for namespace which is abstract container or environment created to hold a logical grouping of unique identifiers or symbols (i.e. names)
What is accomplished with raise ArgumentError? What do you think it's doing? It raises an exception and it is raised when the arguments are wrong
|

| Why do you think we made the .all & .find methods class methods? Why not instance methods? | It exist in the object that defined them (the class) and nowhere else and if we do instance method it'll give nomethod error |
| Why does it make sense to use inheritance for the different types of accounts? | lowers down the hierarchy |
| Did the presence of automated tests change the way you thought about the problem? How? | yes it helps to define the problem and increases its scope for further changes.|

@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 Not quite. The assertions for the all method should be split into separate it blocks to make it more clear which assertions go with which specific situations. Your find tests aren't quite testing what the description is asking for. If you have multiple tests with exactly the same assertion, there should be a clue that we're looking for something else.
Created instances (by calling new) in Account.all Yes
Used CSV library only in Account.all (not in Account.find) No - you are unnecessarily duplicating the CSV reading code in the find method. You should use the all method in the find method so you are not duplicating this code.
Used Account.all to get account list in Account.find No - see above.
Wave 3
All stubbed tests are implemented fully and pass Yes. There is some opportunity for improvement in DRYing up the tests. You can add a before block and put the .new calls in here.
Used inheritance in the initialize for both Checking and Savings Accounts (min balance) No - no inheritance was used in the initialize methods.
Used inheritance for the withdraw in checking and savings accounts No - no inheritance was used in the withdraw methods.
Baseline
Used Git Regularly You did a good job at the start of the project, but Friday only have one commit. Use some sort of reminder to make sure you are always committing regularly.
Answer comprehension questions Yes. Still seems like there is some confusion with these major topics. We define a class method when the method related to the class as a whole rather than an individual instance of the class. We use inheritance when we want to share some behavior across our classes, when we want to extend the behavior of a class we've created. Account is our parent class and SavingsAccount is an example of a subclass.
Code formatting You need some work still on formatting. You should always use the atom shortcut to fix the formatting. You also need to put one line of whitespace (hitting the "enter" key) between your method definitions (lines 40-41, 46-47 of the Account class)
Overall It seems like you missed the important aspects of inheritance in this assignment. By re-writing each method that was in the Account class, you are not saving yourself any additional work. It does seem like you got good practice with creating tests here, so hopefully you feel more confident with that topic.

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