Spreadsheet with reverse Polish equation solving

Interested to hear any suggestions on object orientation and style for this simple spreadsheet implementation. The spreadsheet contains cells with equations in reverse polish notation and has a solver method to either determine the cell values or throw an exception if a circular reference exists.

```
/**
* This class encapsulates a spreadsheet which is a collection of cells together
* with methods for solving the spreadsheet.
*/
public class SpreadSheet {

private int nRows; // number of rows in the spreadsheet
private int nCols; // number of columns, must be less than or equal to 26
private ArrayList cells; // the cells within the spreadsheet in row by row
// order

/**
* Construct a spreadsheet from a given expression array.
*
* @param nRows
* number of rows in spreadsheet.
* @param nCols
* number of columns in spreadsheet.
* @param exprArray
* an array of Strings containing the expressions for the cells
* of the spreadsheet specified in row by row order.
*/
public SpreadSheet(int nRows, int nCols, String... exprArray) {
this.nRows = nRows;
this.nCols = nCols;
cells = new ArrayList(exprArray.length);
for (String expressionString : exprArray) {
cells.add(new Cell(this, expressionString));
}
}

/**
* Solve a spreadsheet and return the solution of each cell.
*
* @return array of doubles containing the solution to each cell in row by
* row order.
* @throws CircularReferenceException if spreadsheet contains a circular reference.
*/
public Double[] dump() throws CircularReferenceException {
int validCells = 0; // cells with valid values
int validCellsPreviousIteration = 0; // cells with valid values in the
// previous iteration
try {
while (val

-

private String expression; // expression within cell in reverse Polish
                            // notation
private double value; // numerical value of evaluating expression

I'd rename it to rpnExpression and evaluatedValue which will make the comments unnecessary. (Check Clean Code by Robert C. Martin, page 53-54 also.)

-

public Cell(SpreadSheet spreadsheet, String expression) {
    this.spreadsheet = spreadsheet;
    this.expression = expression;
}

I'd check nulls here. checkNotNull from Guava is a gread choice for that.
(Effective Java, 2nd edition, Item 38: Check parameters for validity)

-

if (!valueValid) { // prevent reevaluation of cells that have valid
                    // values

This condition is a good candidate for guard clause.
References: Replace Nested Conditional with Guard Clauses in Refactoring: Improving the Design of Existing Code; Flattening Arrow Code

-

throw new InvalidValueException();

I'd put into the message the invalid expression.

-

* @throws InvalidOperatorException
 * @throws UnresolvedReferenceException

Are these lines necessary?

-

if (isCellReference(term)) {
    // if last term in expression is a cell reference then resolve it
    return resolveCellReference(term);
} else {
    double x, y;
    try {
        // if last term in expression is double then return it
        x = Double.parseDouble(term);
    } catch (NumberFormatException e) {
        // otherwise last term is an operator, evaluate operands and
        // apply operator
        y = evaluateRpn(expressionStack);
        x = evaluateRpn(expressionStack);
        x = applyOperator(x, y, term);
    }
    return x;
}

The isCellReference is a guard clause, so the else keyword and its parentheses are unnecessary:

if (isCellReference(term)) {
    // if last term in expression is a cell reference then resolve it
    return resolveCellReference(term);
}
double x, y;
try {
    // if last term in expression is double then return it
    x = Double.parseDouble(term);
} catch (NumberFormatException e) {
    // otherwise last term is an operator, evaluate operands and
    // apply operator
    y = evaluateRpn(expressionStack);
    x = evaluateRpn(expressionStack);
    x = applyOperator(x, y, term);
}
return x;

-
Furthermore, I'd minimize the scope of the local variables:

try {
    return Double.parseDouble(term);
} catch (NumberFormatException e) {
    // otherwise last term is an operator, evaluate operands and
    // apply operator
    double y = evaluateRpn(expressionStack);
    double x = evaluateRpn(expressionStack);
    return applyOperator(x, y, term);
}
return x;

(Effective Java, 2nd edition, Item 45: Minimize the scope of local variables)

-

if (operator.equals("+"))

I prefer to invert the equals here:

if ("+".equals(operator))

It protects from NullPointerExceptions when the operator parameter is null.

-

} catch (InvalidValueException e) {
    throw new UnresolvedReferenceException();
}

Usually it's a good practice to pass the cause to constructor of the new exception:

throw new UnresolvedReferenceException(e);

-

public InvalidOperatorException(String operator) {
    this.operator = operator;
}

public String toString() {
    return "Invalid operator " + operator;
}

I'd pass the "Invalid operator " + operator string to the constructor of the superclass (as the message) and omit the operator field:

public InvalidOperatorException(String operator) {
    super("Invalid operator " + operator);
}

It's more common, and logging libraries will print the class name too, not just the string, which helps debugging.

StackExchange Code Review Q#8889, answer score: 5