3
\$\begingroup\$

I just "finished" a Udemy Java that involves building a banking application.

Here is the problem definition:

Scenario: You are a back-end developer and need to create an application to handle new customer bank requests.

Your application should do the following:

  • Read a .csv file of names, social security numbers, account type and initial deposit
  • Use a proper data structure to hold all these accounts
  • Both savings and checkings accounts share the following properties:
    • deposit()
    • withdraw()
    • transfer()
    • showInfo()
  • 11-Digit Account Number (generated with the following process: 1 or 2 depending on Savings or Checking, last two digits of SSN, unique 5-digit number, and random 3-digit number)

  • Savings Account holders are given a Safety Deposit Box, identified by a 3-digit number and accessed with a 4-digit code

  • Checking Account holders are assigned a Debit Card with a 12-digit number and 4-digitPIN
  • Both accounts will use an interface that determines the base interest rate.

    • Savings accounts will use 0.25 points less than the base rate
    • Checking accounts will use 15% of the base rate
  • The show info method should reveal relevant account information as well as information specific to the Checking account or Savings account

I'm trying to follow the SOLID Principles of Programming while creating new applications. I was hoping to get some feedback on my code, in categories such as things I did well, things I could improve on (such as a better way to implement something). For example, some feedback could be: "Don't do this method in this class, it should be done in a separate class because of this reason."

I will attach my Account and AccountTranscations class in this post. But if you would like to see the other classes used in this project, go to my repository.

Account Class:

package bank;

import utilities.Generate;
import utilities.Watch;

public abstract class Account implements Rate
{
    protected String name;
    protected int ssn;
    protected String dateOfCreation;
    protected long accountNum;
    protected double balance;
    protected double rate;
    protected int accountPin;

    public Account(String name, int ssn, double initialDeposit, int accountPin)
    {
        Watch watch = new Watch();

        this.name = name;
        this.ssn = ssn;
        this.balance = initialDeposit;

        this.accountPin = accountPin;

        dateOfCreation = watch.getDateTime();
        //System.out.println(dateOfCreation);

        rate = getRate();
    }

    //get the rate
    //implementation will be dependent on the type of account
    public abstract double getRate();

    //Getters
    public double getBalance()
    {
        return balance;
    }

    public long getAccountNum()
    {
        return accountNum;
    }

    public int getPin()
    {
        return accountPin;
    }

    public String getName()
    {
        return name;
    }

    //Setters
    //attempt to set a 4-digit pin to the specified account
    public void setPin(int accountPin)
    {
        if (!isValidPinWidth(accountPin)) return;

        this.accountPin = accountPin;
        System.out.println("New Pin Set");
    }

    public void setBalance(double balance)
    {
        this.balance = balance;
    }

    //check if the pin entered is 4 digits
    public boolean isValidPinWidth(int accountPin)
    {
        if ((accountPin > 9999) || (accountPin < 0))
        {
            System.out.println("Failure: Invalid pin.");
            return false;
        }
        else return true;
    }

    //check if the amount requested to withdrawn can be withdrawn from the specified account
    public boolean isValidWithdrawal(double amount)
    {
        if (balance - amount < 0) 
        {
            System.out.println("Failure: Account " + accountNum + " does not have sufficient funds.");
            return false;
        }
        else return true;
    }

    //check if the pin attempted is valid
    public boolean checkPinAttempt(int pinAttempt)
    {
        if (pinAttempt == accountPin)
        {
            //System.out.println("\nValid Pin Entry");
            return true;
        }
        else
        {
            System.out.println("Invalid Pin Entry");
            return false;
        }
    }

    //generate the 11-digit account number
    //1 or 2 depending on Savings or Checking, last two digits of SSN, unique 5-digit number
    //and random 3-digit number
    protected long generateAccountNum(int accountType)
    {
        Generate generate = new Generate();
        long tempAccountNum = (long)(accountType * Math.pow(10, 10));
        tempAccountNum += (ssn % 100) * Math.pow(10, 8);
        tempAccountNum += generate.random(5) * Math.pow(10, 3);
        tempAccountNum += generate.random(3);
        return tempAccountNum;
    }

    //generate a 4 digit pin
    public void generatePin()
    {
        accountPin = (int)(new Generate().random(4));
        System.out.println("New Generated Pin: " + accountPin);
    }

    //attempt to unlock the specified account with the specified 4-digit pin
    public boolean unlock(int pinAttempt)
    {

        if (!checkPinAttempt(pinAttempt))
        {
            //System.out.println("Account: Locked");
            //System.out.println("Failure: Incorrect Pin");
            return false;
        }
        else
        {
            System.out.println("Account " + getAccountNum() + ": Unlocked");
            return true;
        }
    }

    //lock the account
    public void lock()
    {
        System.out.println("Account " + accountNum + ": Locked");
    }

    //show everything about the account
    protected void showInfo()
    {
        StringBuilder sb = new StringBuilder();

        sb.append("\nName: " + name);
        sb.append("\nSSN: " + ssn);
        sb.append("\nBalance: $" + balance);
        sb.append("\nRate: " + rate + "%");
        sb.append("\nAccount Number: " + accountNum);
        sb.append("\nAccount Pin Number: " + accountPin);
        sb.append("\nDate of Account Creation: " + dateOfCreation);

        System.out.println(sb.toString());
    }
}

AccountTransactions Class:

package bank;

import bank.Account;

public class AccountTransactions
{

    public AccountTransactions()
    {

    }

    //print the type of transaction being made
    private void printTranscation(Account account, double amount, String transaction)
    {
        System.out.println("Account Number: " + account.getAccountNum());
        System.out.println(transaction + ": $" + amount);
        //System.out.println("New Balance: $" + account.getBalance());
    }

