Trying to create OO based bank app

[Campbell Ritchie]

I suspect you will find passing an Amount to the Account#withdraw method might be the mos OO approach.
You might do well to have canWithdraw methods.

public boolean canWithdraw(Amount am)
{
   return this.amount.compareTo(am) >= 0;
}

Obviously your version will be slightly different.

[Tushar Goel]

Yeah account class all sort of that methods: deposit , withdraw , interest etc....

Single Responsibility: a class takes care of itself and does its own actions.

In this case option 3 is valid but i will think again..

You might do well to have canWithdraw methods...... Obviously your version will be slightly different.

Yes, i am also thinking the same..

[Tushar Goel]

My Account class:

fields: account id, account status, balance, penalty amount, interest rate, minimum balance to maintain, overdraft amount
behavior: isAccountActive, isWithdrawPossble, isMinimumBalanceMaintained, deposit, withdraw, penalty , makeAccountInActive, interest, closeAccount, storemaximumUsedAccountIdIntoFile


Transaction:
field: amount
behavior: getAmount


I also reading about SOLID again to get more idea...

[Campbell Ritchie]

The names of those methods should match the bean pattern. It should be isActive and setActive, etc etc.

[Campbell Ritchie]

Are you sure the storeMaximumID method belongs to the account class? Might it be a function of the bank class?

And, good idea to find out about SOLID but be aware that many people no longer believe the O bit.

[Tushar Goel]

Are you sure the storeMaximumID method belongs to the account class? Might it be a function of the bank class?

No sorry, i have mentioned wrongly as it is not write operation but read maximum current account id from file and write is definitely part of Bank class as we did in Customer class...

And, good idea to find out about SOLID but be aware that many people no longer believe the O bit.

Yes thanks again..

[Tushar Goel]

Sorry, i took long time to understand SOLID but i am feeling better now(for SOLID)

I going to redesign my Transaction and account class now. I am making account class abstract Defining several interfaces in account class so that as per account type
characteristics particular interfaces can be implemented.

Transaction

 public interface Transaction {

	/**
	 * This will return the amount receive from/to the customer.
	 * @return the amount
	 */
	public BigDecimal getAmount();
}

public class TransactionClass implements Transaction {

	private final BigDecimal amount;
		
	/**
	 * Initialize value
	 * @param amount
	 */
	public TransactionClass(BigDecimal amount) {
		this.amount = amount;
	}

	/**
	 * This will return the amount receive from the customer.
	 * @return the amount
	 */
	@Override
	public final BigDecimal getAmount() {
		return amount;
        }
}

Account Interfaces To follow Interface segregation principle

Interest

public interface Interest {
	
	/**
	 * This method will set the interest rate set by the bank.
	 * @return  interest rate set by the bank
	 */
	void setInterestRate();
	
	/**
	 * This will calculate the interest earn on the account as per
	 * bank policy. There is no interest on Inactive or closed accounts.
	 * 
	 * @return    interest amount calculated as per bank norms
	 */
	BigDecimal calculateInterest();
}

Penalty

public interface Penalty {

    /**
     * This will set the penalty on the account as bank norms
     */
	void setPenalty();

	/**
	 * This will calculate the penalty on the account as per bank norms.
	 * @return
	 */
	BigDecimal calculatePenalty();
} 

Minimum balance

public interface MinimumBalance {

	/**
	 * This method will set the minimum balance required to keep
	 * account active. Minimum balance is set as per bank norms.
	 */
	void setMinimumBalance();
}

maximum overdraft

public interface MaximumOverdraft {

	/**
	 * This will set the maximum overdraft allwed to a account
	 * as per bank norms.
	 */
	void setMaximumOverdraft();
} 

Account class Open closed principle is broken here

package onlinebanking;

public abstract class Account {

	private final int accountNumber;
	private BigDecimal balance ;
	private AccountStatus accountStatus;

	private BigDecimal penalty = setPenaltyAmount();
	private BigDecimal minimumBalance = setMiniumBalanceAmount();
	private BigDecimal interestRate = setInterestRateForAccounts();
	private BigDecimal maximumOverdraft = setMaximumOverdraftAmount();
	private static final int ACCOUNT_NUMBER_STARTS_FROM = getLastUsedAccountNumber();

	private static final AtomicInteger  ACCOUNT_NUMBER_GENERATOR = 
					new AtomicInteger(ACCOUNT_NUMBER_STARTS_FROM);
	

	/**
	 * Initialize value
	 * @param balance     amount entered by user while opening an account
	 * @throws IllegalArgumentException   When amount <= 0
	 * @throws NullPointerException       when amount is null
	 */
	public Account(BigDecimal balance)
			           throws IllegalArgumentException, NullPointerException{
		
        if (balance == null) {
        	throw new NullPointerException("amount can not be null");
        } else if (balance.compareTo(BigDecimal.ZERO) <= 0) {
        	throw new IllegalArgumentException("amount can not be 0 or less than 0");
        }
        this.balance = balance;
		this.accountStatus = AccountStatus.OPEN;
		this.accountNumber = ACCOUNT_NUMBER_GENERATOR.getAndIncrement();
	}
	

