Haby Randall Bank Accounts by habypsow · Pull Request #30 · Ada-C7/BankAccounts

habypsow · 2017-02-27T08:02:34Z

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?	It is useful to place classes inside modules because they are a way of grouping together methods, classes, constants, etc. In this exercise, we were able to utilize the same module for multiple classes and methods.
What is accomplished with `raise ArgumentError`? What do you think it's doing?	The test is ensuring that if something that makes the code not behave in the way that we want it to behave, that an error of the user choice will be placed. It is to ensure that we are getting the behavior we are looking for.
Why do you think we made the `.all` & `.find` methods class methods? Why not instance methods?	I think the class methods were used because we are looking at the class as a whole in those specific examples, and not at single instances of the class there.
Why does it make sense to use inheritance for the different types of accounts?	We were able to adapt specific behaviors desired from the parent class to the subclass, and this way were able to keep our code dry by not repeating code.
Did the presence of automated tests change the way you thought about the problem? How?	Yes, I had to think of building a test before writing the code, which I can see will be useful in writing code that is accurate and precise as I improve with this skill.

…ing account

…t reset_checks method

… failures at this point

CheezItMan · 2017-02-28T00:26:46Z

Bank Account

What We're Looking For

Feature	Feedback
Wave 1
All provided tests pass	Yes, once I changed the `CSV.read`, see the other comment.
Using the appropriate `attr_` for instance variables	Perfect
Wave 2
All stubbed tests are implemented fully and pass	Check
Created instances (by calling `new`) in `Account.all`	Check
Used `CSV` library only in `Account.all` (not in `Account.find`)	Check, well done!
Used `Account.all` to get account list in `Account.find`	Well done!
Wave 3
All stubbed tests are implemented fully and pass	Check
Used inheritance in the initialize for both Checking and Savings Accounts (min balance)	Check
Used inheritance for the withdraw in checking and savings accounts	You did, although you've got an error in `SavingsAccount` withdrawal and you have the same error in `CheckingAccout`. I also noticed you didn't test the CheckingAccount withdraw method.
Baseline
Used Git Regularly	Lots of commits over time, nicely done.
Answer comprehension questions	Modules: Using modules allows us to namespace our classes so we can avoid naming conflicts. ArgumentError: It allows us to force users of our class to only provide usable data. Inheritance: perfect answer.
Extras
Owner Class	Looks like you got started on Owner

Summary

Not bad, a few problems with your inheritance, you could use super in places to DRY your code a bit more. You also had a few bugs handling you withdrawal both in your tests and code. I've put in comments where.

Overall you did a fine job on a hard project. Nice work.

CheezItMan

A few comments in the code, just so you can see where you had small problems.

CheezItMan · 2017-02-27T23:54:22Z

lib/account.rb

+    def self.all
+      account = []
+      CSV.open("/Users/habsr/ada/projects/BankAccounts/support/accounts.csv", "r").each do |file|
+        id = Integer(file[0])


To make it so it'll work no matter where you clone the repo this should be:

CSV.open("support/accounts.csv", "r").each do |file|

CheezItMan · 2017-02-28T00:03:15Z

lib/savings_account.rb

+        puts "Warning, this will make your balance below your mininum , the current balance is " + (@balance.to_s)
+        @balance
+      else
+        super + (-2)


This should probably be:

super(withdrawal_amount + 2) because you're taking the return value of the Account class' withdraw method and subtracting two from it. You're not changing @balance in the end.

Let me know if this is confusing.

CheezItMan · 2017-02-28T00:03:24Z

specs/savings_account_spec.rb

-      # TODO: Your test code here!
+      savings_account = Bank::SavingsAccount.new(11, 200)
+      new_balance = savings_account.withdraw(10)
+      new_balance.must_equal 188


If you run the test

savings_account.balance.must_equal 188

here, it will fail. It's because the balance isn't getting updated. when you call super in the savings account, you should be passing it the amount + 2 to ensure it adds in the fee. You're only getting the return value from the withdraw method not the balance attribute.

CheezItMan · 2017-02-28T00:09:01Z

