Codebox Software

I've used this question a few times in technical interviews for Java developers, and it worked quite well. Some suggested answers are available below the code.

The following class has been written by a junior developer and you have been asked to review it. Please provide suggestions for improvements, and short pieces of replacement code where appropriate. The class compiles without any errors, and will run on a Java 1.5 JVM.

public class Account {
    private String accountNumber;
    
    public Account(String accountNumber){
        // Constructor
        this.accountNumber = accountNumber;
    }
    
    public String getAccountNumber(){
        return accountNumber; // return the account number
    }
    
    public ArrayList getTransactions() throws Exception{
        try{
            List dbTransactionList = Db.getTransactions(accountNumber.trim()); //Get the list of transactions
            ArrayList transactionList = new ArrayList();
            int i;
            for(i=0; i<dbTransactionList.size(); i++){
                DbRow dbRow = (DbRow) dbTransactionList.get(i);
                Transaction trans = makeTransactionFromDbRow(dbRow);
                transactionList.add(trans);
            }
            return transactionList;
            
        } catch (SQLException ex){
            // There was a database error
            throw new Exception("Can't retrieve transactions from the database");
        }
    }
    
    public Transaction makeTransactionFromDbRow(DbRow row){
        double currencyAmountInPounds = Double.parseDouble(row.getValueForField("amt"));
        String description = row.getValueForField("desc");
        return new Transaction(description, currencyAmountInPounds); // return the new Transaction object
    }
    
    // Override the equals method   
    public boolean equals(Account o) {
        return o.getAccountNumber() == getAccountNumber(); // check account numbers are the same 
    }
}       

Click the headings below to view more details for each section.

• Comments
  The comments in this class are inane - they use up space on the screen, add no value, and just repeat what the code is saying. Comments should be used to explain why code does something, if you have to explain what the code is doing in a comment it means the code is unclear and should be refactored. At the moment these comments, although not useful, aren't actually harmful - however in time they could become so. If a developer changes the code and does not update the comments, then they start to misrepresent what the code does, and could cause confusion and misunderstanding.
• Floating point values
  Never use floating point types (float and double) for values that needed to be expressed exactly, such as currency amounts (see Effective Java 2nd Ed, Item 48). In this case, we should just use an int and store the amount as pence, or (if we need to handle fractions of pence) use the BigDecimal class.
• Overriding equals method
  This method does not actually override the equals method inherited from the Object class, it overloads it, which is not what the author intended (the 'o' parameter should have type Object not Account). This means that this custom equals method may or may not be used when two instances are compared, depending on the compile-time type of the reference:

  Object a = new Account("123");
  a.equals(new Account("123")); // returns false, custom method not used
  

  Account a = new Account("123");
  a.equals(new Account("123")); // returns true, custom method is used
  

  This is a common error, and is precisely why the @Override annotation was added to the language in Java 1.5
• Missing hashCode method
  A custom equals method should always have a corresponding custom hashCode method, never implement one without the other. (see Effective Java, 2nd Ed, Item 9)
• Equals method null safety
  It's easy to get custom equals methods wrong - this one isn't null-safe. If you try to compare an Account instance with null this method will throw a NullPointerException, rather than returning false as it should.
• String comparison
  This is usually a bad way to compare two Strings, especially because it may seem to work correctly most of the time and then fail, apparently for no reason. Using the equality operator ('==') checks whether the two variables refer to the same object, not whether the two objects have the same value, which was the intention here. The author should have used the equals method instead (with an appropriate null check). Strings are especially tricky because the JVM takes advantage of their immutability, and automatically 'interns' String literals, re-using the same objects in order to save memory:

  "text" == "text"                         // true
  new String("text") == "text"             // false
  new String("text") == new String("text") // false
  

• Method scope
  This method looks like it should be private, rather than public.
• ArrayList return type
  There is no good reason for having a return-type of ArrayList here. If the returned transactions need to accessed by index (eg transactionList.get(0)) then List is a good choice. If there is no such requirement then Collection is even better. By using an interface rather than a concrete class as the return type, the author retains the freedom to change the collection type without affecting any client code that uses the class.
• Constructor validation
  Depending on exactly how the class will be used, some validation on the accountNumber value may be a wise precaution. A check for null values would be especially valuable:

  public Account(String accountNumber){
      if (accountNumber == null){
          throw new NullPointerException("accountNumber argument was null");
      }
      this.accountNumber = accountNumber;
  }
  

  By throwing the NullPointerException explicity in the constructor, rather than waiting for it to happen later on (for example when .trim() is called in the getTransactions() method) the code ensures that the resulting stack trace points to the source of the problem, identifying where the bad value came from, and making debugging less painful.