	/**
	 * @return
	 */
	protected BigDecimal setMaximumOverdraftAmount() {
		 return BigDecimal.ZERO;
	}


	/**
	 * @return
	 */
	protected BigDecimal setInterestRateForAccounts() {
		 return BigDecimal.ZERO;
	}


	/**
	 * @return
	 */
	protected BigDecimal setMiniumBalanceAmount() {
		 return BigDecimal.ZERO;
	}


	/**
	 * @return
	 */
	protected BigDecimal setPenalityAmount() {
		return BigDecimal.ZERO;
	}


	/**
	 * Read last used account number from the file.
	 * @return
	 */
	private static int getLastUsedAccountNumber() {		
		return DataReadWrite.readFirstIntNumberFromFile("AccountId.txt");
	}

	/**
	 * This method will add the amount entered by the user into the balance
	 *  	
	 * @param amount   amount entered by the user
	 * @throws IllegalArgumentException  When amount entered is <= 0
	 */
	public void setDeposit(Transaction transaction) throws IllegalArgumentException {
	     // will define code here
   	}
	
	/**
	 * Check whether account is still Open (Active/Inactive) or closed
	 * @return  accounrStatus Return true if account status is not closed else false
	 */
	public final boolean isAccountOpen() {
		return (accountStatus != AccountStatus.CLOSE) ? true : false;
	}


	/**
	 * Check whether amount entered by the user is less than or equals to 0.
	 * @param t;ranscation    transaction done by the user
	 * @return  boolean       true if amount  is <= 0 else false
	 */
	protected final boolean isAmountLessThanOrEqualsToZero(Transaction transaction) {
		return isAmountLessThanOrEqualsToZero(transaction.getAmount()) ;
	}

	/**
	 * Check whether amount entered by the user is less than or equals to 0.
	 * @param   amount        
	 * @return  boolean       true if amount  is <= 0 else false
	 */
	protected final boolean isAmountLessThanOrEqualsToZero(BigDecimal amount) {
		return amount.compareTo(BigDecimal.ZERO) <= 0 ;
	}


	/**
	 * This method allow to withdraw up to account balance  + MAXIMUM_OVERDRAFT_LIMIT.
	 * Penalty of amount PENALTY charged on the customer account and deducted from the
	 * user account.
	 * 
	 * @param account                     Customer account number
	 * @param amountToBeWithdraw          amount user wants to withdraw
	 * @throws IllegalArgumentException   When withdraw amount is <= 0 or
	 * 									  greater than (available balance + MAXIMUM_OVERDRAFT_LIMIT)									
	 */
	public void doWithdraw(Transaction transaction) 
			throws InsufficientFundException, IllegalArgumentException {

	   // will define code here	
	}

	/**
	 * Customer can close its account any time, all available balance 
	 * will be refunded to the customer. Although customer cann't close
	 * account if balance is negative(which may happen due to some penalty
	 * or maintenance charges on the account and balance is not sufficient).
	 * If account is already closed then customer cann't close it again.
	 * 
	 * @param account                Customer account number  
	 * @throws IllegalStateException When balance is negative or trying to
	 * 								 close already closed account
	 */
	 protected final void closeAccount(Account account) throws IllegalStateException{
                   //will define code here
         }
    
	
	/**
	 * This will change the bank account status from active to inactive when sufficient
	 * amount is not available in user account or inactive to active when user deposit
	 * sufficient amount in his or her account. 
	 * 
	 * @param isActive   to change bank account status. true to make active
	 * 					 false to make account inactive.
	 */	
	protected void makeAccountActive(boolean isActive) {
		
	     // will define code here
	}  
} 

[Campbell Ritchie]

Looks good

But how are you calculating interest on an account?
Why do your setXXX methods return a BigDecimal?
Do you have any constraints about the values of those BigDecimals? Why are you not doing any validation in the transaction constructor?

[Tushar Goel]

Why do your setXXX methods return a BigDecimal?

Sorry, wrong one. I have corrected it. Instead now, i think let the default value passed in my abstract class account and provide setter methods. So any specific
account which implements specific interfaces can update the value to this variables as per need.

Do you have any constraints about the values of those BigDecimals?

Yes, amount should be greater than 0

Why are you not doing any validation in the transaction constructor?

Corrected

But how are you calculating interest on an account?

I thought interest calculate per day basis and credited it to customer account quarterly basis. Any account which provides interest must implement Interest interface which set interest rate.
Each subclass also have method addDeposit() or recoverPenalty() which add interest rate or subtract penalty from the user account.


Transaction class:

public class TransactionClass implements Transaction {
	private final BigDecimal amount;

	/**
	 * Initialize value
	 * @param amount    
	 * @throws IllegalArgumentException  when amount <= 0
	 * @throws NullPointerException      when amount is null
	 */
	public TransactionClass(BigDecimal amount) 
			throws IllegalArgumentException, NullPointerException{
		
        if (amount == null) {
        	throw new NullPointerException("amount can not be null");
        } else if (amount.compareTo(BigDecimal.ZERO) <= 0) {
        	throw new IllegalArgumentException("amount can not be 0 or less than 0");
        } 
	this.amount = amount;
	}

