Trying to create OO based bank app

[Tushar Goel]

It means that once you have an overridden equals method you should not add any fields in subclasses.

Oh Thanks.. but in case if i add any fields in subclass then i again need to override equals and hashcode method in subclass. Right?

[Campbell Ritchie]

No, if you add a field in the subclass it means you cannot regain symmetrical behaviour in the equals method without breaching the Liskov Substitution Principle.

[Tushar Goel]

you cannot regain symmetrical behaviour in the equals method without breaching the Liskov Substitution Principle.

Right.. It breaks LS principle.. I forgot about this principle. I will note the same in my notebook(Just start preparing from today to avoid possible forget problem in future)

So, if Customer class is ok then should i start working on Account class?

[Campbell Ritchie]

Test Customer. Try to “break” it.

[Tushar Goel]

I tried to test customer class specially equals method and customerId and they looks ok but unable to think how to break it. Can you please give me some hint?
Should i change access specifiers of my fields to protected from private and add setter method ???

package test;
public class CustomerTest {

	@Test
	public void testCustomer() {
		
		Customer customer = new Customer("Tushar Goel", new LocalDate(2012, 3, 12));
	    assertNotNull(customer);
	}

	@Test
	public void testEquals() {
		
		Customer customer = new Customer("Tushar Goel", new LocalDate(2014, 07, 17));
		Customer customer1 = new Customer("Mohit Goel", new LocalDate(2014, 07, 17));

		assertTrue(customer.equals(customer));
		assertTrue(customer1.equals(customer1));
		
		assertNotEquals(customer, customer1);
		assertNotEquals(customer1, customer);
		
		assertNotEquals(customer.hashCode(), customer1.hashCode());
			
		assertNotNull(customer);
		assertNotNull(customer1);
	
		DataReadWrite.writeDataToFile("CustomerId.txt", String.valueOf(Customer.getMaximumCustomerIdUsed()), false);

	}
	
	@Test
	public void testCustomerId() {
		
		Customer customer1 = new Customer(null, null);
		Customer customer2 = new Customer(null, null);

		assertNotEquals(customer1.getCustomerId(), customer2.getCustomerId());
		assertNotNull(customer1);
		assertNotNull(customer2);
		
		customer2 = new Customer("Tushar", null);
		assertNotEquals(customer1.getCustomerId(), customer2.getCustomerId());;
		
		customer2 = new Customer("Tushar", new LocalDate(2014, 03, 02));
		assertNotEquals(customer1.getCustomerId(), customer2.getCustomerId());;
		
		customer1 = new Customer("Tushar", null);
		assertNotEquals(customer1.getCustomerId(), customer2.getCustomerId());
		
		customer1 = new Customer("Tushar", new LocalDate(2014, 03, 02));
		assertNotEquals(customer1.getCustomerId(), customer2.getCustomerId());
		
		customer1 = new Customer(null, new LocalDate(2014, 03, 02));
		assertNotEquals(customer1.getCustomerId(), customer2.getCustomerId());
		
		List<Integer> customer = new ArrayList<Integer>(100);
		
		for (int i = 0; i < 100; i++) {
			customer.add(new Customer("Tushar", new LocalDate(2014, 03, 02)).getCustomerId());
		}
		
		Set<Integer> uniqueCustomer = new HashSet<Integer>(customer);
		
		assertEquals(customer.size(), uniqueCustomer.size());
		
		DataReadWrite.writeDataToFile("CustomerId.txt", 
				                          String.valueOf(Customer.getMaximumCustomerIdUsed()),
				                                 false);
		
	}
}

[Campbell Ritchie]

That isn't trying to break it. You need to try it out with abnormal input.

Did you manage to create an instance by passing null for name? If you did, then you have broken the class. You now need to repair the class so that won't happen again.

[Tushar Goel]

Did you manage to create an instance by passing null for name?

