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 BankAccount Laura #41

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

Conversation

lmelgarejos
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? To avoid conflict with others methods that could been named the same.
What is accomplished with raise ArgumentError? What do you think it's doing? I think it is letting us know when the program is not doing what its suppose to do.
Why do you think we made the .all & .find methods class methods? Why not instance methods? They are methods that apply for all the instances of the class Accounts, but I am not very sure why. I do not see the benefit right now.
Why does it make sense to use inheritance for the different types of accounts? Because there share similar characteristics, and we can rewrite methods to execute different if we want it.
Did the presence of automated tests change the way you thought about the problem? How? Sometimes was very clear what I needed to do based on what the tests were testing, but the majority of time I felt a bit confused.

@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 comment
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 mostly - see inline comment
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 no - see inline comment
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

This is pretty good, and you seem to understand the core concepts well enough, but there are several places where the details don't quite match what we wanted. Please take some time to go through the inline comments, and feel free to reach out to Jamie or I if you have any questions or want to work through something.

end

def self.find(id1)
@accounts_array.each do |line|

Choose a reason for hiding this comment

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

I like the idea of using the accounts loaded in Account.all here in find. But there's a problem - if find is run before all, you get an error! How could you guarantee that all has been run by the time you get here?

end

def self.all
@accounts_array = []

Choose a reason for hiding this comment

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

You probably want a class variable (two @s) here. What you'v got is an instance variable defined at the class level, which can act a little weird in some situations. Better to just stick with the class variable, at least for now.

end

it "Can find the first account from the CSV" do
# TODO: Your test code here!
Bank::Account.all[0].id.must_equal 1212
end

Choose a reason for hiding this comment

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

Since this test is under describe "Account.find", it ought to be using the find method. How might you modify it to test what it says it tests?

@checks = 0
end

def withdraw(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.

# TODO: Your test code here!
end

it "Doesn't modify the balance if the account would go below -$10" do

Choose a reason for hiding this comment

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

It looks like you removed a test here, possibly by accident?

balance = 100
amount = 10
account = Bank::CheckingAccount.new(17682, balance)
3.times {balance = balance - amount}

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 times 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.

2 participants