	/**
	 * This will return the amount receive from the customer.
	 * @return the amount
	 */
	@Override
	public final BigDecimal getAmount() {
		return amount;
	}
} 

[Tushar Goel]

I having a one doubt.. If for any account i set interest rate or penalty or other thing then that changes will be reflect in all the accounts and this is not desired.
I tried to think the solution but could not able to solve it.What i am thinking to make all the methods abstract and any class which implements these methods needs define it.
Or to override some particular methods like withdraw.

[Campbell Ritchie]

You cannot make a method abstract and then have subclasses requiring it implement it. Otherwise classes not requiring that method would have to be abstract.
What you can do is have methods in the superclass like this and override them. In Java8 you can even have such methods as default implementations in an interface:-

/**
 * blahblablah
 * @return somethingsomethingsomething
 * @throws rhubarbrhubarbrhubarb
 */
public BigDecimal getInterestToDate(...)
{
   return BigDecimal.ZERO;
}

You should decide whether you have types of account all with the same interest rate. That is common, actually. In that case you would consider having interestRate as a static field.
Beware: You probably need a separate static field for each class, because of hiding.

[Campbell Ritchie]

Transaction looking good, but I have realised there is something I forgot. Sorry. A Transaction may have an amount and a transaction Date or Time.

[Campbell Ritchie]

Tushar Goel wrote: . . .

Transaction class:

public class TransactionClass implements Transaction {
	 . . .

	/**
	 * This will return the amount receive from the customer.
	 * @return the amount
	 */
    . . .
} 

If the transaction is a withdrawal, that will represent the amount given to the customer. Just say, “amount of this transaction.”
Is that class intended to be final? If not, I suspect it should be abstract with classes like Deposit and Withdrawal as subclasses.

[Tushar Goel]

sorry for late response, i was away due to personal issues..

What you can do is have methods in the superclass like this and override them

Thanks...

with classes like Deposit and Withdrawal as subclasses.

Just wondering why we need to extend Transaction class and have deposit and withdraw as subclass.. I thought they are methods of the Account class...
Is it because of single responsibility principle ?

[Campbell Ritchie]

That way the Account can maintain a List<Transaction>. There would be other ways to do it.

[Tushar Goel]

That way the Account can maintain a List<Transaction>. There would be other ways to do it.

Thanks.. I was unable to think this.. I need more practice...

As suggested i have made changes:

TransactionType

public enum TransactionType {
	DEPOSIT , WITHDRAW
}

TransactionClass

public abstract class TransactionClass implements Transaction {

	private final BigDecimal amount;
	private final String message;
	private final LocalTime localTime ;
	private final TransactionType transactionType;
	
	/**
	 * Initialize value
	 * @param amount    
	 * @throws IllegalArgumentException  when amount <= 0
	 * @throws NullPointerException      when amount is null
	 */
	public TransactionClass(BigDecimal amount, TransactionType transactionType) 
			throws IllegalArgumentException, NullPointerException{
		
        if (amount == null) {
        	throw new NullPointerException("amount can not be null");
        } else if (amount.compareTo(BigDecimal.ZERO) <= 0) {
        	throw new IllegalArgumentException("amount can not be 0 or less than 0");
        } 
        
		message = "You have made " + transactionType.name() + " of " +  amount;
		this.amount = amount;
		this.transactionType = transactionType;
		localTime = LocalTime.now();
	}

	/**
	 * This will return the amount receive from the customer.
	 * @return the amount
	 */
	@Override
	public BigDecimal getAmount() {
		return amount;
	}

	/**
	 * @return the message
	 */
	String getMessage() {
		return message;
	}
}

Deposit

public class Deposit extends TransactionClass {
	/**
	 * Initialize value
	 * @param amount    
	 * @throws IllegalArgumentException  when amount <= 0
	 * @throws NullPointerException      when amount is null
	 */
	public Deposit(BigDecimal amount)
			throws IllegalArgumentException, NullPointerException {
		super(amount, TransactionType.DEPOSIT);
	}
}

Withdraw

public class Withdraw extends TransactionClass {

	/**
	 * Initialize value
	 * @param amount    
	 * @throws IllegalArgumentException  when amount <= 0
	 * @throws NullPointerException      when amount is null
	 */
	public Withdraw(BigDecimal amount)
			throws IllegalArgumentException, NullPointerException {
		super(amount, TransactionType.WITHDRAW);
	}
}

[Campbell Ritchie]

Sounds good. Now you can have takeTransaction methods in Account, or you can have withdraw and deposit methods.
What about transactions for charges or interest?

[Tushar Goel]

What about transactions for charges or interest?

Yes, they deserve separate class as well but i think charges due to not maintaining minimum balance and interest calculation done on quarterly basis so i think
this will be done some scheduled thread and can not calculate in current application. For charges due to overdraft can only be implement as of now.