Yes, i did but i have 1 question. I am using utility class to receive output, in this case null or invalid input is not allowed but for testing i am directly
passing input to the Customer class constructor and in that no check is there, so it is receiving any input including null. So do i need to put check
in Customer class as well but if i do so then isn't duplicity of the code?

[Tushar Goel]

Thanks Campbell..., i figured it out. I realized that it is responsibility of the Customer class to validate the input not the utility or bank class. I need to put a check in
customer class but is it valid to throw exception from the constructor in case of invalid input?

  public class Customer {

       // some declartion

       /**
	 * Initializing value
	 * 
	 * @param customerName
	 * @param dateOfBirth
	 * @throws IllegalArgumentException  When name contains other than alphabets and whitespace
	 * @throws NullPointerException      When name and date of birth is null
	 */
	public Customer(String customerName, LocalDate dateOfBirth) 
				throws IllegalArgumentException, NullPointerException {
		
		if (customerName == null || dateOfBirth == null) {

		    throw new NullPointerException("Name or Date of birth can not be null");

 	       } else if (!customerName.matches("[a-zA-z\\s]+")) {

				throw new IllegalArgumentException("Only alphabets or whitespace is allowed in name");		
		}  
		
		this.customerName = customerName;
		this.dateOfBirth = dateOfBirth;
		
		customerId = customerIdGenerator.incrementAndGet();
	}

    // other methods
}

Bank Class:

	/**
	 * This method will take customer detail to open a new account. It will
	 * only accept name as combination of alphabets or whitespace. Whitespace
	 * is optional. As per the user input it call generateCustomer method
	 * to call customer class and create customerid. If input is invalid or
	 * null then it again ask for the correct input.
	 */
	public void createNewAccount()  {
	
		System.out.println("Welcome to XYZ bank, please enter your details:");
		
		Customer customer = null;
		
		while (customer == null) {
			
			// taking customer name
		    String name = KeyboardInput.userInput("Enter your  name: ");

		    //taking customer date of birth
	 		LocalDate dateOfBirth = 
	 				KeyboardInput.getDate("Enter date of birth (dd-mm-yyyy): ");
	 	
		 	customer = generateCustomer(name, dateOfBirth);
		}	 	
	}

	
    /**
     * This class will pass the value to the Customer class to generate customerId.
     * If the user input is in invalid format or null then null is returned.
     * 
     * @param name          Customer name
     * @param dateOfBirth   Customer date of birth 
     * @return customer     Particular customer class object
     * @exception  IllegalArgumentException  When name contains other than alphabets and whitespace
     * @exception  NullPointerException      When name and date of birth is null
     */
	public Customer generateCustomer(String name, LocalDate dateOfBirth) {

		Customer customer = null;
		
		try {
			
		  customer = new Customer(name, dateOfBirth);
		
		} catch (NullPointerException | IllegalArgumentException e) {
		
			System.out.println(e.getMessage());
		}
		
		return customer ;
	}

I also tried to test now and it is not breaking now...

public class CustomerTest {

	@Test(expected=NullPointerException.class)
	public void testCustomerNullPointerException() {
		
		Customer customer = new Customer("Tushar", new LocalDate(2012, 3, 12));
	//	customer = new Customer(null, new LocalDate(2012, 3, 12));
	//	customer = new Customer("Tushar", null);
		customer = new Customer(null, null);   
	}

	@Test(expected=IllegalArgumentException.class)
	public void testCustomerIllegalArgumentException() {
		
		Customer customer = new Customer("Tushar", new LocalDate(2012, 3, 12));
	//	customer = new Customer("123", new LocalDate(2012, 3, 12));
  		customer = new Customer("!Tus####", new LocalDate(2012, 3, 12));
	    
	}
}

[Campbell Ritchie]

There are various ways you can handle nulls there but you are right. The Customer class needs to validate its own input and throwing an Exception is probably the best response.
Don't catch unchecked Exceptions in production code. That catch is obviously only used for testing.
Careful about alphanumeric input for names only in production code. I know people called Anne-Marie and O'Neill. There are all sorts of weird and wonderful names around.

