Introduction

Code reviews illustrate a crying need for a skill that does not appear to have much traction in the software industry. The absence of code reading skills in schools has created armies of developers unable to comprehend simple code. And, AFAIK, no classes exist to teach this important skill in industry either.

Developers commonly spend at least 90% of their time reading code rather than writing code. Debug time gets included in reading code because searching for bugs definitely involves code reading.

This blog resulted from an email discussion. I found this interesting due to the responses I initially received while interviewing job candidates with this code.

Generally the less experienced, the fewer items noticed. However, even some developers with ten or more years of experience often missed large areas. Due to the number of candidates responding, “There is nothing wrong with this” or similar statements, I started thinking to myself, “Am I going mad?”

This Java code has been used in interviews:

   A developer has submitted the code below to be used
   as a crucial component in your product. This code will
   shortly be on your live site.
 
   Please perform a code review on the following code snippet
   that calculates the new balance based on the current balance,
   total debits, and total credits.

   This code compiles with no warnings or errors and run
   with the expected output from main().
 
============================================================

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"));
 
  }
}

After “field testing” this code on multiple java candidates, I offered this code as a discussion topic for our interview team. The responses were most pleasing.

One commenter:

These problems for sure:

1. Class Name
2. Three BigDecimal params are too easy to mess up, prefer a data
   structure to be passed in
3. No input parameter checks
4. System.out
5. No test? Whats with the main?

Well, after another few minutes of looking at it, I guess you can add:

* Missing package declaration (though I’m not sure if that counts.. I’m missing context here)

* Class is not final, not sure if it was designed that way purposely.

Of course, there are three potential NPEs which can be covered in the missing param validations/checks.

I had assumed this was part of a bigger system and negative balance/credit was ok in some context (like returns/cancels etc). (Yes IRS, please put my money back in 🙂 ) That should be part of the parameter checks/validation and be unit tested.

Me as offering this problem for discussion

Also notice that both debit and credits are assumed to be positive. If either are negative values, the result will be wonky. Wouldn’t you like to have a negative debit for taxes on your payroll? Instead of taking money out, they would be putting it back in. Is this a real problem or is it an artifact of the application? Such a striking decision should at least be noted.

main() is an incredibly poor attempt at a unit test. It should be in a standard unit test location. The various null conditions should be tested. I absolutely agree that nulls should never be passed, but this method has no context for usage so we must err on the defensive. I’ve had some candidates tell me that main() is the only way to invoke this function! They couldn’t explain how each class could have  main() and collaborate with other classes.

The println() should most likely not exist since no user context is available. If output is really desired, it should be a log with account #’s, id’s etc. to properly tag the information.

The method should be static as it neither accesses no local class vars nor does it retain a state. This will also eliminate the requirement that an instance be created before the method can be called.

The complexity of this program is low. Does that make it OK? How do you know the complexity is low?

A “getter” in Java typically returns an existing var. Using a getter to calculate looks clumsy.

I’ve had all kinds of responses:

• “You need to worry about threads.” What kind? “Well…that depends upon your application.” So how should I fix this code to be thread safe? “That depends upon things outside this routine.” (Nobody has presented a concrete reason for threading problems.)
• “Since the result gets printed on the console, you don’t need to return a value.”
• “Since the result gets printed on the console, no value will ever be returned.” (YES!)
• “main() already tests everything in this routine.”
• “This is fine the way it is.” (this is more common than you might expect)
• “If that’s what the spec wanted…it’s OK.”
• “You should catch exceptions.” What kind of exceptions?” Any that are thrown.” Where? “These operations…bad add() and subtract()”
• “It’s up to the caller to ensure that all parameters are OK.”
• “Don’t check for nulls as that is not this routine’s purpose.”

Some of these comments are from developers with many years of experience…my mind boggles.

Still another commenter

On marking it as static method:

If I were to do it, I would create a class Balance. It could possibly extend from BigDecimal (I have to check class structure for BigDecimal), so that it can be used interoperably with BigDecimal. Balance should take constructor with an initial balance and have methods such as add/debit or remove/credit or clear. Also have methods such as isNegative. Apply law of Demeter. We can discuss mutability design of the class Balance too.

This would make the client’s code look really nice and the Balance class can probably be shared across multiple applications. Unit testing would be so easy. I believe that if a static method changes the state of the object/system, maybe the method should be called on that object itself (not always).

In this case we are modifying the currentBalance then probably that method should be on the currentBalance object, which will force you to think what should be the type of currentBalance.