I also made some changes, i thought instead of having Transaction interface let only abstract Transaction class there and other classes will extends it. Also i think i should
add TransactionId as well for uniquely identify each transaction object by overriding equals and hashcode method.

Charges

 public class Charges extends Transaction {

	/**
	 * Initialize value
	 * @param amount    
	 * @throws IllegalArgumentException  when amount <= 0
	 * @throws NullPointerException      when amount is null
	 */
	public Charges(BigDecimal amount)
			throws IllegalArgumentException, NullPointerException {
		super(amount, TransactionType.PENALTY);
	}
}

Account class This is my transaction class.. I think there is one 1 thing left here. saving the list of transaction in file. I think i need to implement Serializable interface to the Transaction class. I will
do it once rest piece of code is fine

public abstract class Account {

	private final int accountNumber;
	private BigDecimal balance ;
	private AccountStatus accountStatus;
	
	private BigDecimal penalty  = BigDecimal.ZERO;
	private BigDecimal minimumBalance = BigDecimal.ZERO;
	private BigDecimal interestRate = BigDecimal.ZERO;
	private BigDecimal maximumOverdraftAmount = BigDecimal.ZERO; 
	private static final int ACCOUNT_NUMBER_STARTS_FROM = getLastUsedAccountNumber();
	
	private final List<Transaction> storeTransaction = new ArrayList<>();
	
	private static final AtomicInteger  ACCOUNT_NUMBER_GENERATOR = 
										new AtomicInteger(ACCOUNT_NUMBER_STARTS_FROM);


	/**
	 * Initialize value
	 * @param balance     amount entered by user while opening an account
	 * @throws IllegalArgumentException   When amount <= 0
	 * @throws NullPointerException       when amount is null
	 */
	public Account(BigDecimal balance)
			           throws IllegalArgumentException, NullPointerException{
		
        if (balance == null) {
        	throw new NullPointerException("amount can not be null");
        } else if (balance.compareTo(BigDecimal.ZERO) <= 0) {
        	throw new IllegalArgumentException("amount can not be 0 or less than 0");
        }
        this.balance = balance;
		this.accountStatus = AccountStatus.OPEN;
		this.accountNumber = ACCOUNT_NUMBER_GENERATOR.getAndIncrement();
	}
	

	/**
	 * Read last used account number from the file.
	 * @return
	 */
	private static int getLastUsedAccountNumber() {		
		return DataReadWrite.readFirstIntNumberFromFile("AccountId.txt");
	}

	/**
	 * This method will add the amount entered by the user into the balance.
	 * @param amount   amount entered by the user
	 */
	void setDeposit(Transaction transaction)  {
		this.balance = this.balance.add(transaction.getAmount()) ;		
                storeTransaction(transaction);
   	}

	/**
	 * This allow to withdraw up to account balance  + MAXIMUM_OVERDRAFT_LIMIT.
	 * @param account                     Customer account number
	 * @param amountToBeWithdraw          amount user wants to withdraw
	 * @throws InsufficientFundException  When withdraw amount greater than 
	 *									  (available balance + MAXIMUM_OVERDRAFT_LIMIT)									
	 */
	void doWithdraw(Transaction transaction) throws InsufficientFundException {

		BigDecimal amount = balance.add(maximumOverdraftAmount);
		
		if (compareAmount(balance, transaction.getAmount())) {
			balance = balance.subtract(transaction.getAmount());
		} else if (compareAmount(amount, balance)) {
			balance = balance.subtract(amount);
			calculatePenalty();
		} else {
			throw new InsufficientFundException("Balance is insufficient for withdraw");
		}

            storeTransaction(transaction);
	}

	/**
	 * This will calculate the penalty amount of an account in case of
	 * overdraft or in case of not maintaining minimum balance.
	 */
	void calculatePenalty() {
		
		Transaction transaction = takeChargesTransaction();
		
		if (transaction != null) {
		   balance = balance.subtract(transaction.getAmount());
		   storeTransaction(transaction);
		}
	}


	/**
	 * This will store the transaction so that user can list all
	 * the transaction performed by him or her.
	 * @param transaction   Transaction object 
	 */
	void storeTransaction(Transaction transaction) {
		 storeTransaction.add(transaction);		
	}

 	
	/**
	 * Check whether account is still Open (Active/Inactive) or closed
	 * @return  accounrStatus Return true if account status is not closed else false
	 */
	public final boolean isAccountOpen() {
		return (accountStatus != AccountStatus.CLOSE) ? true : false;
	}

	/**
	 * @param amount      Original amount which have to be compare
	 * @param compareWith Amount which we needs to compare with
	 * @return  true if compare amount > compare with amount or else
	 *          false will be return
	 */
	boolean compareAmount(BigDecimal amount, BigDecimal compareWith)  {
		return (amount.compareTo(compareWith) >= 0) ? true : false ;
	}
	

	/**
	 * This will change the bank account status from active to inactive when sufficient
	 * amount is not available in user account or inactive to active when user deposit
	 * sufficient amount in his or her account. 
	 * 
	 * @param isActive   to change bank account status. true to make active
	 * 					 false to make account inactive.
	 */	
	public void makeAccountActive(boolean isActive) {
		
		this.accountStatus =  (isActive == true) 
						       ? AccountStatus.OPEN 
						       : AccountStatus.INACTIVE;
	}

