Trying to create OO based bank app

[Tushar Goel]

That means if user enters 06-01-1986 and if return back is 6-1-1986 then it is fine.... Oh great ....

Now if you think everything is fine then let us move towards Customer class....


Customer Class:

state: customerName, dateOfBirth, customerId, customerIdGenerator, CUSTOMER_ID_START_FROM
behaviour: getCustomerName(), setCustomerName(), getDateOfBirtth(), getCustomerId(), equals() and hashcode()

package onlinebanking;

import java.util.concurrent.atomic.AtomicInteger;

import org.joda.time.LocalDate;

public class Customer {

	private String customerName;
	private final LocalDate dateOfBirth;
	private final int  customerId;
	
	//starting range for the customer
	private static final int CUSTOMERID_START_FROM = 9999999;
	private static final AtomicInteger customerIdGenerator = new AtomicInteger(CUSTOMERID_START_FROM);
	
	

	/**
	 * Initializing value
	 * 
	 * @param customerName
	 * @param dateOfBirth
	 */
	public Customer(String customerName, LocalDate dateOfBirth) {
		this.customerName = customerName;
		this.dateOfBirth = dateOfBirth;
		
		customerId = customerIdGenerator.incrementAndGet();
	}


	/**
	 * @return the customerName
	 */
	public String getCustomerName() {
		return customerName;
	}


	/**
	 * @param customerName the customerName to set
	 */
	public void setCustomerName(String customerName) {
		this.customerName = customerName;
	}


	/**
	 * @return the dateOfBirth
	 */
	public LocalDate getDateOfBirth() {
		return dateOfBirth;
	}


	/**
	 * @return the customerId
	 */
	public int getCustomerId() {
		return customerId;
	}


	/* (non-Javadoc)
	 * @see java.lang.Object#toString()
	 */
	@Override
	public String toString() {
		return "Customer Details: [ " +  customerName + " " + 
					    dateOfBirth + " " + customerId + " ]";
	}


	/* (non-Javadoc)
	 * @see java.lang.Object#equals(java.lang.Object)
	 */
	@Override
	public boolean equals(Object obj) {
		
		if (obj == null) {
			return false;
		}
		
		return true;
	}


	/* (non-Javadoc)
	 * @see java.lang.Object#hashCode()
	 */
	@Override
	public int hashCode() {

		return customerId + 31 * (customerName.hashCode() + dateOfBirth.hashCode());
	}
	
	
}

[Tushar Goel]

I also thought that in Customer class , customerId should be saved some where so that if program is terminated for some reason then it should assign customer id greater than the already
assigned. It should not start again from the initial starting level.

Due to this i am as of now storing maximum customerid used into the file when program terminates. To do the same i again defined another utility which is reading and writing into the file.

package utility;

public final class DataReadWrite {

	private DataReadWrite() {
		
	}


	/**
	 * This method will write the data into the file provided by the user.
	 * Whether existing data is overwrite or append will be depend upon the
	 * user choice.
	 * 
	 * @param fileName    file name in which data needs to be written
	 * @param data        data needs to be written
	 * @param isAppend    Whether data needs to be append or overwrite existing one
	 * @exception IllegalStateException When unable to write data into the file
	 */
	public static final void writeDataToFile(String fileName, String data, boolean isAppend) {
		
		PrintWriter printWriter = null;
			
		try {
			if (isAppend) {
				printWriter = new PrintWriter(new BufferedWriter(new FileWriter(fileName, isAppend)), true);
			} else {
				printWriter = new PrintWriter(new BufferedWriter(new FileWriter(fileName)), true);	
			}
		} catch (IOException e) {
			throw new IllegalStateException("Unable to write in " + fileName);
		}
		
		printWriter.write(data);

		if (printWriter != null) {
			printWriter.close();
		}
		
	}
	

	
	/**
	 * This method will read first int number available into the file.
	 * Purpose for this method to read from such files where only single
	 * entry is present. Say: maximum employee id or customer or account no
	 * alloted to the user.
	 * 
	 * @param     fileName    file into which data will be saved
	 * @return    value       read from the file
	 * @exception FileNotFoundException  when file from which scanner reads is not
	 * 								     available
	 * @exception IllegalStateException When unable to write data into the file
	 */
	public static final int readFirstIntNumberFromFile(String fileName) {
		
		int value = 0 ;
		
		Scanner input = null;
		
		try {
			input = new Scanner(new File(fileName));
		} catch (FileNotFoundException e) {
			throw new IllegalStateException(fileName  +" file not found");
		}
		
		if (input.hasNextInt()) {
			value = input.nextInt();
		}
				 
		
		if (input != null) {
			input.close();
		}
		
		return value;
	} 

[Tushar Goel]

Modified Customer class:

Customer Class:

state: customerName, dateOfBirth, customerId, customerIdGenerator, CUSTOMER_ID_START_FROM
behaviour: getCustomerName(), setCustomerName(), getDateOfBirtth(), getCustomerId(), equals() and hashcode(), initializeCustomerId()

package onlinebanking;
 
public class Customer {

