Further reading:Further reading:
Only submit compilable code to review
There is an type-issue in your ATMTest.
The printMiniStatement-Method in Iterator<String> itr = savings.printMiniStatement();
has return type void, but the variable is expecting an Iterator.
Hence, the code can't be compiled.
Logic & Design
You should introduce some relation between User and Account since they are obviously connected. For example:
- A User may have several Accounts: You can add a field
List<Account> accountstoUser - An Account has exactly one User: You can add a field
User usertoAccount
You should add an ATM-Class which is responsible for doing the withdrawal.
This should not be done by the Account itself. Take a look at the Single-Responsibility-Principle
for more information(link below). You should also add a field String id to Account.
It can be used in combination with the accountPin-field
to allow an ATM-Instance to conduct some authentication.
As Gilbert pointed out: Don't use floating point when representing money. Floating point representation isn't 100% accurate so you will lose/add a few cents at times. Use an integer/long instead. They don't suffer from precision errors and won't cause these types of bugs that are hard to pin down(link below).
And why does the Account-Class only have a no argument constructor that assigns arbitrary values?
Further reading:
- Single-Responsibility-Principle: https://stackify.com/solid-design-principles/
- Floating point precision error: https://www.simplexacode.ch/en/blog/2018/07/using-bigdecimal-as-an-accurate-replacement-for-floating-point-numbers/
toString-Method
You can override a class' toString-Method so System.out.println(...) can infer a string representation of its instances.
Take the following scenario: Creating a new User and immediately printing out
the assigned values. You did just that in your ATMTest-class by repeatedly calling different getters.
By overriding the User's "toString"-Method, this can be abbreviated to: System.out.println(new User(...))
Further reading:
this-keyword
Only use this when you have to avoid shadowing/ambiguity.
Otherwise, it makes your code harder to read.
Further reading:
- When to use
this: https://docs.oracle.com/javase/tutorial/java/javaOO/thiskey.html - Tutorial on shadowing: https://javabeginnerstutorial.com/core-java-tutorial/variable-shadowing/
Whitespace
Use whitespaces to your advantage. Visual gaps created by whitespace are a necessary guide when reading someone's code. They provide additional structure and hence increase readability.
Further reading:
Wildcard-Import
Statements like import templates.*; import all the classes in the templates-package.
Avoid wildcard-imports as they may lead to cumbersome errors when compiling due to naming conflicts.
Further reading:
- https://stackoverflow.com/questions/147454/why-is-using-a-wild-card-with-a-java-import-statement-bad
I only added a few comments to your code marking spots where the above points can be implemented. It's running just the way it used to!
Account Class
package templates;
import java.util.ArrayList;
import java.util.Iterator;
import java.util.List;
public class Account {
/*
"accountBalance" can be renamed to "balance" since "accountBalance" always belongs to an instance of "Account".
Preceding "Balance" by "account" doesn't add any information
*/
private double accountBalance;
private List<String> miniStatement;
/*
"accountStatus" is an extremly broad term and may lead to confusion. If your goal was to mark an account as closed
consider renaming "accountStatus" to "isClosed"
*/
private boolean accountStatus;
private String accountPIN;
public Account() {
/*
the "this"-keyword is only required if you need to avoid ambiguity (this applies to most of your getters/setters as well!!)
take a look at your "User"-Constructor for more information (I left another comment over there :) )
*/
this.accountBalance = 10000.0;
this.accountStatus = true;
this.accountPIN = "1111";
this.miniStatement = new ArrayList<>();
}
public String doWithdraw(double amount) {
// Consistency! use either "this.accountStatus" or "checkAccountStatus()"
if(this.accountStatus) {
if(getBalance() > 0.00 && amount > 0.00 && getBalance() >= amount) {
this.accountBalance -= amount;
// use meaningful names e.g. "substatement"
String str = "Account debited with $"+amount;
this.miniStatement.add(str);
return str;
}else
return "Amount entered is high, please enter less amount !!! ";
}else
return "Account is already closed";
}
public double getBalance() {
if(checkAccountStatus())
return this.accountBalance;
// why should the balance of an disabled account be -1??
return -1.00;
}
// replace "changePIN" by "setPIN". The terms getter/setter are commonly in OOP. They make your code more readable to other programmers
public boolean changePIN(String newPin) {
if(checkAccountStatus()) {
this.accountPIN = newPin;
return true;
}
return false;
}
// replace "checkAccountStatus" by "getAccountStatus"
private boolean checkAccountStatus() {
return this.accountStatus;
}
public String closeAccount() {
if(checkAccountStatus()) {
System.out.printf("%s","Please collect $"+ getBalance());
this.accountStatus = false;
return ". Account is now closed";
}
return "Account is already closed";
}
public void printMiniStatement() {
if(checkAccountStatus()) {
Iterator<String> itr = this.miniStatement.iterator();
System.out.printf("%20s%n", "MINI STATEMENT");
while(itr.hasNext()) {
System.out.printf("%s%n", itr.next());
}
}
}
}
User Class
package templates;
public class User {
private String firstName;
private String lastName;
private String address;
private String phoneNumber;
/*
Here, the "this"-keyword was necessary. Otherwise method parameters will shadow the field you want to assign a value to.
*/
public User(String firstName,String lastName,String address,String phoneNumber) {
this.firstName = firstName;
this.lastName = lastName;
this.address = address;
this.phoneNumber = phoneNumber;
}
/* why abbreviate "firstName" to "fname"? Simply use "firstName" as your method parameter.
This way you can avoid unnecessary confusion for other programmers. Keep the "this"-keyword to avoid ambiguity
*/
public boolean setFirstName(String fname) {
if(!fname.isEmpty()) {
this.firstName = fname;
return true;
}
return false;
}
public String getFirstName() {
return this.firstName;
}
public boolean setLastName(String lname) {
if(!lname.isEmpty()) {
this.lastName = lname;
return true;
}
return false;
}
public String getLastName() {
return this.lastName;
}
public boolean setAddress(String addr) {
if(!addr.isEmpty()) {
this.address = addr;
return true;
}
return false;
}
public String getAddress() {
return this.address;
}
public boolean setPhoneNumer(String phonenum) {
if(phonenum.length() == 10) {
this.phoneNumber = phonenum;
return true;
}
return false;
}
public String getPhoneNumber() {
return this.phoneNumber;
}
}
ATMTest Class
package test;
import java.util.Iterator;
// avoid wildcard imports
import templates.*;
public class ATMTest {
public static void main(String[] args) {
Account savings = new Account();
User user1 = new User("Dummy","Name","27, First Floor, Suok-I","8888888888");
System.out.printf("%s%n", "Account Balance is $"+savings.getBalance());
System.out.println("-------------------------------------");
System.out.printf("%s%n", savings.doWithdraw(50));
System.out.println("-------------------------------------");
System.out.printf("%s%n", savings.doWithdraw(5540));
System.out.println("-------------------------------------");
System.out.printf("%s%n", savings.doWithdraw(3350));
System.out.println("-------------------------------------");
System.out.printf("%s%n", savings.doWithdraw(1090));
System.out.println("-------------------------------------");
System.out.printf("%s%n", savings.doWithdraw(90));
System.out.println("-------------------------------------");
System.out.printf("%s%n", savings.doWithdraw(966));
System.out.println("-------------------------------------");
System.out.printf("%s%n", "Account Balance is $"+savings.getBalance());
System.out.println("-------------------------------------");
savings.changePIN("1611");
/*
// the following line can't be compiled -> incompatible types
Iterator<String> itr = savings.printMiniStatement();
System.out.printf("%20s%n", "MINI STATEMENT");
while(itr.hasNext()) {
System.out.printf("%s%n", itr.next());
}
*/
System.out.println("-------------------------------------");
// use the "toString"-Method here to print "user1"
System.out.printf("%s%n", user1.getFirstName());
System.out.printf("%s%n", user1.getLastName());
System.out.printf("%s%n", user1.getAddress());
System.out.printf("%s%n", user1.getPhoneNumber());
System.out.println("-------------------------------------");
user1.setFirstName("Rain");
user1.setLastName("Man");
user1.setAddress("32, Second Floor, Suok-I");
user1.setPhoneNumer("9999999999");
System.out.printf("%s%n", user1.getFirstName());
System.out.printf("%s%n", user1.getLastName());
System.out.printf("%s%n", user1.getAddress());
System.out.printf("%s%n", user1.getPhoneNumber());
System.out.println("-------------------------------------");
System.out.printf("%s%n", savings.closeAccount());
System.out.println("-------------------------------------");
System.out.printf("%s%n", savings.doWithdraw(1111));
System.out.println("-------------------------------------");
}
}
```
lang-java