	/**
	 * Customer can close its account any time, all available balance 
	 * will be refunded to the customer. Although customer cann't close
	 * account if balance is negative(which may happen due to some penalty
	 * or maintenance charges on the account and balance is not sufficient).
	 * If account is already closed then customer cann't close it again.
	 * @throws IllegalStateException When balance is negative
	 */
	 void closeAccount() throws IllegalStateException {
		 
           if (balance.compareTo(BigDecimal.ZERO) < 0) {
        	   throw new IllegalStateException("Please first pay all pending charges");
           }
           
           Transaction transaction = takeWithdrawTransaction(balance);  
           storeTransaction.add(transaction);
           accountStatus = AccountStatus.CLOSE;
	 }

       /**
        * This will create a transaction whenever a charges/Penalty is
        * applied to the account.
        * @return  Transaction object
        */
	Transaction takeChargesTransaction() {
		return new Charges(penalty);
	}

	 /**
	  * This will return a transaction object when new withdraw happened.
	  * @param amount
	  * @return
	  */
	Transaction takeWithdrawTransaction(BigDecimal amount) {
		return new Withdraw(amount);
	}

	/**
	 * This will display all the transaction done by the account holder
	 */
	public void showAllTransaction() {
		
		for (Transaction transaction : storeTransaction) {
			
			System.out.println("You have " +  transaction.getTransactionType().name() +
					            " of " + transaction.getAmount() + " on " + 
					            transaction.getLocalTime());
		}
	}

	@Override
	public boolean equals(Object obj) {
		
		return (this == obj) 
				|| obj != null 
				&& (this.getClass().equals(obj.getClass()))
				&& ((Account)obj).accountNumber == accountNumber  ;
	}
	
	@Override
	public int hashCode() {
		return 31 * accountNumber;
	}
     // getter and setter method	
}

[Campbell Ritchie]

Good grief! You didn't really write == true, didyou?
That method should be called setAccountActive(boolean active)
Agree that serialising is a simple way to maintain account details. The banks don't do that; they maintain the details on a database server behind foot‑thick steel doors. Actually multiple servers several miles apart. And they don't let anybody find out where those servers are.

[Tushar Goel]

You didn't really write == true, didyou

Actually, i did.. Sorry i dint realized it... I made correction..

	
	public void setAccountActive(boolean isActive) {
		
		if (isActive) {
		   this.accountStatus =  AccountStatus.OPEN ;
		} else {
			this.accountStatus =  AccountStatus.INACTIVE;
		}
	}

[Campbell Ritchie]

Caught you Actually I preferred the version with the :? that you had earlier.

[Tushar Goel]

Modified it:

	public void setAccountActive(boolean isActive) {
		
		Boolean value = new Boolean(true) ;
		
		this.accountStatus = (value.compareTo(isActive) == 0) 
				                  ? AccountStatus.OPEN 
				                  : AccountStatus.INACTIVE ;
	}

[Campbell Ritchie]

Modified? Worsened, you mean. Apart from the == true, what was wrong with the first version?
Similarly the isAccountOpen method can be simplified greatly.
The names of the methods should correspond: matched setXXX and getXXX methods should have the same XXX

[Campbell Ritchie]

And what will happen to a closed account if you call setActive(false) on it?

[Tushar Goel]

if account is closed then nothing will happen. I will put this check in bank class so that user can not go further and get warning then and their..

That method is in Account class but will call from the bank class

[Campbell Ritchie]

That check should be in the Account class.

[Tushar Goel]

but i thought invalid input should not come to Account class at first place. Is it ok to put double check on it? If that's so i need to put a check in each method because if account is closed then it again request for deposit and withdraw or others...

	void doWithdraw(Transaction transaction) throws InsufficientFundException,IllegalStateException {

		if (!isAccountOpen()) {
			throw new IllegalStateException("Can not do withdraw for closed accounts");
		}
		
		BigDecimal amount = balance.add(maximumOverdraftAmount);
		
		if (compareAmount(balance, transaction.getAmount())) {
			balance = balance.subtract(transaction.getAmount());
		} else if (compareAmount(amount, balance)) {
			balance = balance.subtract(amount);
			calculatePenalty();
		} else {
			throw new InsufficientFundException("Balance is insufficient for withdraw");
		}
		
		storeTransaction(transaction);
	}

[Tushar Goel]

Reposting again, i think due to reediting previous post goes un-noticed.

[Campbell Ritchie]

Sorry for missing it.
Better to check twice than not to check at all. Yes, you may have to throw that Exception from each method. You can write a private method which checks that the account can receive a transaction and throws that Exception.

public void withdraw(WithdrawalTransaction t) throws ...
{
   testAccountAvailable();
   ...
}

Decide whether the testAccountAvailable method throws the Exception from closed accounts or inactive accounts or what.

[Tushar Goel]

Thanks. I have made recommended changes now..