	private String customerName;
	private final LocalDate dateOfBirth;
	private final int  customerId;
	
	//starting range for the customer
	private static final int CUSTOMERID_START_FROM = initializeCustomerId();
	private static final AtomicInteger customerIdGenerator = new AtomicInteger(CUSTOMERID_START_FROM);
	
	/**
	 * Initializing value
	 * 
	 * @param customerName
	 * @param dateOfBirth
	 */
	public Customer(String customerName, LocalDate dateOfBirth) {
		this.customerName = customerName;
		this.dateOfBirth = dateOfBirth;
		
		customerId = customerIdGenerator.incrementAndGet();
	}

               private final static int initializeCustomerId() {
		
			 return  DataReadWrite.readFirstIntNumberFromFile("CustomerId.txt");
	}

	public static int getCustomeridStartFrom() {
		return customerIdGenerator.get();
	}

	//another getter setter methos

	@Override
	public String toString() {
		return "Customer Details: [ " +  customerName + " " + 
					    dateOfBirth + " " + customerId + " ]";
	}

	@Override
	public boolean equals(Object obj) {
		
		if (obj == null) {
			return false;
		}
		
		return true;
	}

	@Override
	public int hashCode() {

		return customerId + 31 * (customerName.hashCode() + dateOfBirth.hashCode());
	}
	
	
}

}

[Tushar Goel]

Campbell, just wondering if you got time to look at my last 2 posts?

[Tim Cooke]

Tushar, please EaseUp (<- click link and read). You are getting some incredible help here from Campbell but please do remember that we are all volunteers and have other demands on our time, like actual paying jobs, family, hobbies, whatever. Remember PatienceIsAVirtue so sit tight, I'm sure you'll get a response in good time.

[Tushar Goel]

Sorry Tim.. I completely understand your views and having completely respect to Campbell.. He already supported me very much and cannot explain thanks to him in words..

[Campbell Ritchie]

You have a dreadfully incorrect version of equals().

[Tushar Goel]

Thanks Campbell, i dont know what i thought at that time to create such method.. I just did some unit testing to see what error will i get if i use incorrect one and things is clear to me now..

Corrected one is as below:

	
   public boolean equals(Object obj) {
		
		Customer customer = (Customer)obj;
		
		if (this == obj) {
			return true;
		} else if (obj == null || (this.getClass() != obj.getClass())) {
			return false ;
		} 
		
		return   this.customerId == customer.getCustomerId() ;
							
	} 

[Paweł Baczyński]

Tushar Goel wrote:Corrected one is as below:

This is almost correct.
There is one problem. You attempt to cast obj to Customer before you check that you can do this.

[Tushar Goel]

Thanks Pawel, I have corrected that one too now..

 public boolean equals(Object obj) {  
               
        if (this == obj) {  
            return true;  
        } else if (obj == null || (this.getClass() != obj.getClass())) {  
            return false ;  
        }   
       
        Customer customer = (Customer)obj;  
   
        return   this.customerId == customer.getCustomerId() ;  
                              
    }   

[Campbell Ritchie]

That makes the hash code method incorrect. What will happen if you have this?
Customer:id=1234567890 name=Campbell Ritchie dob=12/3/45
Customer:id=1234567890 name=Tushar Goel dob=23/4/56

