-
Notifications
You must be signed in to change notification settings - Fork 23
feat: try to add savingAccount to bankATMLesson 17 #525
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
Conversation
…sson9/dataprovider/montezBProviderProvider.java
|
||
import com.codedifferently.lesson17.bank.exceptions.InsufficientFundsException; | ||
|
||
public class BusinessAccount extends CheckingAccount { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prolly should be called BusinessCheckingAccount
for more clarity.
* @throws InsufficientFundsException If there are insufficient funds in the account. | ||
*/ | ||
@Override | ||
public void withdraw(double amount) throws InsufficientFundsException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't even need to override the superclass method since it does the same thing.
* @param owners The owners of the account. | ||
* @param initialBalance The initial balance of the account. | ||
*/ | ||
public BusinessAccount(String accountNumber, Set<Customer> owners, double initialBalance) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to check that one of the owners is a business, right?
*/ | ||
public Check(String checkNumber, double amount, CheckingAccount account) { | ||
if (amount < 0) { | ||
public Check(String checkNumber, double amount, CheckingAccount account, SavingsAccount savings) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you need to change the constructor? You can only write checks against checking accounts.
|
||
|
||
/** Represents a savings account, which does not support check writing. */ | ||
public class SavingsAccount extends CheckingAccount { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't make sense to me. How is a savings account a kind of checking account?
* @param check The amount to write on the check. | ||
* @throws UnsupportedOperationException Always, since savings accounts don't support checks. | ||
*/ | ||
public void writeCheck(Check check) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why even have this method if you can't write checks account savings accounts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must be still WIP so I'll ignore for now.
} | ||
|
||
@Test | ||
void writeCheck() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code formatting looks wrong, please fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your pull request names are still terrible. You should do something about that.
need an extension