I think you have too many blank lines in your code. No need for a blank line after try { nor before }

[Tushar Goel]

Don't catch unchecked Exceptions in production code

But like in my code, if i do not catch NullPointerException or IllegalArgumentException , it terminate my complete application and i do not want that to
happen only merely due to wrong input. So, i catch them and asked from user again but as am recovering from the exception so it should be checked exception not unchecked.
What should i do then?

There are all sorts of weird and wonderful names around.

Thanks, never thought about so much variety in the name but we need to restrict ourself for certain input format. like name can have alphabets , space , special characters like [. _ - ' ] (excluding square braces)

[Campbell Ritchie]

In which case your app will have to make sure there is never a null input. If Customer can't accept null from the bank, then the bank must make sure it only gets a non‑empty input from the users. Go back to where I was on about nextLine and made a mistake, and you will find out how to get a non‑empty name.

There is a security hazard associated with catching NPEs but I don't know what it is.

[Tushar Goel]

you will find out how to get a non‑empty name.

Yes, my utility class is not accepting any non empty name.

This is my utility class method and through this i am accepting non empty string. I am also validating this input in my customer class where i am doing check for alphabets and some special
characters.

 
       public static final String userInput(String prompt) {
		  String value ;
		  System.out.print(prompt);
		  while ((value = INPUT.nextLine()).trim().isEmpty()) {
			  	System.out.println("Not a valid input. Please try again.");
			  	System.out.print(prompt);
		  }  
		  return value;
	  }

[Campbell Ritchie]

You do realise you may be printing that error message too early? Look at this post.

[Tushar Goel]

Please ignore this post.. I am unable to delete this..

[Tushar Goel]

You mean the mistake you were mentioning... but i am not getting that one i have already corrected that.

But if i have to take corrected input from the user first time only then i need to validate the same in Bank class itself not in Customer class. I can define separate static method in Customer class
which return boolean to validate the input and call them from the Bank class.

like:
Bank

 	
 	      // taking customer name
	      String name = KeyboardInput.userInput("Enter your  name(Alphabets): ");
              while (!Customer.validateName(name)) {
                       name = KeyboardInput.userInput("Enter your  name: ");
               }
    

Customer:

   
    protected static boolean validateName(String name) {
                 return customerName.matches("[a-zA-z\\s'.-]+") ;
        } 

Is this one you are referring to?

[Campbell Ritchie]

No, it was the mistake I made about six months ago. Prasanna Raman found it.

And what is wrong with double validation?

[Tushar Goel]

No, it was the mistake I made about six months ago. Prasanna Raman found it.

I am talking about the same. In my version, i already resolved this problem. Sorry, i was confused so pasted confused things as well

And what is wrong with double validation?

There is nothing wrong, i also supported the same. I was confused with your statement "You do realise you may be printing that error message too early? " I thought that you
were talking about 2 things: 1) why error message displaying twice 2) the mistake you were referring to..

I think i need to think more and seriously i will try. Thanks again...

So i think my test also successful and Customer class does not break. If you agree then can i move ahead to Account class or still something pending which i not noticed?

[Campbell Ritchie]

If you call nextLine after nextXXX you get an empty String. So if your input is

123 456 789
Campbell

… you get an empty String from nextInt() × 3 nextLine()
So you print out the error message while the user is writing Campbell.
You may have to wait one line before you start printing error messages.

[Tushar Goel]

You may have to wait one line before you start printing error messages.