[Tushar Goel]

But i considering unique id.. No 2 customer can have same id...

I used following implementation to achieve the same:-

	//starting range for the customer
	private static final int CUSTOMERID_START_FROM = initializeCustomerId();
	private static final AtomicInteger customerIdGenerator = new AtomicInteger(CUSTOMERID_START_FROM);
	
	/**
	 * Initializing value
	 * 
	 * @param customerName
	 * @param dateOfBirth
	 */
	public Customer(String customerName, LocalDate dateOfBirth) {
		this.customerName = customerName;
		this.dateOfBirth = dateOfBirth;
		
		customerId = customerIdGenerator.incrementAndGet();
	}

	private final static int initializeCustomerId() {
		
			 return  DataReadWrite.readFirstIntNumberFromFile("CustomerId.txt");
	}

Is it wrong?? Also once application is down the maximum id used is being written in the file so that whenever it is up and id grater than already should be assigned:



Bank Class: To save already used customer id to the file

	
       private void saveImportantData() {
		String maxCustomerId = String.valueOf(Customer.getCustomeridStartFrom());
		DataReadWrite.writeDataToFile("CustomerId.txt", maxCustomerId, false);
	}

DataReadWrite Utility: Writing data into the file

	/**
	 * This method will write the data into the file provided by the user.
	 * Whether existing data is overwrite or append will be depend upon the
	 * user choice.
	 * 
	 * @param fileName    file name in which data needs to be written
	 * @param data        data needs to be written
	 * @param isAppend    Whether data needs to be append or overwrite existing one
	 * @exception IllegalStateException When unable to write data into the file
	 */
	public static final void writeDataToFile(String fileName, String data, boolean isAppend) {
		
		PrintWriter printWriter = null;
			
		try {
			if (isAppend) {
				printWriter = new PrintWriter(new BufferedWriter(new FileWriter(fileName, isAppend)), true);
			} else {
				printWriter = new PrintWriter(new BufferedWriter(new FileWriter(fileName)), true);	
			}
		} catch (IOException e) {
			throw new IllegalStateException("Unable to write in " + fileName);
		}
		
		printWriter.write(data);

		if (printWriter != null) {
			printWriter.close();
		}
         }
		

[Campbell Ritchie]

Tushar Goel wrote:But i considering unique id.. No 2 customer can have same id...
. . .

We are trying to make you think about it.

If the ID uniquely identifies the Customer, why not use only the ID in the hash code method?

[Tushar Goel]

Understood Sir.. That's means it will help to provide unique value

  public boolean equals(Object obj) {
		
		if (this == obj) {
			return true;
		} else if (obj == null || (this.getClass() != obj.getClass())) {
			return false ;
		} 

		Customer customer = (Customer)obj;
		return  (this.customerId == customer.getCustomerId()) &&
				     (this.customerName != null && customerName.equals(customer.getCustomerName()))  &&
				          (this.dateOfBirth != null && dateOfBirth.equals(customer.getDateOfBirth()));
							
	}

[Paul Clapham]

So it's possible to have two customers with the same ID but different names?

[Tushar Goel]

It is not possible to have same ID for two customers but by adding extra checks i think it is making doubly sure to have unique object only.

[Campbell Ritchie]

I would suggest you confirm that you cannot get duplicate IDs, then you only need to use ID in equals and hash code.

You should always use the same fields in hash code and equals.

[Tushar Goel]

confirm that you cannot get duplicate IDs

