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 - Kayla Kubicke - Bank Accounts #36

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

Queues - Kayla Kubicke - Bank Accounts #36

wants to merge 12 commits into from

Conversation

ghost
Copy link

@ghost ghost 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? Putting classes inside a module allows you organize your classes (and their names) in some fashion.
What is accomplished with raise ArgumentError? What do you think it's doing? A 'raise ArgumentError', as far as I understand it, basically stops the program from executing in a situation were it really shouldn't.
Why do you think we made the .all & .find methods class methods? Why not instance methods? All of the accounts, including the inherited ones, needed the 'traits' inside those two methods.
Why does it make sense to use inheritance for the different types of accounts? The accounts have similar properties and inheritance allows you to grab 'the basics' of the parent class and then customize as you wish.
Did the presence of automated tests change the way you thought about the problem? How? Yes, very much so. I had to think about the interaction going on between the program and the tests. It was frustrating, but ultimately helpful.

@droberts-sea
Copy link

droberts-sea commented Feb 28, 2017

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
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 no - some are skipped, and one fails
Used inheritance in the initialize for both Checking and Savings Accounts yes
Used inheritance for the withdraw in checking and savings accounts no - see inline comment
Baseline
Used Git Regularly I like how frequently you've been committing, but in the future I would like to see more descriptive commit messages. Try and capture what you accomplished and why, and how the code is different than it was before.
Answer comprehension questions yes

Outside of a couple of parts of Wave 3, this looks solid. Good work!

describe "Account.all" do

before do

Choose a reason for hiding this comment

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

Nice use of before!

require 'csv'

module Bank
class MoneyMarketAccount < Bank::Account

Choose a reason for hiding this comment

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

Why is this MoneyMarketAccount - I would have expected to see CheckingAccount in this file (and that's what you appear to have implemented).

@counter = 0
end

def withdraw(amount)

Choose a reason for hiding this comment

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

It might be cleaner to use super here to call Account#withdraw, and centralize your logic all in one place. In this case Account#withdraw would probably require more parameters, like a fee and a minimum balance.

withdrawal_amount = 2.0
account = Bank::MoneyMarketAccount.new(666, start_balance)

3.times do

Choose a reason for hiding this comment

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

I like that you used a times loop here to DRY up this code. Good work!

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.

1 participant