Yeah if i take int first and then try to read using nextLine() then it will skip that line. As this problem comes only when we are reading after nextXXX() so i already
solve this in my getInt() method. So i can call now use userInput() method safely.

   public static final int getInt(String prompt) {
	      System.out.print(prompt);	  
		   while (!INPUT.hasNextInt()) {
			   System.out.println("Not a number. Please enter again");
			   System.out.print(prompt);
			   INPUT.next();
		   } 
		   int value = INPUT.nextInt(); <-------------------------
		   
		   // to remove enter key from the int output
		   INPUT.nextLine(); 	   <-------------------------------
		   return value;
	  }

[Campbell Ritchie]

Tushar Goel wrote: . . .
Yeah if i take int first and then try to read using nextLine() then it will skip that line. As this problem comes only when we are reading after nextXXX() so i already
solve this in my getInt() method. So i can call now use userInput() method safely.

   public static final int getInt(String prompt) {
	      System.out.print(prompt);	  
		   while (!INPUT.hasNextInt()) {
			   System.out.println("Not a number. Please enter again");
			   System.out.print(prompt);
			   INPUT.next();
		   } 
		   int value = INPUT.nextInt(); <-------------------------
		   
		   // to remove enter key from the int output
		   INPUT.nextLine(); 	   <-------------------------------
		   return value;
	  }

No, the nextInt method and next line do not skip the next line. The nextLine method reads the remainder of the current line. If you have input like

123 456 789
Campbell

… then reading with your method will find 123 and discard 456 789.

[Tushar Goel]

Thanks Campbell.. I have made correction in both the utility class methods.

    public static final String userInput(String prompt) {
		  
	  String value ="";
	  System.out.print(prompt);
		  
	   while (value.trim().isEmpty()) {
		   value = INPUT.nextLine();
	   } 
	return value;
    }


    public static final int getInt(String prompt) {
		
	  System.out.print(prompt);
  	   while (!INPUT.hasNextInt()) {
		   System.out.println("Not a number. Please enter again");
		   System.out.print(prompt);
		   INPUT.next();
	   } 
	   return INPUT.nextInt();
    }
 

[Campbell Ritchie]

Looks good

[Tushar Goel]

Thank you..What is next now? Should i start work on Account class or still something i am missing?

[Campbell Ritchie]

Yes, try the Account class.

[Tushar Goel]

I think general Account have following things:

1) Each account has unique account number.

2) Account status can be Active , Inactive or Closed.

3) Account can be of Saving, Current or Salary type .

4) Minimum amount require to open an account. Saving: 2000 Indian Rupees , Current 10000 Indian Rupee , Salary 0

5) Minimum balance require to maintain per day Saving: 2000 Indian Rupees , Current 10000 Indian Rupee , Salary 0

6) Interest calculate per day basis on the balance and credited to the customer Quarterly. Saving: 6% Annual , Current and salary: 0%

7) If user unable to maintain minimum balance then penalty of 200 Indian Rupees per quarter will be charged and debited from the user account quarterly

8) In case of balance goes to negative due to penalty then account become inactive

9) If during withdraw balance is insufficient then transaction is failed

10) User can deposit only in Active or Inactive account

11) Withdraw is possible from only active account

12) if account is inactive then user needs to deposit minimum balance + penalty amount to make it again active

13) Once account is closed it can not be re-open.

I think these many points.. Are they ok or i am missing something?

[Campbell Ritchie]

Tushar Goel wrote: . . .
1) Each account has unique account number.

Agree

2) Account status can be Active , Inactive or Closed.

Are you sure this is necessary at this stage? Probably right however.

3) Account can be of Saving, Current or Salary type .

How do you know there will be three types?

4) Minimum amount require to open an account. . . .

Why are you specifying that sort of thing at this stage? Even if there are minima, they will change from time to time. So will interest rates and charges.

. . .
13) Once account is closed it can not be re-open. . . .

Agree.

But you are specifying much too much at this stage. Many of those things will change from time to time, so you need to work out rules rather than having them hard‑coded.
You may also need Transaction classes or a Transaction interface.

[Tushar Goel]