I tried to do some testing and it always give me unique case.. I am unable to think about the cases in which customerId is duplicated(except the case when application is shutdown and restarted again or the case of
multithreaded.). Also while testing i realized that my current hashCode implementation is wrong as when i passed null to the fields it throws NullPointerException.. I need to correct that one too...

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

		assertFalse(customer1.getCustomerId() == customer2.getCustomerId());
				
		customer2 = new Customer("Tushar", null);
		assertFalse(customer1.getCustomerId() == customer2.getCustomerId());
		
		customer2 = new Customer("Tushar", new LocalDate(2014, 03, 02));
		assertFalse(customer1.getCustomerId() == customer2.getCustomerId());
		
		customer1 = new Customer("Tushar", null);
		assertFalse(customer1.getCustomerId() == customer2.getCustomerId());
		
		customer1 = new Customer("Tushar", new LocalDate(2014, 03, 02));
		assertFalse(customer1.getCustomerId() == customer2.getCustomerId());
		
		customer1 = new Customer(null, new LocalDate(2014, 03, 02));
		assertFalse(customer1.getCustomerId() == customer2.getCustomerId());
		
		DataReadWrite.writeDataToFile("CustomerId.txt", 
				                          String.valueOf(Customer.getMaximumCustomerIdUsed()),
				                                 false);
		
	}

You should always use the same fields in hash code and equals.

Thanks for it. Earlier was not sure about it...

[Paul Clapham]

Tushar Goel wrote:It is not possible to have same ID for two customers

Yes, right, that's what I thought.

... but by adding extra checks i think it is making doubly sure to have unique object only.

In my opinion the equals() method is not the place to be making those "extra checks". If they aren't done correctly in the place where they should be done, then making your equals() method incompatible with your hashCode() method is just going to lead to more confusion.

[Tushar Goel]

If they aren't done correctly in the place where they should be done, then making your equals() method incompatible.

I tried to make them unique. I tested several cases on it to check and every time i am getting unique value. What other cases i need to test? Because if my implementation is
right then as Campbell suggested i need not to use anything else except customerID in equals() and hasCode().

I have added few more test cases to the existing one:

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

		assertFalse(customer1.getCustomerId() == customer2.getCustomerId());
				
		customer2 = new Customer("Tushar", null);
		assertFalse(customer1.getCustomerId() == customer2.getCustomerId());
		
		customer2 = new Customer("Tushar", new LocalDate(2014, 03, 02));
		assertFalse(customer1.getCustomerId() == customer2.getCustomerId());
		
		customer1 = new Customer("Tushar", null);
		assertFalse(customer1.getCustomerId() == customer2.getCustomerId());
		
		customer1 = new Customer("Tushar", new LocalDate(2014, 03, 02));
		assertFalse(customer1.getCustomerId() == customer2.getCustomerId());
		
		customer1 = new Customer(null, new LocalDate(2014, 03, 02));
		assertFalse(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());
		System.out.println("List " + customer.size() + " unique " + uniqueCustomer.size());
		
		DataReadWrite.writeDataToFile("CustomerId.txt", 
				                          String.valueOf(Customer.getMaximumCustomerIdUsed()),
				                                 false);
		
	}

[Campbell Ritchie]

Tushar Goel wrote: . . . I am unable to think about the cases in which customerId is duplicated(except the case when application is shutdown and restarted again or the case of
multithreaded.). . . .

In which case you need to handle those conditions. I can think of three ways to handle the shutdown problem but one involves a database and we decided earlier not to use databases in this instance. Consider a shutdown hook on the runtime which does one of these things

1: Persist the state in the database.

2: Persist state in an XML or similar file.

3: Persist the state by serialising the bank to a .ser file.

Remember shutdown hooks only hae a short time to operate in. If you were using Swing, you could add a Listener to listen for the app closing.
If you are using AtomicInteger, doesn't that obviate any problems caused by threading? Have you checked?

It is a valid decision at this stage to decide to delay persistence until later in the proceedings, as long as you do it eventually.

[Campbell Ritchie]

Tushar Goel wrote: . . . my current hashCode implementation is wrong as when i passed null to the fields it throws NullPointerException.. I need to correct that one too...
. . .

And the way to correct it is to make sure a null never gets that far.

[Campbell Ritchie]

Tushar Goel wrote: . . . Because if my implementation is right then as Campbell suggested i need not to use anything else except customerID in equals() and hasCode().
. . .

You need to go through the constructor and tease it apart with pencil and paper and confirm whether there are any circumstances in which you can get a duplicate ID. I presume that is what you did earlier when you said shut-down or threading. In which case check what you did to confirm whether your conclusion is correct.
What if you have two Bank instances in different threads? Can you get duplicate IDs in those instances?

[Tushar Goel]

Consider a shutdown hook on the runtime which does one of these things

