Feedback on ArrayList using OOPs

Question

Please review my code for the problem statement below. Many Thanks.

You job is to create a simple banking application.

There should be a Bank class:

• It should have an ArrayListof Branches
• Each Branch should have an ArrayListof Customer s
• The Customer class should have an ArrayListof Doubles (transactions)

Customer:

• Name, and the ArrayList of Doubles.

Branch:

• Need to be able to add a new customer and initial transaction amount.
• Also needs to add additional transactions for that customer/branch

Bank:

• Add a new branch
• Add a customer to that branch with initial transaction
• Add a transaction for an existing customer for that branch

Show a list of customers for a particular branch and optionally a list of their transactions Demonstration autoboxing and unboxing in your code.

import java.lang.Double;
import java.util.ArrayList;
public class Customer{
 private String name;
 private ArrayList<Double> transactions = new ArrayList<>();
 
 public Customer(String name,double initialTransaction){
 this.name = name;
 this.transactions.add(Double.valueOf(initialTransaction)); 
 }
 public String getCustomerName(){
 return this.name;
 }
 
 public Double getInitialTransaction(){
 return this.transactions.get(0);
 }
 
 public void addTransaction(double transaction){
 this.transactions.add(Double.valueOf(transaction));
 }
 public String getCustomerDetails(){
 return getCustomerName()+"\t\t\t"+transactions;
 }
}
import java.lang.Double;
import java.util.ArrayList;
public class Branch{
 private String name;
 private ArrayList<Customer> customers;
 public Branch(String name){
 this.name = name;
 this.customers = new ArrayList<>();
 }
 
 public String getBranchName(){
 return this.name;
 }
 
 public void addCustomer(Customer customer){
 if(customer!=null){
 if(!searchCustomer(customer)){
 this.customers.add(customer);
 //System.out.println("Customer with name "+customer.getCustomerName()+" added to branch "+getBranchName());
 }else{
 //System.out.println("Customer with name "+customer.getCustomerName()+" already present in branch "+getBranchName());
 } 
 }else{
 //System.out.println("Customer with null values entered!!!");
 }
 }
 public void addTransaction(Customer customer,double transaction){
 if(customer!=null || transaction<=0.0){
 if(searchCustomer(customer)){
 this.customers.get(this.customers.indexOf(customer)).addTransaction(transaction);
 //System.out.println("Transaction of "+transaction+" added in account of Customer with name "+customer.getCustomerName());
 }else{
 //System.out.println("Customer with name "+customer.getCustomerName()+" not present in branch "+getBranchName()+". Please add the customer.");
 } 
 }else{
 //System.out.println("Customer with null values or no transaction entered!!!");
 }
 }
 
 public void printCustomerList(){
 System.out.println("Customer Name \t\t Transactions");
 for(Customer cx:customers){
 System.out.println(cx.getCustomerDetails());
 }
 }
 
 public boolean searchCustomer(Customer customer){
 for(int i=0;i<this.customers.size();i++){
 if(this.customers.get(i).equals(customer)){
 return true;
 }
 }
 return false;
 }
}
import java.lang.Double;
import java.util.ArrayList;
public class Bank{
 private String name;
 private ArrayList<Branch> branches;
 
 public Bank(String name){
 this.name = name;
 this.branches = new ArrayList<>();
 }
 
 public String getBankName(){
 return this.name;
 }
 public void addBranch(Branch branch,Customer customer){
 if(branch!=null){
 if(!searchBranch(branch)){
 branch.addCustomer(customer);
 this.branches.add(branch);
 System.out.println("Customer with name "+customer.getCustomerName()+" with an initialTransaction of "+customer.getInitialTransaction().doubleValue()+" is added to Branch with name "+branch.getBranchName()+" of bank "+getBankName());
 }else{
 System.out.println("Branch with name "+branch.getBranchName()+" already present");
 }
 }else{
 System.out.println("Branch with null values entered!!!");
 }
 }
 
 public void addTransaction(Branch branch, Customer customer,double transaction){
 if(branch!=null || customer!=null){
 if(searchBranch(branch) && branch.searchCustomer(customer)){
 branch.addTransaction(customer,transaction);
 System.out.println("Transaction of "+transaction+" added in account of Customer with name "+customer.getCustomerName()+" under branch "+branch.getBranchName()+" of bank "+getBankName());
 }else{
 System.out.println("Customer with name "+customer.getCustomerName()+" not present in branch "+branch.getBranchName()+". Please add the customer.");
 } 
 }else{
 System.out.println("Branch or Customer with null values entered!!!");
 }
 }
 
 public void printCustomersOfBranch(Branch branch){
 if(branch!=null){
 if(searchBranch(branch)){
 branch.printCustomerList();
 }
 }else{
 System.out.println("Branch with null values entered!!!");
 }
 }
 
 private boolean searchBranch(Branch branch){
 for(int i=0;i<branches.size();i++){
 if(this.branches.get(i).equals(branch)){
 return true;
 } 
 }
 return false;
 }
}
public class BankTest{
 public static void main(String[] args){
 
 Bank pnb = new Bank("Punjab National Bank");
 
 Branch peachtree = new Branch("Peach Tree");
 
 Branch rodeodrive = new Branch("Rodeo Drive");
 
 Branch goodearth = new Branch("Good Earth");
 
 Customer harsh = new Customer("Harsh",0.5);
 
 Customer nidhi = new Customer("Nidhi",600.75);
 
 Customer yuv = new Customer("Yuv",1785.95);
 
 pnb.addBranch(peachtree,harsh);
 
 pnb.addBranch(rodeodrive,nidhi);
 
 pnb.addBranch(goodearth,yuv);
 
 pnb.printCustomersOfBranch(peachtree);
 
 pnb.printCustomersOfBranch(rodeodrive);
 
 pnb.printCustomersOfBranch(goodearth);
 
 pnb.addTransaction(peachtree,harsh,500.60);
 
 pnb.addTransaction(rodeodrive,nidhi,150000.70);
 
 pnb.addTransaction(goodearth,yuv,121798.12);
 
 pnb.printCustomersOfBranch(peachtree);
 
 pnb.printCustomersOfBranch(rodeodrive);
 
 pnb.printCustomersOfBranch(goodearth);
 }
}

• gervais.b: Do you expect some specifc remarks on the code. About the perfs, structure, maitainability, style, .. ?
• markspace: Importing java.lang.Double is unnecessary, all of java.lang.* is imported by default.
• Harsh Sengar: @gervais.b Well I am a starter(10th time), so any feedback would be helpful. Also would like a suggestion to concentrate solely on gaining conceptual understanding or also focus more on style, structure, maintainability because I haven't come across all these in the course I am following.
• Harsh Sengar: @markspace Duly noted. Also I noted that instead of calling Double.valueOf, I could directly pass the double value which will be autoboxed.

Answer 1

Demonstration autoboxing and unboxing in your code.

You can then remove the calls to Double#valueOf(double) and Double#doubleValue():doube. Then you can replace Double with double when applicable (return of public methods).

Searching items in list

At some times you are using List#indexOf(Object):int but in other places, you are looping on the list to test the equality of an item (searchCustomer, searchBranch).

The Javadoc of ArrayList#indexOf(Object):int states that :

returns the lowest index i such that (o==null ? get(i)==null : o.equals(get(i)))

So you can easily replace your loops in your search methods by return theList.indexOf(something) > -1

But there is still a better way to check if a list contains an item: ArrayList#contains(Object):boolean

Naming

Based on the previous comment, you could also rename your searchXyz methods to contains or another more meaningful name. This may be a cultural issue but, most of the time when "we" (BE_fr devs) read search, "we" expect to receive a collection of found objects.

Complexity

Instead of nesting your ifs like in Branch#addTransaction(..) you can fail quickly :

if ( customer==null || transaction <= 0.0 ) {
 // fail
} else if ( !contains(customer) ) {
 // fail 
} else {
 // This is fine
}

If you rely on Exception for your failure you can also reduce a bit the nesting of your method :

if ( isThisInvalid(..) ) {
 throw new InvalidOperation(..); 
} 
// This is fine

The less path/branches you have in your code, the better it is. That's also why I introduced a validation method, to move the validations rules outside of the business code.

Encapsulation

Not all your methods have to be public, try to mark as much as possible as private or package protected to reduce your public API.

Separation of concerns

Most of the time, for maintainability and testability concerns, you try extract concerns in different classes. That's why there are some popular high-level patterns like MVC, MVP, ...

In your case you could move the printing to a dedicated class that will be responsible to gather, format and print values from your model.

---

Here are some excerpt of how your final code may looks like:

class Branch {
 private final ArrayList<Customer> customers;
 private final String name;
 public Branch(String name) {
 this.customers = new ArrayList<>();
 this.name = name;
 }
 public String getBranchName() {
 return this.name;
 }
 public ArrayList<Customer> getCustomers() {
 return new ArrayList<>(customers);
 }
 public void addCustomer(Customer customer) {
 if (!isValidCustomer(customer)) {
 throw new InvalidCustomerException( customer);
 }
 customers.add(customer);
 }
 private boolean isValidCustomer(Customer customer) {
 return customer!=null &&
 !contains(customer);
 }
 public void addTransaction(Customer customer, double transaction) {
 if (!isValidaTransaction(customer, transaction)) {
 throw new InvalidTransactionException(customer, transaction);
 }
 getCustomer(customer).addTransaction(transaction);
 this.customers.get(this.customers.indexOf(customer)).addTransaction(transaction);
 }
 private boolean isValidaTransaction(Customer customer, double transaction) {
 return customer!=null &&
 transaction <= 0.0 &&
 contains(customer);
 }
 private boolean contains(Customer customer) {
 return customers.contains(customer);
 }
 private Customer getCustomer(Customer customer) {
 return customers.get(customers.indexOf(customer));
 }
} 
class BranchCustomersPrinter implements Consumer<Bank> { 
 private final PrintWriter output;
 @Override
 public void accept(Bank bank) {
 Branch branch = bank.getBranch(branch);
 output.append("Customer Name \t\t Transactions");
 branch.getCustomers().forEach(customer ->
 output.append(customer.getCustomerName())
 .append("\t\t")
 .append(customer.getTransactions())
 );
 output.flush();
 }
} 
Bank bank = ...
bank.print(new BranchCustomersPrinter(..));

---

Note; Your validation in Branch#addTransaction(..) seems to be buggy. It will accept a null customer with a transaction bigger than 0. It will also never accept a positive transaction.

---

Note; Java uses references and since your program will always run in memory you don't need to get an item from the list to update his values.

Customer c1 = new Customer("One", 10);
ArrayList list = new ArrayList();
list.add(c1);
c1.addTransaction(5); // Is the same as :
list.get(list.indexOf(c1)).addTransaction(5);

But most of the time; you have to save and retrieve the customer from a persistent storage.

This is also why list.contains(c1) returns true. Because it is effectively the same "object"; doing the same with two customers having the same properties will not work:

Customer c1 = new Customer("Customer", 10); 
Customer c2 = new Customer("Customer", 10);
c1.equals(c2); // false 

But I guess that you will learn the subtleties of object references and equals soon.

• Harsh Sengar: Doesn't this accomplish the same. Please correct me if I am wrong. getCustomer(customer).addTransaction(transaction); this.customers.get(this.customers.indexOf(customer)).addTransaction(transaction)
• gervais.b: It depends of how ‘getCustomer’ is implemented. But it can be the same yes. However ‘getSomething().doAnother()’ is not a good idea because it expose your implementation details. It is better to do ‘doAnother(Something on)’ so that you can change the may you store your objects. Read a bit on encapsulation to know more.