Once you design your basic object right, further design becomes so much more easy. If there is need for different kinds of Balances, new developer would think of extending the Balance class instead of implementing more and more static methods with code such as (if (this kind of balance) else if(that kind of balance).

For same reason using float/double/BigDecimal to represent money is not a good idea. Create a Money class. Balance can extend from Money then. So every time you see a static methods which modifies the state of an object, that is a possible symptom of we are not doing proper object oriented design.

This is so important for modular and testable code.

More commenters:

I agree that once we go towards the path of asking questions about the design we get a very different place than where this code is at.

In fact, we have a problem with static methods and cascading static method calls, making everything pretty much non-OO.

From what you have, I will debate the idea of extending from a java library class just because our business implementation uses it. We should wrap the balance BigDecimal and then provide the main functionality that acts on it.

Apart from that, right on. Once we have a CurrentBalance kind of a class, we can have one constructor that takes a BigDecimal and throws an NPE if its null. Add in the required methods.

The other option is to have this as a simple data object encapsulating the BigDecimal currentBalance and then have another class that actually manipulates this. Either way, we have more options to test and design.

This has been a good discussion already I think and probably more to come.

It’ll be nice to interview a candidate who can/will go down this discussion during the interview.

Me, as original submitter of this “code”

My original concern was that absolutely nobody even came close to identifying anything near what these discussions have brought forward. My previous list of responses were from both internal and external candidates. I can somewhat excuse recent grads as this is not taught in school and they have likely little exposure to these ideas. But how can I excuse career developers?

There is so much that can be gleaned from this simple, dumb example.

Also, notice that no one has commented on the overall architecture of this class. “Why does this stupid class exist at all? Why shouldn’t this logic reside in some accounting or payroll class?”

The lack of the “package” statement dovetails into a question of what happens without the package statement. Do they understand its significance?

Still others like this discussion

Good discussion. I like this kind of question because the premise and the example are very simple, but it can lead in all kinds of different areas in discussion with multiple layers of depth. Tying this back to interviewing technique, I think a key point:

I agree that once we go towards the path of asking questions about the design we get a very different place than where this code is at.

I think when a lot of developers (even the experienced ones) hear “Do a code review”, they default to “Run this code through the compiler in your head and make sure it compiles”. That in and of itself is a mistake, obviously, but there may be a way to present this question that encourages the candidate to think more towards the design and it wouldn’t be bad as the interviewer to make that explicit when presenting the problem. What we’re really asking is, “If you were to implement a function that totals debits and credits from an existing balance, how would your code differ from this example.”

PS. I’m surprised that no one who’s attempted to answer this question has pointed out that there are no comments in the code. 😉

Still other commenters

This is really a good discussion. This particular example, to me is a perfect interview question, as it is so simple and yet, it can tell us so much about the candidate. I like the way Nicholas framed the question, narrowing the problem, so the candidate can concentrate only on this aspect. If he/she can find these problems and come up with a good solution (class/method), the next level would be design questions. I suppose we can have a simple use case, which adds more requirements and see how the developer changes its implementation/design. The requirements out of this use case can be asked incrementally after the candidate answer the previous one, or all of them up-front to see how he/she can solve more complex problem directly.

Some more notes on my end:

I completely agree that static methods breaks OOP. In general I consider them more harmful than helpful. They couple this concrete class with other concrete classes(they cannot be abstract as the concrete instance has to be created within the class itself). We give up control on when this instance is going to be instantiated and how it is going to be initialized(when: at first use of the class and when exactly is that? ), (how: well, there is only one way as all the instances has to be available). It also forces us to use instances of other static classes, as otherwise our instance cannot be initialized properly. So, not only they have bad implications on the current class, but can also force bad design on other classes as well. I suppose this can be a good topic to discuss in general.

I also believe that naming of (classes, methods and variables) is really important considerations as part of this example. Method like: getCalculatedAvailableBalance() means to me, the balance is already pre-computed, may be persisted and available somewhere.

I like how this group picked up all the main issues right away.

Another commenter

In theory, it is possible to enter with the following method with one set of inputs (say, 1000, 250, 250), and exit with an output not equal to 1000. That’s possible if the caller shot themselves in the foot by having several caller threads muck up the arguments willy-nilly, during the course of the daisy-chained computation below.

So a couple of observations here: Make defensive copies of the arguments before they are subject to a long sequence of calculations. And two, make the sequence of calculations as atomic as possible, to aid debugging in a multi-threading environment.

Alas, both suggestions are moot here. BigDecimal is immutablee, thereby obviating the need to make defensive copies.

Another commenter

In essence, you are right about defensive coding and preparing for a multi-threaded access.

Though:

* As it is, I don’t see a threading problem in this code.

* BigDecimal is documented as immutable, but alas, you can, in theory extend from BigDecimal (its not final 🙂 ) and do some weird stuff with it.

To get one though, all you need to do is add another variable of your own and do something to it in a public static method without any thread-safety considerations.

Then.. you can go find the person who did this and find out why!

Of course, the library provided variables in BigDecimal and methods are all safe, so it will take some extraordinary effort to screw this up.. its possible though.

Maybe if the candidate has all/most of this down pat, you can ask him how to mess this up for brownie points?

Another commenter

That’s a timely reminder (to me, especially) that BigDecimal, while calling itself immutable,isn’t truly so, for all it’s methods can be overridden by a (malicious) subclass.

I’ll copy-paste the relevant part from Effective Java, for I cannot put it better:

“If you write a class whose security depends on the immutability of a BigDecimal argument from an untrusted client, you must check to see that the argument is a ‘real’ BigDecimal, rather than an instance of an untrusted class.”

If the interview candidate can get until here, then he’s doing ok.