Decide whether the testAccountAvailable method throws the Exception from closed accounts or inactive accounts or what.

It will throws exception for closed accounts.

This is my updated Account class:

public abstract class Account {

	private final int accountNumber;
	private BigDecimal balance ;
	private AccountStatus accountStatus;
	
	private BigDecimal penalty  = getPenaltyAmount();
	private BigDecimal minimumBalance = getMiniumBalance();
	private BigDecimal interestRate = getInterestRate();
	private BigDecimal maximumOverdraftAmount = getMaximumOverdraftAmount();
	
	private static final int ACCOUNT_NUMBER_STARTS_FROM = getLastUsedAccountNumber();
	
	private final List<Transaction> storeTransaction = new ArrayList<>();
	
	private static final AtomicInteger  ACCOUNT_NUMBER_GENERATOR = 
										new AtomicInteger(ACCOUNT_NUMBER_STARTS_FROM);


	/**
	 * Initialize value
	 * @param balance     amount entered by user while opening an account
	 * @throws IllegalArgumentException   When amount <= 0
	 * @throws NullPointerException       when amount is null
	 */
	public Account(BigDecimal balance)
			           throws IllegalArgumentException, NullPointerException{
		
        if (balance == null) {
        	throw new NullPointerException("amount can not be null");
        } else if (balance.compareTo(BigDecimal.ZERO) <= 0) {
        	throw new IllegalArgumentException("amount can not be 0 or less than 0");
        }
        this.balance = balance;
		this.accountStatus = AccountStatus.OPEN;
		this.accountNumber = ACCOUNT_NUMBER_GENERATOR.getAndIncrement();
	}
	

	/**
	 * @return
	 */
	public BigDecimal getInterestRate() {
		return  BigDecimal.ZERO;
	}
	
	/**
	 * @return
	 */
	public BigDecimal getMiniumBalance() {
		return  BigDecimal.ZERO;
	}
	
	/**
	 * @return
	 */
	public BigDecimal getPenaltyAmount() {
		return BigDecimal.ZERO;
	}
	
	/**
	 * 
	 * @return
	 */
	public BigDecimal getMaximumOverdraftAmount() {
		return BigDecimal.ZERO;
	}

	/**
	 * Read last used account number from the file.
	 * @return
	 */
	private static int getLastUsedAccountNumber() {		
		return DataReadWrite.readFirstIntNumberFromFile("AccountNumber.txt");
	}

   
	/**
	 * This method will add the amount entered by the user into the balance.
	 * @param amount   amount entered by the user
	 * @throws IllegalStateException  When trying to do deposit on closed account
	 */
	public void setDeposit(Transaction transaction) throws IllegalStateException {
		
		testAccountAvailable();
		this.balance = this.balance.add(transaction.getAmount()) ;
		storeTransaction(transaction);
   	}

	/**
	 * This allow to withdraw up to account balance  + MAXIMUM_OVERDRAFT_LIMIT.
	 * @param account                     Customer account number
	 * @param amountToBeWithdraw          amount user wants to withdraw
	 * @throws InsufficientFundException  When withdraw amount greater than 
	 *									  (available balance + MAXIMUM_OVERDRAFT_LIMIT)									
     * @throws IllegalStateException      When trying to do withdraw on closed account
     */
	void doWithdraw(Transaction transaction) throws InsufficientFundException,IllegalStateException {

		testAccountAvailable();
		
		BigDecimal amount = balance.add(maximumOverdraftAmount);
		
		if (compareAmount(balance, transaction.getAmount())) {
			balance = balance.subtract(transaction.getAmount());
		} else if (compareAmount(amount, balance)) {
			balance = balance.subtract(amount);
			calculatePenalty();
		} else {
			throw new InsufficientFundException("Balance is insufficient for withdraw");
		}
		
		storeTransaction(transaction);
	}

	/**
	 * This will calculate the penalty amount of an account in case of
	 * overdraft or in case of not maintaining minimum balance.
	 */
	void calculatePenalty() {
		
		Transaction transaction = takeChargesTransaction();
		
		if (transaction != null) {
		   balance = balance.subtract(transaction.getAmount());
		   storeTransaction(transaction);
		}
	}


	/**
	 * This will store the transaction so that user can list all
	 * the transaction performed by him or her.
	 * @param transaction   Transaction object 
	 */
	void storeTransaction(Transaction transaction) {
		 storeTransaction.add(transaction);		
	}

    /**
     * This will create a transaction whenever a charges/Penalty is
     * applied to the account.
     * @return  Transaction object
     */
	Transaction takeChargesTransaction() {
		return new Charges(penalty);
	}
	
	
	/**
	 * Check whether account is still Open (Active/Inactive) or closed
	 * @return  accounrStatus Return true if account status is not closed else false
	 */
	public final boolean isAccountOpen() {
		return (accountStatus != AccountStatus.CLOSE) ? true : false;
	}

	/**
	 * @param amount      Original amount which have to be compare
	 * @param compareWith Amount which we needs to compare with
	 * @return  true if compare amount > compare with amount or else
	 *          false will be return
	 */
	boolean compareAmount(BigDecimal amount, BigDecimal compareWith)  {
		return (amount.compareTo(compareWith) >= 0) ? true : false ;
	}
	

