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 - Hyunji Kim - Bank Accounts #26

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

Conversation

ricecakemonster
Copy link

@ricecakemonster ricecakemonster 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? In case there are multiple people writing a program at the same time, and they use the same class name for different functions, that will cause a big chaos when you run the program. Using modules can prevent that problem.
What is accomplished with raise ArgumentError? What do you think it's doing? Letting the users know that what they have entered is not what is expected to be entered.
Why do you think we made the .all & .find methods class methods? Why not instance methods? Because the .all method needs to show the all the accounts not just a single account, which is an instance. The .find method will need to search an account among all the accounts, not within a single account.
Why does it make sense to use inheritance for the different types of accounts? Because all the different types of accounts share the same elements, such as a name and a balance
Did the presence of automated tests change the way you thought about the problem? How? It helped me think of coding as a linear process

@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 Well Done
Wave 2
All stubbed tests are implemented fully and pass Looks great, well done
Created instances (by calling new) in Account.all Not happening in .all, but is creating new instances in within class. Right idea, but could be cleaned up.
Used CSV library only in Account.all (not in Account.find) Not in there, but could be refactored to do so
Used Account.all to get account list in Account.find Used the class variable. If Account.all returned a collection of account objects, it could be used in Account.find to keep code efficient.
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 withdraw methods in child classes are rewriting the method. How can you still utilize the functionality of the parent class' withdraw method, while still being able to add on top of it. Hint: you can use the keyword super here as well!
Baseline
Used Git Regularly Should be committing more often, commit every time you create and change a new thing. Then the message should be specific on what that new thing you did was.
Answer comprehension questions Well Done
Extras Code should be wrapped up in methods, there is some in the Account class that makes it difficult to read and makes it less organized.

Summary:

  • Avoid using class variables, instead have .all method return the collection of objects.
  • Utilize inheritance within methods.
  • Commit more often

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