Thanks Campbell.. Let me read about shutdown hooks and get back to you. Also i think in shutdown hook i will save state into the file.

If you are using AtomicInteger, doesn't that obviate any problems caused by threading? Have you checked?

Yeah i did.. AtomicInteger is thread safe and it helps to maintain a unique customer id.

you did earlier when you said shut-down or threading. In which case check what you did to confirm whether your conclusion is correct.

As of now when the application is shutting down i am saving the customerId into the file and whenever services is up then i reading entry from the file so
this way i am making sure that customerId should not again start from initial value and new customer will get oldValue +1.

What if you have two Bank instances in different threads? Can you get duplicate IDs in those instances?

i am using Atomic Integer which is thread safe so i think this will handle the case as well and no duplicate. Though i have not tested in multi threaded environment.

[Tushar Goel]

Consider a shutdown hook on the runtime which does one of these things

I have added shutdown Hook up in my BankStarter Class. As of now i am just storing customerId into the file. Once i will implements other i will
add them to. I defined a inner class in my Bank class for the same.

public class Bank {

  // fields declaration

    class StoreData extends Thread {
    
    	@Override
    	public void run() {
    		
    		String maxCustomerId = String.valueOf(Customer.getMaximumCustomerIdUsed());
    		DataReadWrite.writeDataToFile("CustomerId.txt", maxCustomerId, false);
    	}
    }
    
    // other methods
}

public class BankStarter {

	public static void main(String[] args) {
				
		Bank bank = new Bank();
		Runtime.getRuntime().addShutdownHook(bank.new StoreData());
		bank.startBankServices();      
	}
}

[Campbell Ritchie]

Looks good
Remember you are adding the shutdown hook to the Runtime, not a particular class. Also remember the Runnable must not do anything taking a long time because the lifetime of the JVM after the shutdown hook is activated is very short.

Now let's see you write that in Java8 notation without defining a Runnable class!

[Tushar Goel]

Now let's see you write that in Java8 notation without defining a Runnable class!

Do you mean using Lambda expression? I do not have any idea but let me read about it.

[Campbell Ritchie]

Search my posts for the last week and look out for a spactecular misspelling.

[Campbell Ritchie]

Try here for the wrong spelling.

[Tushar Goel]

Try here for the wrong spelling.

SingUtilities or SwingUtilities... Syntax are quite different for JRE-8, i need to read whole story about expressions... Pardon me for later replying as i was very busy from last 2 -3 days due
to family issue.. I will finish my work by today evening and start again concentrate on it....

[Tushar Goel]

Now let's see you write that in Java8 notation without defining a Runnable class!

Finally, i am able to get it. It looks better ad short. Is it what you referring to?

public class BankStarter {

    public static void main(String[] args) {
		
		Bank bank = new Bank();
		
	 	Runnable t1 = () -> Bank.storeData();
	 	Runtime.getRuntime().addShutdownHook(new Thread(t1));

		bank.startBankServices();
       	}
}

Also i have removed local inner class from the Bank and instead defined a static method to store data.

[Campbell Ritchie]

Agree it seems better without the inner class. But why is the store data method static?

[Tushar Goel]

Because i think it is independent to the instances though i agree there will be only one instance of the Bank class. So i thought both are equal but i think you are pointing
to possible risk to access the method directly from the Class?

[Campbell Ritchie]

… and where will the data come from, if not from the instance?

[Tushar Goel]

got your point. Thanks for clearing it.

 public class BankStarter {  
     public static void main(String[] args) {  
          
        Bank bank = new Bank();  
          
        Runnable t1 = () -> bank.storeData();  
        Runtime.getRuntime().addShutdownHook(new Thread(t1));  
  
        bank.startBankServices();  
        }  
} 

[Campbell Ritchie]

You're welcome

[Tushar Goel]

So my customer class is now looks like this. Is this correct now?

package onlinebanking;

import java.io.IOException;
import java.util.concurrent.atomic.AtomicInteger;

import org.joda.time.LocalDate;

import utility.DataReadWrite;

public class Customer {

	private String customerName;
	private final LocalDate dateOfBirth;
	private final int  customerId;
	
