Codebox Software
An Interview Question for Java Developers
Published:
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
anddouble
) 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 anint
and store the amount as pence, or (if we need to handle fractions of pence) use theBigDecimal
class. -
Overriding equals method
This method does not actually override the
equals
method inherited from theObject
class, it overloads it, which is not what the author intended (the 'o
' parameter should have typeObject
notAccount
). This means that this customequals
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 customhashCode
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 anAccount
instance withnull
this method will throw aNullPointerException
, rather than returningfalse
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 theequals
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 thanpublic
. -
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 (egtransactionList.get(0)
) thenList
is a good choice. If there is no such requirement thenCollection
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 fornull
values would be especially valuable:public Account(String accountNumber){ if (accountNumber == null){ throw new NullPointerException("accountNumber argument was null"); } this.accountNumber = accountNumber; }
By throwing theNullPointerException
explicity in the constructor, rather than waiting for it to happen later on (for example when.trim()
is called in thegetTransactions()
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 thecatch SQLException
code, and allow theSQLException
to be thrown out of the method. If clients need to be insulated from the fact that theAccount
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 theDb
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 theDb
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 theDb
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 theDb
class then thegetTransactions()
method should be made non-static, and aDb
instance should be passed in via the constructor (preferably) or by asetDb()
method. If theDb
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 ofDbWrapper
, 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 theDb
dependency, and would have to ensure that theDb
class was ready for use before thegetTransactions()
method was called (this would probably involve configuring, and then activating, the database connection). On the other hand, if the dependency on theDb
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 theex
object. MostException
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 theint i
declaration inside thefor
statement like so:for(int i=0; i<dbTransactionList.size(); i++){
This prevents any errors that might result from inadvertant re-use of the samei
variable later on in the code. A more significant improvement would be to dispense with the index variable altogether and use anIterator
: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 aList
- Does not call the
.size()
method on each iteration (which may be quite inefficient for some types ofList
) - Does not retrieve elements out of the
List
by index, which can be very inefficient especially for aLinkedList
which will always start at the beginning of the list and traverse each link until it finds the required item
for(DbRow dbRow : dbTransactionList){ ... }
- Less error prone - no chance of using '