    //print the type of request being made
    private void printRequest(String request)
    {
        System.out.println("\n" + request + " Request Recieved");
    }

    //deposit the specified amount of money  into the specified account
    public void deposit(Account targetAccount, double amount)
    {
        printRequest("Deposit");
        targetAccount.setBalance(targetAccount.getBalance() + amount);
        printTranscation(targetAccount, amount, "Deposited");
    }

    //attempt to withdraw the specified amount of money from the source account
    public void withdraw(Account sourceAccount, double amount)
    {   
        printRequest("Withdraw");
        if (!sourceAccount.isValidWithdrawal(amount)) return;

        sourceAccount.setBalance(sourceAccount.getBalance() - amount);
        printTranscation(sourceAccount, amount, "Withdrawn");
    }

    //attempt to transfer the specified amount of money from the source account
    //to the target account
    public void transfer(Account sourceAccount, Account targetAccount, double amount)
    {
        printRequest("Transfer");
        if (!sourceAccount.isValidWithdrawal(amount)) return;

        withdraw(sourceAccount, amount);
        deposit(targetAccount, amount);
    }

    //calculate the accrued interest and add it to the account's balance
    public void accrueInterest(Account targetAccount)
    {
        printRequest("Accrue Interest");
        double accruedInterest = (targetAccount.getBalance() * (targetAccount.getRate()/100.0));
        targetAccount.setBalance(targetAccount.getBalance() + accruedInterest);
        printTranscation(targetAccount, accruedInterest, "Accrued Interest");
    }
}
\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

Interfaces, Contracts, APIs

Your code do not follow the interface specified. Contracts/APIs may not be well-designed, and indeed usually are not; however they also usually are not modifiable. Therefore it is good practice put them in separate files such that you need not consult docs to see what you can change and what you cannot:

public interface Accout {
    void deposit(BigDecimal amount);
    void withdraw(BigDecimal amount);
    void transfer(Account destination, amount);
    String showInfo();
}

If you want to share behaviors between subtypes then you may put those in a BaseAccount abstract class.

Data representation

Primitive types can be grouped in three: numbers, strings, and time.

If you are not going to do arithmetic on it, it is not a number. Even if it is called a number. You may need to use numbers as keys IRL for efficiency reasons, this is not it.:

protected int ssn;
protected long accountNum;
protected int accountPin;

How you coud you have known this, without experience? Requirements like "should start with ... , must be ... long" these are operations on strings so they should be represented as string.

Never use floating points for money or rates(tax, interest), floating point arithmetic is inexact, order-dependent, not reproducible :

protected double balance;
protected double rate;

Money and rates can be represented as BigDecimals.

Standard representation of date is java.time.LocalDate. Because no requirements exist for this field, it is best to delete it. If you had requirements like "3 months after account creation ...", a string representation is a ton of misery to deal with:

protected String dateOfCreation;
\$\endgroup\$
2
  • \$\begingroup\$ public interface Account { void deposit(BigDecimal amount); void withdraw(BigDecimal amount); void transfer(Account destination, amount); String showInfo(); } So why wouldn't I implement that interface into my AccountTranscation class? I understand the instructions said the subclass Checking and Savings need to share those functions but if I'm following the Single Responsibility rule, why would the Account Class take on those methods? \$\endgroup\$
    – suky
    Aug 21, 2019 at 23:56
  • \$\begingroup\$ Thanks for your advice on Data representation! Will definitely make the necessary adjustments in my code. \$\endgroup\$
    – suky
    Aug 21, 2019 at 23:58
1
\$\begingroup\$

Single responsibility

The account class is responsible for maintaining the account balance, generting account numbers and PIN codes. Instead of using primitive long and int types for account numbers and PIN codes, you should declare classes for both types and have those classes implement the validation.

Logically, the Account class contains information about the account. The account owner SSN should be a member of the class that holds account owner information.

Account number generation, as implmented, does not fulfill the specification. The unique 5 digit part is only a random number which most likely will violate the uniqueness restriction long before the address space is exhausted. Account number generation should be moved to a class that manages accounts so that the uniqueness restriction can be efficiently maintained. The account locking and unlocking should be implemeted in a similar class that implements access management.

The interest rate should be implemented as a reference to an instance of AccountProduct, which is a class that contains static information of a type of an account (e.g. account product name, interest rate, fees, etc).

The purpose of AccountTransactions is not very clear. There are a set of methods that print the parameters they receive without changing the program state. The plural in the class name suggests that it implements a collection of transactions but that doesn't seem to be the case.

And please, for the love of James Gosling, never use floating point types for representing money. They are not accurate.

\$\endgroup\$
3
  • \$\begingroup\$ "Instead of using primitive long and int types for account numbers and PIN codes, you should declare classes for both types and have those classes implement the validation" Could you clarify what you mean by this? I'm just not sure why PIN codes and Account Number would need their own class. \$\endgroup\$
    – suky
    Aug 21, 2019 at 23:27
  • \$\begingroup\$ "Account number generation, as implmented, does not fulfill the specification. The unique 5 digit part is only a random number which most likely will violate the uniqueness restriction long before the address space is exhausted." How does the current implementation not fulfill the specification? I was confused with that part of the problem description, how would that unique 5-digit code be generated? \$\endgroup\$
    – suky
    Aug 21, 2019 at 23:30
  • \$\begingroup\$ "The purpose of AccountTransactions is not very clear. There are a set of methods that print the parameters they receive without changing the program state." I'm working in the develop branch currently, and I use that class to perform transactions within a specific account or between multiple accounts. How could I make that clearer? \$\endgroup\$
    – suky
    Aug 21, 2019 at 23:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.