Search | Blog | Contact

Another Refactoring Exercise: Design Patterns Recommended!-) 41

Posted by Brett Schuchert Wed, 10 Jun 2009 01:57:00 GMT

Well the previous exercise was fun so here’s another one. The following code is taken from the same problem, an RPN calculator. Originally, the interface of the calculator was “wide”; there was a method for each operator. E.g., plus(), minus(), factorial(). In an effort to fix this, a new method, perform(String operatorName) was added and ultimately the interface was fixed gradually to remove those methods.

Changing he calculator API in this way is an example of applying the open/closed principle. However, the resulting code is just a touch ugly (I made it a little extra ugly just for the hack [sic] of it). This code as written does pass all of my unit tests.

Before the code, however, let me give you a little additional information:
  • I changed the calculator to use BigDecimal instead of int
  • Right now the calculator has three operators, +, – !
  • Eventually, there will be many operators (50 ish)
  • Right now there are only binary and unary operators, however there will be other kinds: ternary, quaternary, and others such as sum the stack and replace just the sum on the stack or calculate the prime factors of the top of the stack so take one value but push many values

So have a look at the following code and then either suggest changes or provide something better. There’s a lot that can be done to this code to make it clearer and make the system easier to extend.

The Perform Method

   public void perform(String operatorName) {
      BigDecimal op1 = stack.pop();

      if ("+".equals(operatorName)) {
         BigDecimal op2 = stack.pop();
         stack.push(op1.add(op2));
         currentMode = Mode.inserting;
      } else if ("-".equals(operatorName)) {
         BigDecimal op2 = stack.pop();
         stack.push(op2.subtract(op1));
         currentMode = Mode.inserting;
      } else if ("!".equals(operatorName)) {
         op1 = op1.round(MathContext.UNLIMITED);
         BigDecimal result = BigDecimal.ONE;
         while (op1.compareTo(BigDecimal.ONE) > 0) {
            result = result.multiply(op1);
            op1 = op1.subtract(BigDecimal.ONE);
         }
         stack.push(result);
      } else {
         throw new MathOperatorNotFoundException();
      }
   }

Unlike the last example, I’ll provide the entire class. Feel free to make changes to this class as well. However, for now focus on the perform(...) method.

One note, Philip Schwarz recommended a change to what I proposed to avoid the command/query separation violation. I applied his recommendation before posting this updated version.

The Whole Class

package com.scrippsnetworks.calculator;

import java.math.BigDecimal;
import java.math.MathContext;

public class RpnCalculator {
   private OperandStack stack = new OperandStack();
   private Mode currentMode = Mode.accumulating;

   enum Mode {
      accumulating, replacing, inserting
   };

   public RpnCalculator() {
   }

   public void take(BigDecimal value) {
      if (currentMode == Mode.accumulating)
         value = determineNewTop(stack.pop(), value);

      if (currentMode == Mode.replacing)
         stack.pop();

      stack.push(value);
      currentMode = Mode.accumulating;
   }

   private BigDecimal determineNewTop(BigDecimal currentTop, BigDecimal value) {
      BigDecimal newTopValue = currentTop;
      String digits = value.toString();
      while (digits.length() > 0) {
         newTopValue = newTopValue.multiply(BigDecimal.TEN);
         newTopValue = newTopValue.add(new BigDecimal(Integer.parseInt(digits
               .substring(0, 1))));
         digits = digits.substring(1);
      }

      return newTopValue;
   }

   public void enter() {
      stack.dup();
      currentMode = Mode.replacing;
   }

   public void perform(String operatorName) {
      BigDecimal op1 = stack.pop();

      if ("+".equals(operatorName)) {
         BigDecimal op2 = stack.pop();
         stack.push(op1.add(op2));
         currentMode = Mode.inserting;
      } else if ("-".equals(operatorName)) {
         BigDecimal op2 = stack.pop();
         stack.push(op2.subtract(op1));
         currentMode = Mode.inserting;
      } else if ("!".equals(operatorName)) {
         op1 = op1.round(MathContext.UNLIMITED);

         BigDecimal result = BigDecimal.ONE;
         while (op1.compareTo(BigDecimal.ONE) > 0) {
            result = result.multiply(op1);
            op1 = op1.subtract(BigDecimal.ONE);
         }
         stack.push(result);
      } else {
         throw new MathOperatorNotFoundException();
      }
   }

   public BigDecimal getX() {
      return stack.x();
   }

   public BigDecimal getY() {
      return stack.y();
   }

   public BigDecimal getZ() {
      return stack.z();
   }

   public BigDecimal getT() {
      return stack.t();
   }
}