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 - Sofia Kim - BankAccounts #21

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

Conversation

Sofia15
Copy link

@Sofia15 Sofia15 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? Allows for us to keep all related classes in the same namespace (module).
What is accomplished with raise ArgumentError? What do you think it's doing? Identifies where errors can be possible and if an error occurs, will either terminate the program or execute the rescue associated with the ArgumentError.
Why do you think we made the .all & .find methods class methods? Why not instance methods? .all & .find class methods were needed to make the csv file available across all accounts instead of on specific instance.
Why does it make sense to use inheritance for the different types of accounts? To allow us to utilize and modify instance methods and variables efficiently from the parent class by using 'super' instead creating new methods and variables each time.
Did the presence of automated tests change the way you thought about the problem? How? Yes. The automated tests have made me think about what would make the program run into errors and what errors I should be considering, including results that are not desired. A program can be great with the proper inputs but we have to also make sure that bad inputs are accounted for in some way as well.

|

@droberts-sea
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 mostly - see inline comments
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 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

it "Can find the first account from the CSV" do
# TODO: Your test code here!
acct_id_1212 = Bank::Account.find(1212)
acct_id_1212.must_be_instance_of Bank::Account #Account class

Choose a reason for hiding this comment

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

In addition to checking that what you got back from Account.find is an account, you should verify that the ID matches what you asked for. As is, find could return any old account and your test would still pass.

end

def self.all
all_accounts = [] #array of classes

Choose a reason for hiding this comment

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

Good work on Account.all and Account.find, I feel like this implementation is about as clean as it gets.

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