	/**
	 * This will change the bank account status from active to inactive when sufficient
	 * amount is not available in user account or inactive to active when user deposit
	 * sufficient amount in his or her account. 
	 * 
	 * @param isActive   to change bank account status. true to make active
	 * 					 false to make account inactive.
     * @throws IllegalStateException   When trying to change the status of already closed 
     * 								   accounts
     */
	public void setAccountActive(boolean isActive) throws IllegalStateException {
		
		Boolean value = new Boolean(true) ;
		
		this.accountStatus = value.compareTo(isActive) == 0 
				              ? AccountStatus.OPEN 
				              : AccountStatus.INACTIVE ;
	}

	/**
	 * Customer can close its account any time, all available balance 
	 * will be refunded to the customer. Although customer cann't close
	 * account if balance is negative(which may happen due to some penalty
	 * or maintenance charges on the account and balance is not sufficient).
	 * If account is already closed then customer cann't close it again.
	 * @throws IllegalStateException When balance is negative or when account 
	 * 							     is already closed
	 */
	 public void closeAccount() throws IllegalStateException {

		 testAccountAvailable();
		 
           if (balance.compareTo(BigDecimal.ZERO) < 0) {
        	   throw new IllegalStateException("Please first pay all pending charges");
           }
           
           Transaction transaction = takeWithdrawTransaction(balance);  
           storeTransaction.add(transaction);
           accountStatus = AccountStatus.CLOSE;
	 }
	

	 /**
	  * This will return a transaction object when new withdraw happened.
	  * @param amount
	  * @return
	  */
	Transaction takeWithdrawTransaction(BigDecimal amount) {
		return new Withdraw(amount);
	}

	/**
	 * This will display all the transaction done by the account holder
	 */
	public void showAllTransaction() {
		
		for (Transaction transaction : storeTransaction) {
			
			System.out.println("You have " +  transaction.getTransactionType().name() +
					            " of " + transaction.getAmount() + " on " + 
					            transaction.getLocalTime());
		}
	}
	
	/**
	 * Check whether account is opened or not. If account is closed
	 * and some operation on it then it will throw exception
	 * @throws IllegalStateException   When operation are performed on
	 * 								   already closed account
	 */
	private void testAccountAvailable() throws  IllegalStateException{
		if (!isAccountOpen()) {
			throw new IllegalStateException("Can not make already closed account active.");
		}
	}
	
	@Override
	public boolean equals(Object obj) {
		
		return (this == obj) 
				|| obj != null 
				&& (this.getClass().equals(obj.getClass()))
				&& ((Account)obj).accountNumber == accountNumber  ;
	}
	
	@Override
	public int hashCode() {
		return 31 * accountNumber;
	}

	/**
	 * return maximum number of account number used
	 * @return
	 */
	public static int getMaximumAccountNumberUsed() {
		return ACCOUNT_NUMBER_GENERATOR.get();
	}
	

}

[Campbell Ritchie]

Tushar Goel wrote: . . .
It will throws exception for closed accounts.
. . .

But I can't see any Exceptions in the set active method. Why are you using Booleans and compareTo?
I presume you are going to override the mechanism for calculating penalties in subclasses; it looks a very complicated way to get 0. I would prefer to see all fields initialised in the constructor (others will disagree with me), in which case you would want only to call private methods or final methods from the constructor.

[Tushar Goel]

But I can't see any Exceptions in the set active method.

Oops sorry, i forgot to put it. Corrected now.. Thanks for pointing..

public void setAccountActive(boolean isActive) throws IllegalStateException {
		testAccountAvailable();
		Boolean value = new Boolean(true) ;
		this.accountStatus = value.compareTo(isActive) == 0 
				              ? AccountStatus.OPEN 
				              : AccountStatus.INACTIVE ;
	}

Why are you using Booleans and compareTo?

Because i earlier used, ( isActive == true ) and after your pointing i changed it to if-else but that one too not looks good. So i used this. Is this again wrong?

prefer to see all fields initialised in the constructor

You are right. It is much cleaner and eaiser approach then previous one. Is this one you referring to? Also if its ok, i think then i should use builder pattern as number of fields to be initialized are too many.

	public Account(BigDecimal balance, BigDecimal penalty, BigDecimal minimumBalance,
					BigDecimal interestRate,BigDecimal maximumOverdraftAmount) 
			           throws IllegalArgumentException, NullPointerException{
		
        if (balance == null) {
        	throw new NullPointerException("amount can not be null");
        } else if (balance.compareTo(BigDecimal.ZERO) <= 0) {
        	throw new IllegalArgumentException("amount can not be 0 or less than 0");
        }
        this.balance = balance;
        this.penalty = penalty;
        this.minimumBalance = minimumBalance;
        this.interestRate = interestRate;
        this.maximumOverdraftAmount = maximumOverdraftAmount;
	   this.accountStatus = AccountStatus.OPEN;
	   this.accountNumber = ACCOUNT_NUMBER_GENERATOR.getAndIncrement();
	}

