1
\$\begingroup\$

RunMyAtm

package ATM;

import java.util.*;
import java.io.*;

public class RunMyAtm 
{   
int input;
static Scanner sc = new Scanner(System.in);
Account[] myAccounts = new Account[3];

public static void main(String[] args)
{   
    RunMyAtm rma = new RunMyAtm();
    rma.preAtmMenu();
}
public void preAtmMenu()
{        
    while (input != 5)
    {
        System.out.println("1.) Populate Accounts");
        System.out.println("2.) Pick Account");
        System.out.println("3.) Load Accounts");
        System.out.println("4.) Save Account");
        System.out.println("5.) Exit");
        System.out.print("Please select one of the options: ");

        input = sc.nextInt();
        System.out.println();

        if (input == 1)
        {
           populateAccts();     
            System.out.println();
        }
        else if (input == 2)
        {
            pickAccts();
            System.out.println();
        }
        else if (input == 3)
        {
            loadAccount();                        
        }
        else if (input == 4)
        {
            saveAccount();
        }
        else if (input <=0 || input >=6)
        {
            System.out.println("Please enter a nubmer from the Menu");
        }            
    }
} 
public void populateAccts()
{        
    for(int i = 0; i < myAccounts.length; i++)
    {
        myAccounts[i]= new Account ((i+1), 100);
        System.out.println(myAccounts[i].getAcctNum());
    }   
}  
public void pickAccts()
{   
    while (input != 4)
    {
        System.out.println("Press 1 for account 1");
        System.out.println("Press 2 for account 2");
        System.out.println("Press 3 for account 3");
        System.out.println("Press 4 to exit");
        System.out.print("Select an account: ");

        input = sc.nextInt();
        System.out.println();

        if (input <1 || input >4)
        {
            System.out.println("Please enter another number");
        }
        else if(input == 1 || input == 2 || input ==3)
        {
            myAccounts[input - 1].AtmMenu();
            saveAccount();
        }
    }           
}   
public void saveAccount()
{
    try
    {
        FileOutputStream outStream = new FileOutputStream("E:/03INFSYS 
3806001 - Mngrl Appl Obj-Orntd Prg
/tempfile1/BankAccounts.txt");
        ObjectOutputStream os = new ObjectOutputStream(outStream);

        os.writeObject(myAccounts);
        os.flush();
        os.close();
    }
    catch (IOException ioe)
    {
        System.err.println(ioe);
    }
}         
void loadAccount()
{
    try  
    {
        FileInputStream inStream = new FileInputStream("E:/03INFSYS
3806 001-Mngrl Appl Obj-Orntd Prg
/tempfile1/BankAccounts.txt");
        ObjectInputStream is = new ObjectInputStream(inStream);
        myAccounts = (Account[])is.readObject();
        is.close();
    }
    catch (Exception ioe)
    {
        System.out.println(ioe.getMessage());
    }      
}

Account

package ATM;


import java.io.Serializable;
import java.text.DecimalFormat;
import java.text.NumberFormat;
import java.text.ParsePosition;
import java.text.SimpleDateFormat;
import java.util.*;


public class Account implements Serializable
{
int acctnum;
double newBalance;
double withdraw;
double deposit;
double amount;
int firstdate;
int seconddate; 
double rate;
Date date = new Date();
boolean dateflag = false;
static Scanner sc = new Scanner(System.in);
Calendar cal1 = new GregorianCalendar();
Calendar cal2 = new GregorianCalendar();
DecimalFormat df = new DecimalFormat("#.##");
static NumberFormat fmt = NumberFormat.getCurrencyInstance(Locale.US);

Account()
{

}

Account(int acctnum, double newBalance)
{
   this.newBalance = newBalance;
   this.acctnum = acctnum;
}
public void setAcctNum(int newId)
{
    acctnum = newId;
}
public int getAcctNum()
{
    return this.acctnum;
}
    public void withdraw(int amount)
{
    System.out.println("Your current balance is :" + 
            fmt.format(this.getNewBalance()) + "\n");
    System.out.print("Enter withdraw amount: ");
    amount = sc.nextInt();
    System.out.println();
    if (this.getNewBalance() >= amount)
    {
        newBalance = this.getNewBalance() - amount;
        System.out.println("Your current balance is: " 
                + fmt.format(newBalance));
    }
    else
    {
        System.out.println("Insufficient Funds Availiable" + "\n");
    }
}    
public void deposit(double amount)
{
    System.out.println("Your current balance is :" 
            + fmt.format(this.getNewBalance()) + "\n");
    System.out.print("Enter deposit amount: ");
    amount = sc.nextDouble();
    newBalance = amount + this.getNewBalance();
    System.out.println("Your new balance is: " + fmt.format(newBalance));
    System.out.println();
}
public void newBalance()
{
    System.out.println("Your balance is: " + fmt.format(newBalance) +"\n");    
}
public double getNewBalance()
{
    return this.newBalance;
}
public void calcInterest()
{
    getDate1();
    getDate2();        

    if (firstdate > seconddate)
    {
        System.out.println("You must enter a future date:");
        getDate2();
    }
    else
    {
        System.out.println(" Thank you:");           
    }

    int datediff = seconddate - firstdate;
    rate = .05/365;
    double ratetime = Math.pow(1+rate,datediff);
    newBalance = getNewBalance() * ratetime; 
    System.out.println("Your Balance with interest is: " 
            + df.format(newBalance));
}
public void getDate1()
{
    System.out.print("Enter first date(mm/dd/yyyy): ");
    String input = sc.next();
    SimpleDateFormat formatter = new SimpleDateFormat("MM/dd/yyyy");
    ParsePosition pos = new ParsePosition(0);
    Date date = formatter.parse(input, pos);
    cal1.setTime(date);
    firstdate = cal1.get(Calendar.DAY_OF_YEAR);
    dateflag = true;
} 
public void getDate2()
{
    System.out.print("Enter second date(mm/dd/yyyy): ");
    String input = sc.next();
    System.out.println();
    SimpleDateFormat formatter = new SimpleDateFormat("MM/dd/yyyy");
    ParsePosition pos = new ParsePosition(0);
    Date date = formatter.parse(input, pos);
    cal2.setTime(date);
    seconddate = cal2.get(Calendar.DAY_OF_YEAR);
    dateflag = true;    
}
public void AtmMenu()
{
    int input = 0;

    while (input !=5)
    {   

        System.out.println("1.) Withdraw");
        System.out.println("2.) Deposit");
        System.out.println("3.) Check Balance");
        System.out.println("4.) Calculate Interst");
        System.out.println("5.) Exit");
        System.out.print("Please enter a nubmer from the menu above" 
                + " and press enter:" );
        input = sc.nextInt();
        System.out.println();

        if (input == 1)
        {
           withdraw((int) input);                     
        }
        else if (input == 2)
        {
            deposit(input);     
        }
        else if (input == 3)
        {
            newBalance();    
        }
        else if (input == 4)
        {
            calcInterest();             
        }
        else if (input <=0 || input >=6)
        {
            System.out.println("Please enter a nubmer from the Menu");
        } 
    }
}  
}
\$\endgroup\$
1
\$\begingroup\$

Package names should be lower case and reverse domain name

Your package name is ATM. That violates the Java Code Conventions. It is recommended that package names are all lowercase or at least lowerCamelCase. And they should be the reverse domain name. So it should be atm or better com.yourdomain.atm or something like that.

Avoid star imports

Most Java developers that I know find it better to avoid star imports and let the IDE manage explicit imports. Explicit imports give a better overview over the fan-out (outgoing dependencies) of your class than star imports. Also, explicit imports avoid the problem that with star imports, class names can be ambiguous of two imported packages contain classes with the same name.

Serializable classes should provide serialVersionUID

If a class is Serializable, it should provide a field private static final long serialVersionUID which acts as a version number for the serialized object of that class. This field can then be used to ensure data compatibility between serialized objects of different versions of that class.

Use K&R / 1TBS / SUN brace style

It is usual for Java programmers to put the open curly brace at the end of the line, not the start of the next line.

Fields should be private

All your fields have package default visibility. In languages which support scoping and data hiding, it is best practice to reduce scope as much as possible and hide data as much as possible. In Java this means that it is common practice to make fields private per default.

Sort static first

It is common to sort static fields before non-static fields. Many code conventions even demand this. And it makes sense: static fields exist as soon as the class is loaded. Non-static fields only exist when an object of this class has been constructed.

Make constant fields final

When a field value is never changed, the field should be declared final. In your case this is at least the fields df and fmt.

Make constant fields static

If a field value is never changed and all objects would use the same value, the field can be not just final but static final. In your case, that is field df.

Use good names that reveal intent

Use camel case

  • firstdate -> firstDate
  • seconddate -> secondDate
  • dateflag -> dateFlag (on these names more below, especially on dateflag)

Methods getDate1() and getDate2() do not get dates, they read dates

In languages like Java, get means asking an object for information. Methods starting with get are expected to return values and have no side effects. Method getAcctNum() is an example for such a method.

However, getDate1() and getDate2() do not get dates, they do not return values - and they even have side effects, they change fields. The methods should be renamed readDate1() and readDate2(). Even better, readFirstdate() and readSeconddate().

Use longer names

The names sc, df and fmt can be difficult to understand. In order to understand the fields, the context needs to be known. For example, does df stand for DateFormat or DateFormatter or DecimalFormat or DefaultFormatter? It is better to use longer names that reveal the intent of the field. Examples:

  • instead of sc you could use stdinScanner
  • instead of df you could use decimalFormat
  • instead of fmt you could use currencyFormat

What do first and second even mean?

It's not clear what firstdate and seconddate mean until all the code using them is understood. How about using the following names: interestStartDate and interestEndDate.

It should be noted, however, that interestStartDate and interestEndDate have a few other problems. These variables are not Dates but the day of the year.

So they should actually be named interestStartDayOfYear and interestEndDayOfYear. And if you would have named them like this, you might have, just by the name, already discovered a flaw in the program that was otherwise more difficult to spot:

Bug: Interest calculation only operates correctly if the start and end are in the same year.

The code

    getDate1();
    getDate2();
    if (firstdate > seconddate) {
        System.out.println("You must enter a future date:");
        getDate2();
    }

has a few flaws, but first things first. While the code seems to compare dates, it actually compares day in years. That is difficult to spot for a number of reasons, the one relevant in this section is the following:

firstdate and seconddate are wrongly named: While they are named as if they hold dates, they actually hold the day in the year of their respective year. That's something entirely different.

Replace global variables (fields) with local variables as much as possible

As already mentioned, it's better to keep scope as low as possible.

This goes very well with the next one:

Avoid code that has side effects

Reduce side effects, that is assignments of new values to fields, to the absolute necessary minimum. In the class Account this is only one variable: newBalance.

For example, firstdate and lastdate are actually only really needed in calcInterest(). If you change the two methods that read and assign firstdate and lastdate to instead return the value, and move the assignment into calcInterest(), firstdate and lastdate can become local variables in calcInterest().

Remove unused code

Your constructor Account() (without arguments) is not used. You should delete it.

Remove unnecessary this qualifiers

this.getNewBalance() and getNewBalance() are the same thing. It is unusual in Java to use this for anything else than resolving ambiguity, shadowing and constructor calls.

Avoid hard-coded line separators

You use hard-coded line separators like "\n". Unless you are sure you want to use them (which you're probably not), you should avoid them.

Instead of

    System.out.println("Your current balance is :" +
            fmt.format(this.getNewBalance()) + "\n");

You could use

    System.out.format("Your current balance is: %s%n", fmt.format(getNewBalance());

Do not use double to represent currencies

Currencies are expected to not be rounded. However, double rounds. Although the mathematical capabilities of double seem impressive, the precision of double is insufficient for financial calculations. It is better to use long and store pennies instead of dollars, and use double only temporarily for interest calculations. You could also use BigDecimal instead.

Your program crashes on EOF

When System.in reaches EOF (end of file, can be simulated by a Unix terminal with CTRL+D, by cmd.exe with CTRL+Z), your program crashes with an exception. That is because you're using the Scanner wrongly. You are asking the Scanner for the next element without checking if such an element would actually be available. Call hasNext() first.

Wrong interest calculation input

If the user enters the following first date: 01/20/2016 And the user enters the following last date: 01/19/2017 The user is presented with the message, "You must enter a future date". Not only is this wrong, as already discussed above. But there's also another flaw in the code. If the user enters 01/19/2017 again, the program continues without checking again whether the input is valid.

Consider using the Java 8 Datetime API

The classes Date and Calendar can be regarded deprecated. It's better to use the Java 8 Datetime API around Clock, LocalDate and so on.

Use types which are as generic as possible.

For example, in your I/O code, when opening the file, that is, in the constructor call, you need to know that it's a file. However, during the course of using that stream, this information becomes irrelevant. Your code will be more generic and reusable if you use the most abstract type available that still does the job.

So, instead of

FileOutputStream outStream = new FileOutputStream(".../BankAccounts.txt");

you should use

OutputStream outStream = new FileOutputStream(".../BankAccounts.txt");

Wrong I/O code for reading / writing accounts

The I/O code for reading / writing accounts does not close the file in case there was an exception.

The correct I/O code in case of saveAccount() would look like this:

public void saveAccount() {
    try (
            OutputStream outStream = new FileOutputStream(".../BankAccounts.txt");
            ObjectOutputStream os = new ObjectOutputStream(outStream)
    ) {
        os.writeObject(myAccounts);
    } catch (IOException ioe) {
        System.err.println(ioe);
    }
}

That is the Java 7 "try-with-resources" syntax. All autoclosable resources which are allocated in the () part of the try-block are automatically closed at the end of the try block, no matter whether or not an exception occurred. Also, exceptions that happen during closing will be handled properly as suppressed exceptions. Using flush() for I/O code is only necessary if there is a risk of an asynchronous code, or in situations like buffered multi-plexed I/O (like a Socket). In your case you can safely rely on close() calling flush() implicitly.

Serialized files are not text-files

It may therefore be misleading to use .txt as the file suffix for storing and retrieving the account data. The typical file suffix for Java serialized files is .ser.

Use simple relative file paths

That makes it easier for people who review your code to actually try and play with your code without having too many changes first. People who use Linux, Solaris or Mac OS don't even have an E: drive. The path would never work for them.

Class Account is doing too many things

You could easily split this into multiple classes:

  • A class Account which really is just an account, nothing else.
  • A class AccountUI which takes care of the user interface for managing accounts.
  • A class InterestCalculator which calculates interest rates.
  • A class InterestCalculatorUI which takes care of the user interface for calculating interest rates.

Design

This continues and deepens the previous point. It is common to separate the UI and the business logic from each other. There are various reasons for that, to name a few:

  • It is easier to test business logic when it is decoupled from the UI.
  • A separate UI makes it simple to replace the UI with a different UI, no changes to the business logic needed. For example, you might want to replace the interactive command line UI with a Swing or JavaFX UI in future.

So, your classes Account and InterestCalculator should not have any UI code, that is, in your case, no code related to System.in, System.out, System.err,Scanner, anyFormatters` and so on.

Learn test frameworks, write tests

The most popular test frameworks for Java probably are JUnit, Cucumber and TestNG. Your application would be a very nice case for testing with Cucumber.

\$\endgroup\$
  • \$\begingroup\$ First, wow. I do appreciate you taking so much time to look over this a give me some great feedback. I feel like you just taught me way more about coding from this, than an entire semester in class. I'm going through my code and trying all the critiques you made. I truly wasn't excepting something so elaborated from anybody so again thank you for all the time you spent on this. \$\endgroup\$ – Jeremiah Mar 17 '17 at 5:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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