	//starting range for the customer
	private static final int CUSTOMERID_START_FROM = initializeCustomerId();
	private static final AtomicInteger customerIdGenerator = new AtomicInteger(CUSTOMERID_START_FROM);
	
	/**
	 * Initializing value
	 * 
	 * @param customerName
	 * @param dateOfBirth
	 */
	public Customer(String customerName, LocalDate dateOfBirth) {
		this.customerName = customerName;
		this.dateOfBirth = dateOfBirth;
		
		customerId = customerIdGenerator.incrementAndGet();
	}


	/**
	 * @return
	 */
	private final static int initializeCustomerId() {
		
			 return  DataReadWrite.readFirstIntNumberFromFile("CustomerId.txt");
	}


	/**
	 * @return the customerName
	 */
	public String getCustomerName() {
		return customerName;
	}


	/**
	 * @param customerName the customerName to set
	 */
	public void setCustomerName(String customerName) {
		this.customerName = customerName;
	}


	
	/**
	 * @return the customeridStartFrom
	 */
	public static int getMaximumCustomerIdUsed() {
		return customerIdGenerator.get();
	}


	/**
	 * @return the dateOfBirth
	 */
	public LocalDate getDateOfBirth() {
		return dateOfBirth;
	}


	/**
	 * @return the customerId
	 */
	public int getCustomerId() {
		return customerId;
	}


	/* (non-Javadoc)
	 * @see java.lang.Object#toString()
	 */
	@Override
	public String toString() {
		return "Customer Details: [ " +  customerName + " " + 
					    dateOfBirth + " " + customerId + " ]";
	}


	@Override
	public boolean equals(Object obj) {
				
		if (this == obj) {
			return true;
		} else if (obj == null || (this.getClass() != obj.getClass())) {
			return false ;
		} 
		
		Customer customer = (Customer)obj;
		
		return  (this.customerId == customer.getCustomerId());
							
	} 

	@Override
	public int hashCode() {

		return 31 * customerId;
	}
	
	
}

[Campbell Ritchie]

Looks all right.

But…
Is LocalDate immutable?
Why are you using a method rather than the field in equals? Why are you using else after a return? I prefer only to use return once per method so I would reduce that method to
return other == this || other != null && other.getClass().equals(this.getClass()) && ((Customer)other).id == id;
You do have to keep the terms in that order and use || && not | & to avoid a null exception.
Do you really want to reset names with set methods? What validation of names are you doing in those set methods?

Search these fora to find out about the controversy about whether you should or shouldn't use getClass in an equals method.

[Tushar Goel]

Is LocalDate immutable?

Yes it is..

Do you really want to reset names with set methods?

No. Removed now.

whether you should or shouldn't use getClass in an equals method.

Yes we should use getClass instead of instanceOf because instanceOf will not return false in case if argument is subclass of the original class but getClass will
return false. I refer this link http://www.javaranch.com/journal/2002/10/equalhash.html

What validation of names are you doing in those set methods?

No validation in set method but i think validation in terms of only alphabets and single whitespace allowed in name should be done in Bank class which accepting string from the utility class.

  
   private void createNewAccount()  {
  	  // taking customer name
	    String name = "";
	    
	    //only accept alphabets or zero or one whitespace
	    while (!name.matches("[a-zA-z\\s?]+")) { 
	    	name = KeyboardInput.userInput("Enter your  name: ");
	    }

	    //other stuff
   }

Why are you using a method rather than the field in equals? Why are you using else after a return?

for better readability. Thanks for the suggestion, i almost forgot this way to get fields from the object. I have update the code as suggested as well.

 @Override
 public boolean equals(Object obj) {
		
	return  this == obj 
  		    || obj != null 
		    && (this.getClass().equals(obj.getClass()))
		   && ((Customer)obj).customerId == customerId ;
							
	}

[Campbell Ritchie]

Tushar Goel wrote:

Is LocalDate immutable?

Yes it is.. . . .

You may have checked that before, but I couldn't remember, so I asked it again.

Coming on nicely

It is still worth reading the controversy about the equals method. It means that once you have an overridden equals method you should not add any fields in subclasses.