lib/checking_account.rb

+  class CheckingAccount < Account
+    attr_reader :id, :balance
+    def initialize(id, balance)
+      @id = id


super would be swell here!

CheezItMan · 2017-02-28T00:17:24Z

lib/checking_account.rb

+        if check_balance < 0
+          puts "Warning, this will be below 0 , " + (@balance.to_s)
+        else
+          super + (-1)


Same problem with CheckingAccount earlier.

CheezItMan · 2017-02-28T00:19:33Z

lib/checking_account.rb

+      @balance = balance
+      @check_num = 3
+
+      def withdraw(withdrawal_amount)


I don't see any testing for this method...

CheezItMan · 2017-02-28T00:25:09Z

lib/savings_account.rb

+    attr_reader :id, :balance, :interest
+
+    def initialize(id, balance)
+


You can probably use super to set the instance variables.

habypsow added 26 commits February 21, 2017 15:05

Started Bank module

4c6f7db

Passing withdrawal tests

c7f1bea

need to add withdrawal

fc87609

Wave 1 done

ff71675

Added self.all class, starting to write tests

d7ba438

Starting tests, wrote find.all method

5803629

Added more tests in .all method

b28a680

more tests added

a57d631

Finished round 1 tests. Moving on to Find.all tests

c9eca1b

Passed first find test

e210de6

Wave 2 primary done

3651023

Fixed errors I was having with Wave 2 and adding inheritance to check…

fbd9277

…ing account

Set crazy warnings in Rakefile to false

4a88a92

Working on withdraw_with_check method

e6c979c

Started Savings Account Inheritance, still working on Checking Accoun…

0881911

…t reset_checks method

SavingsAccount withdraw method initialized

897a05e

Working on Savings Account, getting failure with balance below 0 test.

c9db8c4

Working on savings_account, finished withdraw method

1671c8b

Updated interest calculation for savings_account

f9af0b6

savingd_account.rb primary potentially done

5ddf712

refined withdraw_with_check method for checking_account.rb

44d888c

Refactor withdraw_with_check method in checking_account.rb

e6a8ccc

Refactored withdraw_with_check method in checking_account.rb, with no…

dea67d0

… failures at this point

Primary Requirements Done

0c0d33f

Bank Accounts submitting

6a90016

Removed extraneous interest variable from savings_account

3a8e0e8

CheezItMan reviewed Feb 28, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Haby Randall Bank Accounts #30

Haby Randall Bank Accounts #30
habypsow wants to merge 26 commits intoAda-C7:masterfrom
habypsow:master

habypsow commented Feb 27, 2017

Uh oh!

CheezItMan commented Feb 28, 2017

Uh oh!

CheezItMan left a comment

Uh oh!

CheezItMan Feb 27, 2017

Uh oh!

CheezItMan Feb 28, 2017

Uh oh!

CheezItMan Feb 28, 2017

Uh oh!

CheezItMan Feb 28, 2017

Uh oh!

CheezItMan Feb 28, 2017

Uh oh!

CheezItMan Feb 28, 2017

Uh oh!

CheezItMan Feb 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		attr_reader :id, :balance, :interest

		def initialize(id, balance)

Conversation

habypsow commented Feb 27, 2017

Bank Account

Comprehension Questions

Uh oh!

CheezItMan commented Feb 28, 2017

Bank Account

What We're Looking For

Summary

Uh oh!

CheezItMan left a comment

Choose a reason for hiding this comment

Uh oh!

CheezItMan Feb 27, 2017

Choose a reason for hiding this comment

Uh oh!

CheezItMan Feb 28, 2017

Choose a reason for hiding this comment

Uh oh!

CheezItMan Feb 28, 2017

Choose a reason for hiding this comment

Uh oh!

CheezItMan Feb 28, 2017

Choose a reason for hiding this comment

Uh oh!

CheezItMan Feb 28, 2017

Choose a reason for hiding this comment

Uh oh!

CheezItMan Feb 28, 2017

Choose a reason for hiding this comment

Uh oh!

CheezItMan Feb 28, 2017

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants