Spring Hibernate Reviewing a given Java code in job interviews and code review sessions

In job interviews, it is common to give a smippet of bad code, and ask you to describe what is wrong with the code and how you would go about improving it. A thorough review of the code will be beneficial not only in job interviews to impress your potential employers, but also to impress your peers and superiors during code review sessions.

Q. How will you go about improving on the following code snippet that calculates the new balance based on the current balance, total debits, and total credits?

 Note: All amounts need to be positive values.

import java.math.BigDecimal;

public class CashCalculatorBasic {

 public BigDecimal getCalculatedAvailableBalance(BigDecimal currentBalance, BigDecimal totalDebits, 
                                           BigDecimal totalCredits) {
  
  BigDecimal result =  currentBalance
     .subtract(totalDebits)
     .add(totalCredits);
  
  System.out.println("The calculated result is " + result);
  
  return result;

 }

 public static void main(String[] args) {
    
    new CashCalculatorBasic().getCalculatedAvailableBalance(
    new BigDecimal("1250.00"), new BigDecimal("250.00"), new BigDecimal("500.00"));
 }

}

A. Firstly, a good thing about the above code is that it uses the BigDecimal class instead of a floating point type like float or double. Here are a number of things that can be improved.


1. Don't use System.out.println(.....) and replace it with the log4j and slf4j framewowrks. It is a bad practice to use System.out.println because you cannot easily change log levels, turn it off, customize it, etc. A proper logging system, like Log4J, adds all sorts of features that are very useful in real applications. You can direct the output to different places (console, file, network, etc.). You can tell it to output only a subset of the messages, without having to recompile. You can get timestamp on each message, etc.




2. Don't use static void main method to test your class. Write unit tests using a unit testing framework like JUnit or TestNG.

3. Don't test only the happy path. Include negative test scenarios as well. By writing proper test cases, the code can be refactored with confidence.

4. The above code does not fail fast. A fail fast code is the one that performs input validation and throws any validation errors if the pre-conditions or post conditions are not met. For example, testing for null, empty String, negative values, etc.

5. Your code should favour code to interface. What if you want to write an improved CashCalculator? You will have to go and change all your references where CashCalculatorBasic is used.

6. The readability can be improved by abstracting out the calculation to a separate util class or an inner class as shown below. This will be really useful when you have more methods to calculate different values, which would be the case in real life scenario. This will also improve readability and reusability of the code. Here is the improved code. Define an interface, which is the contract for the invokers or callers.

import java.math.BigDecimal;


public interface CashCalculator {
 
  BigDecimal getCalculatedAvailableBalance(BigDecimal currentBalance, BigDecimal totalDebits, 
                                     BigDecimal totalCredits) ;

}

Now, define the implementation class that takes into consideration the best practices listed above. even though it has more code, as the class takes on more methods, it will improve its reusability and readability.

import java.math.BigDecimal;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;


/**
 * 
 * coding to an interface
 *
 */
public class CashCalculatorImproved implements  CashCalculator {
 
  //using a logging framework 
 private static final Logger log = LoggerFactory.getLogger(CashCalculatorImproved.class);

 public BigDecimal getCalculatedAvailableBalance(BigDecimal currentBalance, BigDecimal totalDebits, 
                                           BigDecimal totalCredits) {
      
  //improved readability
  BigDecimal result = new Balance(currentBalance)
                             .less(totalDebits)
                             .more(totalCredits)
                             .getBalance();
      
   //using a logging framework 
      if(log.isDebugEnabled()) {
       log.debug("The calculated result is {0}" , result);
      }
      
      return result;
 }

 //an inner class that abstracts out the calculation logic
 private class Balance {

  private BigDecimal balance;

  public Balance(BigDecimal balance) {
   //check for pre condition
   if (balance == null) {
      throw new IllegalArgumentException("Invalid balance " + balance);//fail fast
   }
   this.balance = balance;
  }

  public BigDecimal getBalance() {
   return balance;
  }

  public Balance less(BigDecimal amount) {
   //check for pre condition
   if (!isPositiveOrZero(amount)) {
      throw new IllegalArgumentException("Invalid amount " + amount);//fail fast
   }

   balance = balance.subtract(amount);
   
   //check for post condition
   if(!isPositiveOrZero(balance)){
      //should throw a real business exception
      throw new RuntimeException("The balance is negative " + balance);//fail fast
   }

   return this;
  }

  public Balance more(BigDecimal amount) {
   //check for pre condition
   if (!isPositiveOrZero(amount)) {
    throw new IllegalArgumentException("Invalid amount " + amount);//fail fast
   }

   balance = balance.add(amount);
 
   return this;
  }

  //a method with a meaningful name
  private boolean isPositiveOrZero(BigDecimal amount) {
     return amount != null && amount.signum() != -1;
  }

 }

}

The above code also demonstrates a good use of an inner class. Finally the unit tests that test for both positive (i.e. happy path) and negative scenarios.

import java.math.BigDecimal;

import junit.framework.Assert;

import org.junit.Before;
import org.junit.Test;

public class CashCalculatorImprovedTest {
 
 CashCalculator calculator = null;

 @Before
 public void init() {
  // prefer Spring/Guice to inject the dependency
  calculator = new CashCalculatorImproved();
 }

 @Test
 public void testCalculatedAvailableBalance(){
  BigDecimal result = calculator.getCalculatedAvailableBalance(
    new BigDecimal("1250.00"), new BigDecimal("250.00"), new BigDecimal("500.00"));
  Assert.assertEquals(new BigDecimal("1500.00"), result);
 }
 
 @Test
 public void testForZeroCalculatedAvailableBalance(){
  BigDecimal result = calculator.getCalculatedAvailableBalance(
    new BigDecimal("1250.00"), new BigDecimal("0.00"), new BigDecimal("0.00"));
  Assert.assertEquals(new BigDecimal("1250.00"), result);
 }
 
 
 @Test(expected=IllegalArgumentException.class)
 public void negativeTest1CalculatedAvailableBalance(){
   calculator.getCalculatedAvailableBalance( null, new BigDecimal("250.00"), new BigDecimal("500.00"));
 }
 
 @Test(expected=IllegalArgumentException.class)
 public void negativeTest2CalculatedAvailableBalance(){
   calculator.getCalculatedAvailableBalance( new BigDecimal("1250.00"), new BigDecimal("-25.00"), new BigDecimal("500.00"));
 }
 
 @Test(expected=IllegalArgumentException.class)
 public void negativeTest3CalculatedAvailableBalance(){
   calculator.getCalculatedAvailableBalance( new BigDecimal("1250.00"), new BigDecimal("250.00"), new BigDecimal("-500.00"));
 }
}  

More similar examples are covered in my Java career Essentials and companion books. For example, you will be expected to know what design pattern to apply to a given piece of code and design patterns are discussed with examples in "Java/J2EE Job Interview Companion".