• Throwing Exception
  It is almost always wrong to throw Exception. If the clients of this class are 'database aware', it may be appropriate just to remove the catch SQLException code, and allow the SQLException to be thrown out of the method. If clients need to be insulated from the fact that the Account class accesses a database, then we could transform the exception into some custom exception type which the application uses to indicate a data/resource issue.
• Use of Generics
  Since this code will run on a Java 1.5 JVM, the code that uses Collection classes should specify type arguments to take advantage of the 'generics' language feature:

      public List<Transaction> getTransactions() throws Exception{
          try{
              @SuppressWarnings("unchecked")
              List<DbRow> dbTransactionList = Db.getTransactions(accountNumber);
              
              List<Transaction> transactionList = new ArrayList<Transaction>();
              int i;
              for(i=0; i<dbTransactionList.size(); i++){
                  DbRow dbRow = dbTransactionList.get(i);
                  ...
  

• Static method for database access

  By using a static method on the Db class to perform database access, the author has made the class very hard to unit test, and has also hidden the dependency that this class has on the Db class.

  In a unit test, objects that access external resources should be replaced with stubs, however because this code uses a static method directly on the Db class the author has effectively hard-coded a reference to the real database class into the code. If the author has the freedom to change the code in the Db class then the getTransactions() method should be made non-static, and a Db instance should be passed in via the constructor (preferably) or by a setDb() method. If the Db class cannot be changed by the author, then a thin wrapper interface should be created to allow the substitution of a stub during unit testing:

  public static interface DbWrapper{
      public List getTransactions(String accountNumber);
  }
  

  private DbWrapper dbWrapper;
  
  public Account(DbWrapper dbWrapper, String accountNumber){
      this.dbWrapper     = dbWrapper;
      this.accountNumber = accountNumber;
  }
  
  public ArrayList getTransactions() throws Exception{
      try{
          List dbTransactionList = dbWrapper.getTransactions(accountNumber); 
          ArrayList transactionList = new ArrayList();
          ...                                        
  

  The default implementation of DbWrapper, used by non-test code, would look like this:

  public static class DbWrapperImpl{
      public List getTransactions(String accountNumber) throws SQLException{
          return Db.getTransactions(accountNumber);
      }
  }                    
  

  Another disadvantage of using static methods to access external resources is that the dependency on the resource is not obvious unless you read the source code (which may not be available if, for example, the class has been packaged into a JAR file). Anyone trying to use the Account class as it stands would need to know about the Db dependency, and would have to ensure that the Db class was ready for use before the getTransactions() method was called (this would probably involve configuring, and then activating, the database connection). On the other hand, if the dependency on the Db class was made explicit via a constructor argument, then it is immediately apparent that an external resource is required.

• Error handling code
  This is a really bad piece of error handling code - as well as committing the cardinal sin of throwing Exception, it also discards all the information about the original error contained in the ex object. Most Exception classes provide a constructor which allows you to specify a cause as well as a message - when an exception is transformed, as it is here, the original exception should be passed in to the constructor along with the error message, so that details of the original error are retained:

  } catch (SQLException ex){
      throw new DbException("Can't retrieve transactions from the database", ex);
  }
  

• For loop
  There are several ways to loop through a List - this is probably the worst one! We can improve things slightly by moving the int i declaration inside the for statement like so:

  for(int i=0; i<dbTransactionList.size(); i++){ 
  

  This prevents any errors that might result from inadvertant re-use of the same i variable later on in the code. A more significant improvement would be to dispense with the index variable altogether and use an Iterator:

  for(Iterator iter = dbTransactionList.iterator(); iter.hasNext(); ){
      DbRow dbRow = (DbRow)iter.next();
      ...
  

  This approach has several advantages:

  • Less error prone - no chance of using '<=' when you should use '<' for example
  • Can be used with any type of Collection, not just a List
  • Does not call the .size() method on each iteration (which may be quite inefficient for some types of List)
  • Does not retrieve elements out of the List by index, which can be very inefficient especially for a LinkedList which will always start at the beginning of the list and traverse each link until it finds the required item

  However, since we know that this code will be running on Java 1.5 JVM we can take advantage of the 'for each' construct, which has all the advantages of the Iterator approach above, and is less verbose:

  for(DbRow dbRow : dbTransactionList){
      ...
  }