[Campbell Ritchie]

And what is wrong with
status = active ? Status.ACTIVE : Status.INACTIVE;
?

[Junilu Lacar]

    /** 
     * This will change the bank account status from active to inactive when sufficient 
     * amount is not available in user account or inactive to active when user deposit 
     * sufficient amount in his or her account.  
     *  
     * @param isActive   to change bank account status. true to make active 
     *                   false to make account inactive. 
     * @throws IllegalStateException   When trying to change the status of already closed  
     *                                 accounts 
     */  
    public void setAccountActive(boolean isActive) throws IllegalStateException {  
          
        Boolean value = new Boolean(true) ;  
          
        this.accountStatus = value.compareTo(isActive) == 0   
                              ? AccountStatus.OPEN   
                              : AccountStatus.INACTIVE ;  
    }  

There are a few things that are not quite right with this code.

First, the comment includes a description of logic that does not exist at all in the code. That is, despite what the comment says, there is not one line of code in the method that examines the account balance and determines "sufficiency" of that amount before setting the status.

Second, passing in a boolean parameter is a code smell that indicates a method that should probably be split into two methods. So instead of setAccountActive(boolean), prefer having two methods instead: activate() and inactivate() - this makes the code more focused, that is, each method does one and only one thing, and it makes the semantics clearer. Compare setAccountActive(true) vs activate() and setAccountActive(false) vs inactivate().

As for the "rule" that involves account balances and "sufficiency", I would try to define an AccountRule, perhaps as an interface, like so:

public interface AccountRule {
   void apply(Account account);
}

public class MinimumBalanceToRemainActiveRule implements AccountRule {
   private BigDecimal minimum;
   public MinimumBalanceToRemainActiveRule(BigDecimal minimum) {
      this.minimum = minimum;
   }

   public void apply(Account account) {
      if (hasMinimumBalance(account)) {
         account.activate();
      } else {
         account.inactivate();
      }
   }

   private boolean hasMinimumBalance(Account account) {
       return account.getBalance().compareTo(this.minimum) >= 0;
   }
}

Of course, there would be other variations to this, depending on how complex your rule set is. For example, you may want to only do something if at least one rule in a set of rules is satisfied. Or do something only if all rules in a given set are satisfied.

[Tushar Goel]

And what is wrong with

My goodness.. It is very simple but i could not able to think of this. I slapped my forehead.

Junilu
First, the comment includes a description of logic that does not exist at all in the code.

Sorry. Actually i made several correction in my account class since i made though i forgot to update comments as well due to lack of practice... I am sorry again.
I will keep them updated as well as soon as i update the method logic.

a method that should probably be split into two methods.

Thanks it looks more cleaner approach then i was doing.

As for the "rule" that involves account balances and "sufficiency", I would try to define an AccountRule, perhaps as an interface, like so:

Thanks a lot. It is very good example better than i can able to thought. I thought to make individual interface for each rule like for MinimumBalance, InteresetRate, Penalty etc.. and let the subclass of
Account class implement them but that one too not worked. So we (Campbell and I) discussed to let this initialization do using constructors.
So now i think i should add both of your advice into one Like let the initialization done through constructor and in the method do check using the individual classes

[Campbell Ritchie]

Tushar Goel wrote: . . . i could not able to think of this. . . .

Really? It is what you wrote earlier minus the == true bit. Well, as near as I could remember it, anyway.

[Campbell Ritchie]

Junilu Lacar wrote: . . .
First, the comment includes a description of logic that does not exist at all in the code. . . .

I thought we had discussed reopening a closed account, in which case that method should throw an Exception.

Second, passing in a boolean parameter is a code smell that indicates a method that should probably be split into two methods. . . .

If you use the bean pattern, however, activate and deactivate methods would not fit. It would be setActivated(true) rather like setVisible rather than show and hide.

[Junilu Lacar]

Tushar,

Don't get down on yourself or apologize for not knowing any better. It's almost always the case that beginners tend to write more code than is necessary. In time and with more experience, you will learn how to write more concise code.

One of the best ways to get better at programming is to read other people's code. Look for code that's easy to understand rather than code that is "clever" - prefer clear and concise code over clever but convoluted code. Then of course there's elegant code that combines being clear, concise, and clever. Also, read a few style guides. Here's a search that brings up a few, including JavaRanch's own style guide: java style guide -- read these, try them out and see what works for you.

[Junilu Lacar]

Campbell Ritchie wrote:If you use the bean pattern, however, activate and deactivate methods would not fit. It would be setActivated(true) rather like setVisible rather than show and hide.

Either way is acceptable, IMO. I think it just boils down to the choice of names and the style you want to follow.

setActivated(false) and deactivate() are both preferable to setAccountActive(false). Once you have a name that makes sense, you just have to decide whether following bean conventions is more important than not passing in a boolean parameter. You could also reason that the semantics of setActivated(boolean) is clear enough and doesn't really smell, and you're following bean conventions to boot, to which I would probably say, "Sure!"

[Campbell Ritchie]

OK