Thanks Campbell but have some doubt here. Why we are not defining other things like minimum amount require to open , minimum balance required, interest and penalty
now, i understand they keep changing and can not be hard coded but then how we pass there value later and where we define them? Regarding account type you are
right there may be other type as well and each account have separate features.

What i am thinking, right now after making changes as you suggested we are defining basic version and later as per specific requirement we will extend this class and
add features as required?

Updated rule:

1) Each account has unique account number.

2) Account status can be Active , Inactive or Closed.

3) If during withdraw balance is insufficient then transaction is failed

4) User can deposit only in Active or Inactive account

5) Withdraw is possible from only active account

6) Once account is closed it can not be re-open.

[Campbell Ritchie]

What if you have $10000 in the account and wish to withdraw exactly $10000?

[Tushar Goel]

We can withdraw but account will be INACTIVE after that.

Adding more rule:

7) customer can withdraw only in dollars not in cents. like 100$ not 100.25$.

8) customer can withdraw all the amount from its account. Balance can go 0 but not negative and also once balance becomes 0 then it will changed it to INACTIVE.
To make it active again customer needs to add money into it.

9) Account Number and CustomerId is required for making withdraw

10) Only account number is required for making deposit

11) Only 2 decimal points are allowed. Rounding off will be done after 2 decimal points in round half up.

[Campbell Ritchie]

Why only whole numbers of dollars?
An empty account would be empty not inactive. You may close an empty account but it is only inactive if there has been no activity for a long time.
Why are you rounding in the account? Any rounding should be done before anything gets anywhere near the account.

[Tushar Goel]

Why only whole numbers of dollars?

I was considering withdraw through ATM only so i forgot to think the case of online transfer..

An empty account would be empty not inactive. You may close an empty account but it is only inactive if there has been no activity for a long time.

Thanks for the correction

Why are you rounding in the account? Any rounding should be done before anything gets anywhere near the account.

You mean before amount is added into the balance.

But Campbell, i still have one doubt which i pasted yesterday.. Could you please clear it? Below was my query:

.... Why we are not defining other things like minimum amount require to open , minimum balance required, interest and penalty
now, i understand they keep changing and can not be hard coded but then how we pass there value later and where we define them? .....

What i am thinking, right now after making changes as you suggested we are defining basic version and later as per specific requirement we will extend this class and
add features as required? Is it correct?

[Campbell Ritchie]

Tushar Goel wrote: . . . I was considering withdraw through ATM only so i forgot to think the case of online transfer...

I was thinking of an over the counter withdrawal: again it would be in $ and ¢

. . . You mean before amount is added into the balance.

Yes. Any amounts in or out should be rounded two 2dp before they get anywhere near the Account object.

But Campbell, i still have one doubt which i pasted yesterday.. Could you please clear it? Below was my query:

.... Why we are not defining other things like minimum amount require to open , minimum balance required, interest and penalty
. . .

Is a minimum amount an essential feature of an account? If it is, then code a minimum balance field in the superclass and set it to zero. Give any methods to set that amount appropriate access and rules for setting it. You can regard a negative minimum as a permitted overdraft.
Similarly interest and penalties could be superclass fields. Set them to zero too. That will get you out of the problem of adding fields later on and messing up the equals() method.

[Tushar Goel]

Is a minimum amount an essential feature of an account?

Not essential.

You can regard a negative minimum as a permitted overdraft.

Ok, i will take care and also minimum balance to maintain.

Similarly interest and penalties could be superclass fields. Set them to zero too

Ok, thanks but is not this hard coded again?

[Tushar Goel]

I also designed Transaction interface. Please suggest.

package onlinebanking;

import java.math.BigDecimal;

/**
 * @author etusgoe
 *
 */
public interface Transcation {

	/**
	 * This method will first check amount entered by the user and should be >= 0
	 * and it 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 IllegalStateException      When account is inactive or closed
	 * @throws IllegalArgumentException   When withdraw amount is <= 0 or
	 * 									  greater than (available balance + MAXIMUM_OVERDRAFT_LIMIT)									
	 */
	void withdraw(Account account, BigDecimal amountToBeWithdraw) 
		                       throws IllegalStateException, IllegalArgumentException;
	
	
	/**
	 * This will add the amount user want to deposit into its balance. Only amount > 0
	 * is accepted for deposit. 
	 * 
	 * @param account                   account in which deposit needs to be made
	 * @param moneyToBeDeposit          amount needs to be deposited
	 * @throws IllegalArgumentException  When amount <= 0
	 */
	void deposit(Account account, BigDecimal moneyToBeDeposit) 
										throws IllegalArgumentException;
	
	/**
	 * 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
	 */
	void closeAccount(Account account) throws IllegalStateException ;
	
	/**
	 * This will make the account inactive if the balance goes negative for
	 * more than 90 days.
	 * 
	 * @param account       account which needs to Inactive
	 * @param isNonActive   to make an account inactive. true - make inactive
	 *                      false - make active              
	 */
	void changeAcccountToInactive(Account account, boolean isNonActive);
	
	/**
	 * This will calculate the penalty on the account as per the bank norms
	 * Penalty will be applied when user do overdraft or unable to maintain 
	 * minimum balance.
	 *  
	 * @return  penalty amount            amount calculate as per bank norms
	 */
	BigDecimal penalty() ;
	
	/**
	 * This will calculate the interest earn on the account as per bank
	 * norms. There is no interest on Inactive or closed accounts
	 * 
	 * @return  BigDecimal    interest calculated on the balance
	 */
	BigDecimal interest();
}

[Campbell Ritchie]

No, that is not a transaction interface. That would suggest you can withdraw and deposit in the same transaction.

I suspect you only need a method to get the amount. Then you can have Deposit and Withdrawal as implementing classes or subclasses.

[Campbell Ritchie]

Tushar Goel wrote: . . .
Ok, thanks but is not this hard coded again?

Yes, but hard-coding zero is less serious.

[Tushar Goel]

. That would suggest you can withdraw and deposit in the same transaction.

Thanks Campbell. I was not thinking about it. You gave me new way to think as well..

I define transaction interface as below:

public interface Transaction {
    public void getAmount(Account account, BigDecimal amount) ;	
}

But i am getting feeling that if instead of interface if i create Transaction class then it will be nice so that for transaction single instance of transaction should create
and it will destroyed as well once transaction is either completed or failed. This should be called from the Bank class once user select any operation. I may be wrong.

 final class Transaction  {
	private final Account account;
	private final BigDecimal amount;

	public Transaction(Account account, BigDecimal amount) {
		this.account = account;
		this.amount = amount;
	}
	// getter methods
}

[Campbell Ritchie]

Why are you passing an account to that method? That looks like a procedural approach. Pass the transaction object to the appropriate method of the Account object. Or maybe you only need to pass the amount of money. Maybe the transaction class should be called amount.

I am not sure. But it isn't my project. It is yours. The idea is to make you think about it and query all your decisions; that way you will improve all your decisions.

[Tushar Goel]

Why are you passing an account to that method?

Thanks Campbell, was unaware that is procedural way.

he idea is to make you think about it and query all your decisions; that way you will improve all your decisions.

Yeah thanks Campbell, personally i also want the same so that i can able to develop my thinking..

I able to think 3 scenario:

1) Pass the account object , amount to the Transaction class constructor then call method of the account class from it but this may leak my account class reference and
also when my transaction is completed then this (transaction) object is not gc.

2) To pass amount in Transaction class and then pass this object into the account class methods. like account.deposit(Transaction t). This i think an OO way and better

3) To pass amount directly to the account class but then this will give direct exposure to my account class and also thinking this is not OO approach.

[Campbell Ritchie]

The account class should have a withdraw method.
Single Responsibility: a class takes care of itself and does its own actions. It does not take care of any other classes.