This assumes a fair bit about how much we can change dependent classes, but assuming that Mode is specific to whatever take(int) is a member of, I’d go with something like this:

public enum Mode {
    ACCUMULATING {
        @Override
        public Mode take(Stack stack, int v) {
            int digits = (int)Math.pow(10, (int)Math.log10(v) + 1);
            int x = stack.pop();
            x *= digits;
            x += v;
            stack.push(x);
            return ACCUMULATING;
        };
    },
    REPLACING {
        @Override
        public Mode take(Stack stack, int v) {
            stack.pop();
            stack.push(v);
            return ACCUMULATING;
        };
    },

    INSERTING {
        @Override
        public Mode take(Stack stack, int v) {
            int top = stack.peek();
            stack.push(top);
            stack.push(v);
            return ACCUMULATING;
        };
    }

    public abstract Mode take(Stack stack, int v);
}

// ... meanwhile, back at the ranch ...

public void take(int v) {
    currentMode = currentMode.take(stack, v);
}

So Mode becomes a strategy and not a state.

My initial refactor assumed that the Modes were actually objects, not ints from an enum:

public void take(int v) {
  take(currentMode, v)
  currentMode = Mode.accumulating;
}

private void take(AccumulatingMode mode, int v)
{
      int digits = (int)Math.pow(10, (int)Math.log10(v) + 1);
      int x = stack.pop();
      x *= digits;
      x += v;
      stack.push(x);
}

private void take(ReplacingMode mode, int v)
{
        stack.pop();
        stack.push(v);
}

private void take(InsertingMode mode, int v)
{
        int top = stack.peek();
        stack.push(top);
        stack.push(v);
}

I see some other things, but I’m interested to see what others have. I’m sure some of the stack operations would be clearer in a different language.

My Ruby solution would look like this:

class Fixnum
  def shiftValue
    10 ** (Math.log10(self.to_f) + 1.0).to_i
  end
end

# ... in _THE_ class ...
def take(v)
  newMode = self.send @currentMode, v
  if self.respond_to?(newMode, true) then
    @currentMode = newMode
  else
    raise NoMethodError,
      "The state '#{@currentMode}' has violated the contract" 
  end
  nil
end

def modeAccumulate(v)
  @stack.push(@stack.pop() * v.shiftValue + v)
  :modeAccumulate
end

def modeReplace(v)
  @stack.pop()
  @stack.push(v)
  :modeAccumulate
end

def modeInsert(v)
  @stack.push(@stack.last)
  @stack.push(v)
  :modeAccumulate
end

Instead of throwing an exception I could just fallback to a default state. But if someone extends this class it could be interessting to know which method returns an invalid state/method. I would still prefer compile-time errors :-\

After adding some behavior describing tests I refactored to a strategy with some influences from the state pattern. Mostly I was influenced by Refactoring to Patterns on this. On a second note I don’t like the mess I ended up with.

  Stack stack = new Stack();

  CalculationMode strategy = new InsertionMode();

  public void take(int v) {
    strategy.take(this, v);
  }

}

abstract class CalculationMode {

  abstract void take(Challenge object, int v);
}

class InsertionMode extends CalculationMode {

  @Override
  void take(Challenge object, int v) {
    int top = object.stack.peek();
    object.stack.push(top);
    object.stack.push(v);
    object.strategy = new AccumulationMode();
  }
}

class ReplacementMode extends CalculationMode {

  @Override
  void take(Challenge object, int v) {
    object.stack.pop();
    object.stack.push(v);
    object.strategy = new AccumulationMode();
  }

}

class AccumulationMode extends CalculationMode {

  @Override
  void take(Challenge object, int v) {
    int digits = (int) Math.pow(10, (int) Math.log10(v) + 1);
    int x = object.stack.pop();
    x *= digits;
    x += v;
    object.stack.push(x);
  }

This is C# Refactoring with a focus on readability.

In any event Take itself seems odd in the sense that it does several things (handle v and change the mode) and its name does not say anything on what it does. So there’s probably a need to refactor the stack to remove the take method altogether. It is hard to understand without the full context so I didn’t try to come up with something “smart” instead of the case/if statements

        public void Take(int v)
        {
            switch (CurrentMode)
            {
                case Mode.accumulating:
                    Accumulate(v);
                    break;
                case Mode.replacing:
                    Replace(v);
                    break;
                case Mode.inserting:
                    Insert(v);
                    break;
                default:
                    throw new ArgumentOutOfRangeException("Invalid mode");
            }
           CurrentMode = Mode.accumulating;

        }
       private void Replace(int v)
        {
            TheStack.Pop();
            TheStack.Push(v);
        }

        private void Insert(int v)
        {
            int top = TheStack.Peek();
            TheStack.Push(top); //why another copy of top?
            TheStack.Push(v);
        }

        private void Accumulate(int v)
        {
            int current = TheStack.Pop();
            current.Shift(v.NumberOfDigits());
            TheStack.Push(current);
        }

Shif and NumberOfDigits are defined as extension methods:
    static class IntExtensions
    {
        public int NumberOfDigits(this int v)
        {
            return (int)Math.Pow(10, (int)Math.Log10(v));
        }
        public int Shift(this int number, int numberOfDigits)
        {
            return number*(numberOfDigits*10);
        }
    }

My first thought is that if you’re in accumulating mode then the value on the stack is going to overflow pretty quickly, since “int” isn’t very big, and you’re multiply by powers of ten. Related to that, if the value passed in is negative then you’ll get an error on the log10 call.

Anyway, here’s some C++ code:

class X
{
    typedef void (X::*take_function)(int v);

    take_function current_mode;
    std::stack<int> stack;

    static int find_next_power_of_ten(int v)
    {
        return (int)pow(10, (int)log10(v) + 1);
    }

    void take_accumulating(int v)
    {
        int x = stack.top();
        x *= find_next_power_of_ten(v);
        x += v;
        stack.top()=x;
    }

    void take_replacing(int v)
    {
        stack.top()=v;
    }

    void take_inserting(int v)
    {
        stack.push(stack.top());
        stack.push(v);
    }

public:
    void take(int v)
    {
        (this->*current_mode)(v);
        current_mode=&X::take_accumulating;
    }

};

Here is a scala version. Structure looks pretty much like ruby but uses pattern matching.

class StackOps {

def take(v: int) : Unit = {
  currentMode match {
    case Mode.accumulating => accumulate(v)
    case Mode.replacing => replace(v)
    case Mode.inserting => insert(v)
    case _ => currentMode
  }
}

def accumulate(v: int): Unit = {
    val log10: int = Math.log10(v).instanceof[int]
    val digits: int = Math.pow(10, log10 + 1).instanceof[int]
    var x: int = stack.pop()
    x *= digits
    x += v
    stack.push(x)
}

def replace(v: int): Unit = {
    stack.pop()
    stack.push(v)
    currentMode = Mode.accumulating
}

def insert(v: int): Unit = {
    stack.push(stack.peek())
    stack.push(v)
    currentMode = Mode.accumulating
}

}

I like what I’m seeing so far. By the way, this is the beginnings of a method to handle input for an RPN calculator. Input is more complex than I first imagined:

• Initially digits are accumulating.
• After pressing enter, the next digit will replace the top of the stack (the x register) because pressing enter duplicated the current top.
• After executing an operator, e.g., + – / * !, typing a digit duplicates the top of the stack (while another operator uses what’s there).

Of course I have tests for this.

Anthony makes a good point regarding handling negative numbers. In practice, the take method (as in take a digit – deliberately a bit misleading for the exercise) could just as easily use a character or verify that the value coming in is a value between 0 – 9. I left it as is to accommodate different kinds of clients, but that’s probably better reserved for the UI (digit accumulation requirements would be different for a GUI versus say a text UI) – it’s probably a bad assignment of responsibility (SRP violation maybe) and certainly could be handled with an adapter of some kind (or the control of an MVC).

Anyway, I’ll keep watching (though I haven’t got anything better than what I’ve already seen!-)

Here’s a C# refactoring with an eye searing dispatch map rather than switch.

protected State _state;
        protected class State
        {
            public Stack<int> Stack { get; set; }
            public Mode Mode { get; set; }
        }

        protected IDictionary<Mode, Action<int,State>> _dispatch = new Dictionary<Mode, Action<int,State>>
                        {
                            {Mode.accumulating,Accumulating},
                            {Mode.replacing,Replacing},
                            {Mode.inserting,Inserting} 
                        };

        public void Take(int v)
        {
            _dispatch[_state.Mode](v,_state);
        }

        protected static void Accumulating(int v, State state)
        {
            var digits = (int)Math.Pow(10, (int)Math.Log10(v) + 1);
            var x = state.Stack.Pop();
            x *= digits;
            x += v;
            state.Stack.Push(x);
        }

        protected static void Replacing(int v, State state)
        {
            state.Stack.Pop();
            state.Stack.Push(v);
            state.Mode = Mode.accumulating;
        }

        protected static void Inserting(int v, State state)
        {
            var top = state.Stack.Peek();
            state.Stack.Push(top);
            state.Stack.Push(v);
            state.Mode = Mode.accumulating;
        }

Here is mine. Mode class is a State.

public class Exercise {

    private Mode currentMode;
    private Stack stack;

    public Exercise(Mode currentMode, Stack stack) {
        super();
        this.currentMode = currentMode;
        this.stack = stack;
    }

    public void take(int v) {
        currentMode = currentMode.take(stack, v);
      }
}

interface Mode {

    public Mode take(Stack stack,int v);

    public static Mode accumulating = new Mode() {

        @Override
        public Mode take(Stack stack, int v) {
            int digits = (int)Math.pow(10, (int)Math.log10(v) + 1);
              int x = stack.pop();
              x *= digits;
              x += v;
              stack.push(x);
             return Mode.accumulating;
        }

    };

    public static Mode replacing = new Mode() {

        @Override
        public Mode take(Stack stack, int v) {
            stack.pop();
            stack.push(v);
            return Mode.accumulating;
        }
    };

    public static Mode inserting = new Mode() {
        @Override
        public Mode take(Stack stack, int v) {
            int top = stack.peek();
              stack.push(top);
              stack.push(v);
              return Mode.accumulating;
        }
    };
}

Submission is slow. Progress bar needed slow

Based on your comments, I’d take a look at your calculation, the log stuff boils down to a base ten shift, which makes sense for handling a keystroke, but then you multiply with it? I’ve redone the calc to expose it a bit better. .

  protected void accumulate(int v) {
      int newV = calculate( v, stack.peek());
      replace( newV );
  }
  protected int calculate(int v, int x) {
      // should be (x * 10) + v ?
      return (x*x)*10 + v;
  }
  protected void replace(int v) {
      stack.pop();
      stack.push(v);
  }
  protected void insert(int v) {
      int top = stack.peek();
      stack.push(top);
      stack.push(v);
  }
  public void take(int v) {
    if (currentMode == Mode.accumulating) {
      accumulate(v);
    }
    else if (currentMode == Mode.replacing) {
      replace(v);
    }
    else if (currentMode == Mode.inserting) {
      insert(v);
    }
    currentMode = Mode.accumulating;
  }

During my evaluation of the algorithm by bringing in some tests to describe for myself what happens, I realized that the calculation in accumulating mode does the following: - take the top of the stack and assign it to x - calculate the number of digits for the given parameter v - multiply x with the number of digits - add v

Describing this with some tests leads to: x=5, v=5—> 55 x=4, v=23—> 423 x=589, v=1243—> 5891243

The calculation from Brian seems to wrong on this, I think.

Using extension methods from .Net 3.5 makes life a little bit more easier and intentional:

public class ClassThatTakes
{
    private Mode currentMode;
    private Stack<int> stack;
    private int x;

    public void take(int v)
    {
        switch (currentMode)
        {
            case Mode.accumulating:
                stack.UpdateTopElement(x => 10 * v * x + v);
                break;
            case Mode.replacing:
                stack.ReplaceTopElementBy(v);
                currentMode = Mode.accumulating;
                break;
            case Mode.inserting:
                stack.RepeatTopElement();
                stack.Push(v);
                currentMode = Mode.accumulating;
                break;
        }
    }
}

internal static class StackExtensions
{
    public static void RepeatTopElement(this Stack<int> stack)
    {
        int top = stack.Peek();
        stack.Push(top);
    }

    public static void ReplaceTopElementBy(this Stack<int> stack, int v)
    {
        stack.Pop();
        stack.Push(v);
    }

    public static void UpdateTopElement(this Stack<int> stack, Func<int, int> func)
    {
        int x = stack.Pop();
        int result = func(x);
        stack.Push(result);
    }
}

My mistake. I misread the log line as operating on x instead of v.

So, we want

  int lengthOfInput = (int)Math.LogBase10(v) + 1;
  int shiftedOriginalInput = x * Math.Pow(10, lengthOfInput);
  int accumulatedValue = shiftedOriginalInput + v;

Which does boil down to the original algorithm. I’d like to amend my answer to include the expanded algorithm, since I got confused with the more concise original version.

Note to self: Never trust excel to do integer math. I missed a trunc() the first time around.

I agree with Markus that Brian’s calculation looks wrong. But so far his is what I would consider to be the cleanest solution. It is very straight forward to figure out what is going on.

Some of the others, such as Coda Hale’s approach of embedding the logic into the enumeration is quite a cool idea. But then having to pass the stack into the enumeration seems a little backwards to me.

Likewise with Cory Foy’s overloaded methods, to me this adds confusion. I’m stupid and I want the code to point me at exactly what is happening, not that it makes me have to figure out which method is being called.

I started off following Brian’s approach, but then as I said I’m stupid and couldn’t think out a neat way of coding the calculation. On idea went as far as:

    void takeAccumulating(int v) {
        StringBuilder xString = new StringBuilder();
        xString.append(stack.pop());
        xString.append(v);
        int x = Integer.valueOf(xString.toString());
        stack.push(x);
    }

But that doesn’t deal with negative numbers as was also pointed out…

Yet another in C#:

class RPNCalculator
  {
    private Mode currentMode;
    private Stack<int> stack = new Stack<int>();

    public RPNCalculator()
    {
      currentMode = new AccumulatingMode(stack);
    }

    public void take(int v)
    {
      currentMode = currentMode.take(v);
    }
  }

  abstract class Mode
  {
    protected Stack<int> stack;

    public Mode(Stack<int> stack)
    {
      this.stack = stack;
    }

    public abstract Mode take(int v);
  }

  class AccumulatingMode : Mode
  {
    public AccumulatingMode(Stack<int> stack)
      : base(stack)
    {
    }

    public override Mode take(int v)
    {
      int digits = (int)Math.Pow(10, (int)Math.Log10(v) + 1);
      int x = stack.Pop();
      x *= digits;
      x += v;
      stack.Push(x);

      return this;
    }
  }

  class ReplacingMode : Mode
  {
    public ReplacingMode(Stack<int> stack)
      : base(stack)
    {
    }

    public override Mode take(int v)
    {
      stack.Pop();
      stack.Push(v);

      return new AccumulatingMode(stack);
    }
  }

  class InsertingMode : Mode
  {
    public InsertingMode(Stack<int> stack)
      : base(stack)
    {
    }

    public override Mode take(int v)
    {
      int top = stack.Peek();
      stack.Push(top);
      stack.Push(v);

      return new AccumulatingMode(stack);
    }
  }

The obligatory Haskell solution:

1. rewrote it in Haskell (obviously)
2. renamed “take” to “push”
3. function is now side-effect free
4. algorithm now depends on stack type
5. stack is immutable
6. I don’t know if insertion is meant to leave the existing top cloned, but I’ve kept that

data Stack = AccumulatingStack [Integer]                                                   
          | ReplacingStack [Integer]                                                       
          | InsertingStack [Integer]                                                       
          deriving (Show)                                                                  

push :: Integer -> Stack -> Stack                                                          
push v (AccumulatingStack (x:xs)) = AccumulatingStack (top:xs)                             
  where top = v + x * digits                                                               
        digits = 10 ^ (1 + floor (logBase 10 $ fromInteger v))                             
push v (ReplacingStack (_:xs)) = AccumulatingStack (v:xs)                                  
push v (InsertingStack (x:xs)) = AccumulatingStack (v:x:x:xs) 

The math for calculating the magnitude was incorrect. I think this one works. I actually toyed with

Math.pow(10, Integer.toString(v).length())

for awhile, but decided that the loop was a better approach.

package brettsChallenge;

import java.util.Stack;

public class Register {
  private Mode mode;
  private Stack<Integer> stack = new Stack<Integer>();

  public Register() {
    accumulate();
    stack.push(0);
  }

  void dup() {
    stack.push(stack.peek());
  }

  void enter(int v) {
    stack.push(v);
  }

  void replace(int v) {
    stack.pop();
    stack.push(v);
  }

  int top() {
    return stack.peek();
  }

  public void take(int v) {
    mode.take(this, v);
  }

  public Stack<Integer> getStack() {
    return stack;
  }

  public void insert() {
    mode = Mode.inserting;
  }

  public void replace() {
    mode = Mode.replacing;
  }

  void accumulate() {
    mode = Mode.accumulating;
  }

  enum Mode {
    inserting {
      public void take(Register r, int v) {
        r.dup();
        r.enter(v);
        r.accumulate();
      }
    },
    replacing {
      public void take(Register r, int v) {
        r.replace(v);
        r.accumulate();
      }
    },
    accumulating {
      public void take(Register r, int v) {
        r.replace(r.top() * magnitude(v) + v);
      }
    };

    private static int magnitude(int v) {
      if (v == 0)
        return 10;
      int magnitude = 1;
      for (; v > 0; v /= 10)
        magnitude *= 10;
      return magnitude;
    }

    public abstract void take(Register r, int v);
  }
}

Uncle Bob is right. While I did write unit tests first, I was think about the state and not the digit accumulation. I did test that, but I missed an equivalence class. Turns out, the log base 10 of 0 is not a number. The (int) of that is -2 billion and the pow of that is 0 after another cast. I wanted a mess!

On of the .Net solutions used the length of the number, removing that bug. Bob asked me bout and sure enough a quick test ;which he had already created) exposed the problem.

I’m at the airport now wanting to write more buy the iPhone is saying otherwise.

Great stuff so far!

With a pattern. Still ugly though, me thinks, but that’s just how the language rolls…

public class BrettSmileNicely {
    private String currentMode;
    private MyStack stack;

    public void take(int value) {
        currentStacker().actOn(value);
    }

    private Stacker currentStacker() {
        return stackers.get(currentMode);
    }

    private Map<String, Stacker> stackers = new HashMap<String, Stacker>(){{
        put(Mode.accumulating, new Accumulater());
        put(Mode.replacing, new Replacer());
        put(Mode.inserting, new Inserter());
    }};

    private abstract class Stacker {
        public abstract void actOn(int value);
        protected void accumulate() {
            currentMode = Mode.accumulating;
        }
    } 

    private class Inserter extends Stacker {
        @Override
        public void actOn(int value) {
            insert(value);
            accumulate();            
        }

        private void insert(int value) {
            stack.push(stack.peek());
            stack.push(value);
        }
    }

    private class Replacer extends Stacker {
        @Override
        public void actOn(int value) {
            swapTopWith(value);
            accumulate();
        }

        private void swapTopWith(int value) {
            stack.pop();
            stack.push(value);
        }
    }

    private class Accumulater extends Stacker {
        @Override
        public void actOn(int value) {
            stack.push(topAdjustedBy(value));
        }

        private int topAdjustedBy(int value) {
            int topValue = stack.pop();
            topValue *= magnitude(value);
            topValue += value;
            return topValue;
        }

        private int magnitude(int value) {
            return (int)Math.pow(10, (int)Math.log10(value) + 1);
        }
    }
}

I didn’t really understand the accumulation calculation, so I had to add a few tests to make sure I didn’t break it. Then it was amazing to watch the duplications appear as I separated the queries from the commands:

public void Take(int v)
{
    var top = stack.Pop();
    if (currentMode == Mode.Inserting)
    {
        stack.Push(top);
        stack.Push(top);
    }
    stack.Push(CalculatedValue(v, top));
    currentMode = Mode.Accumulating;
}

private int CalculatedValue(int v, int top)
{
    if (currentMode == Mode.Accumulating)
    {
        var digits = (int) Math.Pow(10, (int) Math.Log10(v) + 1);
        return top*digits + v;
    }
    return v;
}

oh, good god, allow me to revisit that….

public void addNumberToStack(int number)
  {
    switch (currentMode)
    {
      case accumulating :
        stack.push(new Integer(stack.pop() + "" + number));
      case replacing :
        stack.set(stack.size() - 1, number);
      case inserting :
        stack.add(stack.size() - 1, number);
    }
    currentMode = Mode.accumulating;
  }

I’d like to play devils advocate and say that the method doesn’t need refactoring, since it is only 13 statements and not hard to understand.

Why should you refactor this method? Note that some of the current refactorings come out to more than 13 statements and don’t necessarily add anymore clarity.

However, this is an exercise so lets assume we have a good reasons to refactor.

For me the code has two responsibilities in one place; the responsibility of the calculation and the responsibility of managing the stack. I’d like to see these concerns separated.

I think Cory Foy’s refactoring is a good start, but I’d change the operations on stack to be methods on the stack. ie: stack.replace(v).

I’d also go further and suggest the following, given I like functors:

  public void take(final int v) {

    if (currentMode == Mode.accumulating) {
        stack.push(stack.withTopDo(calculationFunctor(v)));
    }

    if (currentMode == Mode.replacing) {
        stack.replaceTopWith(v);     
    }

    if (currentMode == Mode.inserting) {
        stack.insert(v);
    }

    currentMode = Mode.accumulating;
  }

Resulting in 4 statements and a flow that is easy to see.

Well let me say I like these ideas.

To those of you refactoring to state/strategy, well done. A little painful, huh? (I think it’s closer to state than strategy, but then they are similar patterns).

Those of you just extracting methods to make it cleaner, good idea. Expressing the intent is a great idea.

I give Anthony +10 points for using pointers to members. And Christian, thanks for the Haskel. I was hoping for an example from that language.

Also, I think James makes a valid point questioning the need to refactor the code in the first place. If this code was already working and not changing, then no need to refactor. Of course, it should never have gotten that ugly in the first place!

So I made some changes per uncle bob’s observation that the calculation was wrong. This did not essentially change my refactoring.

Through some experimentation, I accidentally came up with the following (note that I do nothing, which is special, for inserting mode now):

  public void take(int value) {
    if (currentMode == Mode.accumulating) 
      value = determineNewTop(value);

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

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

Then I extracted the calculation and used a looking construct:

  private int determineNewTop(int value) {
    int newTopValue = stack.pop();

    String digits = Integer.toString(value);
    while(digits.length() > 0) {
      newTopValue *= 10;
      newTopValue += Integer.parseInt(digits.substring(0,1));
      digits = digits.substring(1);
    }

    return newTopValue;
  }

I certainly like the take method better. I’m not too keen on reassigning to the value parameter (note the change name refactoring), so that’s at least a little dubious. Also, the determineNewTop method both changes the stack (with a pop) and returns a value. Violates command/query separation, but it is also a private method, so I’m not as concerned about that violation.

I like this better. I wanted to use the state pattern, but as the early solutions demonstrate, that makes for a complex result.

FWIW, I also had some FitNesse tests (thanks to Bob Koss):

|Calculator             |
|user enters|x?|y?|z?|t?|
|6          |6 |0 |0 |0 |
|enter      |6 |6 |0 |0 |
|5          |5 |6 |0 |0 |
|+          |11|0 |0 |0 |

|Calculator             |
|user enters|x?|y?|z?|t?|
|3          |3 |0 |0 |0 |
|6          |36|0 |0 |0 |
|enter      |36|36|0 |0 |
|5          |5 |36|0 |0 |
|+          |41|0 |0 |0 |

|Calculator             |
|user enters|lhs? |rhs? |
|123        |0    |123  |
|+          |0    |123  |
|-          |0    |-123 |
|-          |0    |123 ||
|21         |123  |21   |
|98         |123  |2198 |
|-          |0    |-2075|
|-2070      |-2075|-2070|
|-          |0    |-5   |
|-          |0    |5    |
|!          |0    |120  |

And here’s the Decision Table Fixture to handle this:

public class Calculator {
  RpnCalculator calculator = new RpnCalculator();

  public void setUserEnters(String value) {
    if ("enter".equals(value))
      calculator.enter();
    else if (value.matches("[+-]{0,1}\\d+"))
      calculator.take(Integer.parseInt(value));
    else
      calculator.perform(value);
  }

  public int rhs() {
    return x();
  }

  public int lhs() {
    return y();
  }

  public int x() {
    return calculator.getX();
  }

  public int y() {
    return calculator.getY();
  }

  public int z() {
    return calculator.getZ();
  }

  public int t() {
    return calculator.getT();
  }
}

If you’re interested in the full source, let me know and I’ll make it available.

Brett the final refactoring is very nice and similar to Thomas’.

I think both show a deeper understanding of the stack and hence a simpler approach to using it.

Did separating concerns help with this ?

Note: I would have refactored the original method but wanted to draw out when/why to refactor because as I like to say “One mans hack is another mans quality.”

@Christian Vest Hansen, Srinivas Nandina, and Uncle Bob:

Here is the Scala equivalent of the Haskell version, using Uncle Bob’s magnitude function (not shown for brevity):

abstract class Stack(items:List[Int])
{    
  def take(v:Int) : Stack = this match
  {
      case ReplacingStack(_::rest)      
        => AccumulatingStack(v::rest)
      case InsertingStack(top::rest)    
        => AccumulatingStack(v::top::top::rest)
      case AccumulatingStack(top::rest) 
        => AccumulatingStack( top * magnitude(v) + v :: rest)
  }    
  def replace() : Stack = { ReplacingStack(items) }
  def insert() : Stack = { InsertingStack(items) }
  def accumulate() : Stack = { AccumulatingStack(items) }   
}
case class ReplacingStack(items:List[Int]) 
        extends Stack(items:List[Int])
case class InsertingStack(items:List[Int]) 
        extends Stack(items:List[Int])
case class AccumulatingStack(items:List[Int]) 
        extends Stack(items:List[Int])

James asked:

Did separating concerns help with this ?

So separating concerns … As it is, my calculator has a few concerns:

• Handling input modes depending on previous interactions
• Coordination of operation execution

I think it’s OK. While it does have multiple concerns, breaking it out more seems like possible overkill. This is really your original point, right?

Here’s my current calculator:

package com.scrippsnetworks.calculator;

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

  enum Mode {
    accumulating, replacing, inserting
  };

  public RpnCalculator() {
  }

  public RpnCalculator(MathOperatorFactory factory) {
    mathOperatorFactory = factory;
  }

  public void take(int value) {
    if (currentMode == Mode.accumulating) 
      value = determineNewTop(value);

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

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

  private int determineNewTop(int value) {
    int newTopValue = stack.pop();

    String digits = Integer.toString(value);
    while(digits.length() > 0) {
      newTopValue *= 10;
      newTopValue += 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) {
    MathOperator op = mathOperatorFactory.determineOperator(operatorName);
    op.execute(stack);
    currentMode = Mode.inserting;
  }

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

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

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

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

I think there’s more that could be done with this. There are some competing issues:

• Handling the four registers of a typical HP calculator (x, y, z, t) – sometimes it’s OK to violate the Law of Demeter.
• Taking in multiple kinds of input: single digits that are accumulated, multiple digits that are not accumulated, handling both (because I can) – this could be broken out into an “input policy” I suppose
• There’s also the MathOperatorFactory that maps strings to operators (this is there to allow for extension without change[the open/close principle]).

So given this, what next? This “works”, and there’s no changes coming down the pike. Based on that, no need to change anything. But what if there were? What’s next?

How about this for overkill:

• Allow the stack to be injected.
• Create an observable stack and inject it.
• The x, y, z, t methods could be removed and instead changes to the stack could be recorded via the observer pattern

What about the other suggestion, an input policy. I think this is a bad idea because the input policy is a little too coupled to the input mechanism.

i did get a gist of many posts…obviously could not read them all so i abstain from refactoring (mine was a suggestion on method extraction as the take() method is doing far too many things by itself. others have already made note of it).

Yes, there is a need for refactoring the method because a function must be functional ! And the take() is doing more than what it should. Functionally, its responsibility is to take some value and process it or forward it in a certain manner. In C#, this can be done with Lambda Expressions in a functional approach as demonstrated in the Haskell example, which is by far the most readable code of them all because it is a functional approach.

On the State and Strategy patterns applied, don’t they add up more code than is neccessary? Strategy is probably too specialized a pattern to apply in this scenario and State introduces only more code. If we look at the Haskell example design patterns look ridiculous!

Here is my example in c#, uses the original logic in the post. Also uses the state pattern but each state is just a lambda expression:

using System; using System.Collections.Generic;

public class States { public Action Accumulating { get { return (state, v) => { var digits = (int)Math.Pow(10, (int)Math.Log10(v) + 1); var x = state.Stack.Pop(); x *= digits; x += v; state.Stack.Push(x); }; } }

public Action Replacing
   {
      get
      {
         return (state, v) =>
         {
            state.Stack.Pop();
            state.Stack.Push(v);
            state.CurrentState = Accumulating;
         };
      }
   }
}

public Action Inserting
{
   get
   {
      return (state, v) =>
      {
         var top = state.Stack.Peek();
         state.Stack.Push(top);
         state.Stack.Push(v);
         state.CurrentState = Accumulating;
      };
   }
}

class State
{
   internal Stack Stack { get; set; }
   internal Action CurrentState { get; set; }
}

public State(Action initialState)
{
   CurrentState = initialState;
}

public void Take(int v)
{
   CurrentState(this, v);
}

Here is my example in c#, uses the original logic in the post. Also uses the state pattern but each state is just a lambda expression:

using System; using System.Collections.Generic;

public class States { public Action Accumulating { get { return (state, v) => { var digits = (int)Math.Pow(10, (int)Math.Log10(v) + 1); var x = state.Stack.Pop(); x *= digits; x += v; state.Stack.Push(x); }; } }

public Action Replacing
   {
      get
      {
         return (state, v) =>
         {
            state.Stack.Pop();
            state.Stack.Push(v);
            state.CurrentState = Accumulating;
         };
      }
   }
}

public Action Inserting
{
   get
   {
      return (state, v) =>
      {
         var top = state.Stack.Peek();
         state.Stack.Push(top);
         state.Stack.Push(v);
         state.CurrentState = Accumulating;
      };
   }
}

class State
{
   internal Stack Stack { get; set; }
   internal Action CurrentState { get; set; }
}

public State(Action initialState)
{
   CurrentState = initialState;
}

public void Take(int v)
{
   CurrentState(this, v);
}

Sorry last post got mangled, re posting:

Here is my example in c#, uses the original logic in the post. Also uses the state pattern but each state is just a lambda expression:

using System;
using System.Collections.Generic;

class State
   {
      internal Stack<int> Stack { get; set; }
      internal Action<State, int> CurrentState { get; set; }

      public State(Action<State, int> initialState)
      {
         CurrentState = initialState;
      }

      public void Take(int v)
      {
         CurrentState(this, v);
      }
   }

public class States
   {
      public Action<State, int> Accumulating
      {
         get
         {
            return (state, v) =>
            {
               var digits = (int)Math.Pow(10, (int)Math.Log10(v) + 1);
               var x = state.Stack.Pop();
               x *= digits;
               x += v;
               state.Stack.Push(x);
            };
         }
      }

      public Action<State, int> Replacing
      {
         get
         {
            return (state, v) =>
            {
               state.Stack.Pop();
               state.Stack.Push(v);
               state.CurrentState = Accumulating;
            };
         }
      }

      public Action<State, int> Inserting
      {
         get
         {
            return (state, v) =>
            {
               var top = state.Stack.Peek();
               state.Stack.Push(top);
               state.Stack.Push(v);
               state.CurrentState = Accumulating;
            };
         }
      }
   }

Sorry last post got mangled, re posting:

Here is my example in c#, uses the original logic in the post. Also uses the state pattern but each state is just a lambda expression:

using System;
using System.Collections.Generic;

class State
   {
      internal Stack<int> Stack { get; set; }
      internal Action<State, int> CurrentState { get; set; }

      public State(Action<State, int> initialState)
      {
         CurrentState = initialState;
      }

      public void Take(int v)
      {
         CurrentState(this, v);
      }
   }

public class States
   {
      public Action<State, int> Accumulating
      {
         get
         {
            return (state, v) =>
            {
               var digits = (int)Math.Pow(10, (int)Math.Log10(v) + 1);
               var x = state.Stack.Pop();
               x *= digits;
               x += v;
               state.Stack.Push(x);
            };
         }
      }

      public Action<State, int> Replacing
      {
         get
         {
            return (state, v) =>
            {
               state.Stack.Pop();
               state.Stack.Push(v);
               state.CurrentState = Accumulating;
            };
         }
      }

      public Action<State, int> Inserting
      {
         get
         {
            return (state, v) =>
            {
               var top = state.Stack.Peek();
               state.Stack.Push(top);
               state.Stack.Push(v);
               state.CurrentState = Accumulating;
            };
         }
      }
   }

public void take(int v) {
    int top = stack.peek();

    if (currentMode == Mode.inserting)
        stack.push(top);
    else
        stack.pop();

    if (currentMode == Mode.accumulating)
        stack.push(v + (top * (int)Math.pow(10, (int)Math.log10(v) + 1)));
    else
        stack.push(v);

    currentMode = Mode.accumulating;
}

Some of the proposed solutions have fifty or sixty lines and add a whole lot of classes and methods. Why make it so complex?

public void take(int v) {
    int top = stack.peek();

    if (currentMode == Mode.inserting)
        stack.push(top);
    else
        stack.pop();

    if (currentMode == Mode.accumulating)
        stack.push(v + (top * (int)Math.pow(10, (int)Math.log10(v) + 1)));
    else
        stack.push(v);

    currentMode = Mode.accumulating;
}

I don’t know about refactoring this short method, but if your goal is an rpn calculator program, you can see mine on my blog of programming etudes at http://programmingpraxis.wordpress.com/2009/02/19/rpn-calculator/.

>

@Mike Bria

Neat…I like the fact that you got the actOn methods to shield the reader from the implementation details of stack manipulation….but wait….in Accumulater you didn’t go the whole way… shouldn’t its actOn method be calling something like adjustTopBy(value), rather than stack.push(topAdjustedBy(value))?

@Brett L. Schuchert

You said: the determineNewTop method both changes the stack (with a pop) and returns a value. Violates command/query separation, but it is also a private method, so I’m not as concerned about that violation.

But what does violating command/query separation buy you? Unless the violation pays for itself, how about avoiding it by simply pushing the pop() (pun not intended) out of determineNewTop, and explicitly passing the popped value into the method?:

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

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

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

  private int determineNewTop(int top, int value) {
    int newTopValue = top;

    String digits = Integer.toString(value);
    while(digits.length() > 0) {
      newTopValue *= 10;
      newTopValue += Integer.parseInt(digits.substring(0,1));
      digits = digits.substring(1);
    }

    return newTopValue;
  }

And maybe, since you tolerated modifying input parameter ‘value’ in the ‘take’ method, you may want to do the same thing in ‘determineNewTop’: drop ‘newTopValue’ and just modify input parameter ‘top’.

Erlang version

obligary use of processes and message passing

start() ->
    F = fun(Mode, [Top | Stack], F) ->
        receive
        {take, V} ->
             case Mode of
             accumulating ->
                 Digits = round(math:pow(10, round(math:log10(V) + 1))),
                 F(Mode, [Top * Digits + V | Stack], F);
             replacing ->
                 F(accumulating, [V | Stack], F);
             inserting ->
                 F(accumulating, [V, Top, Top | Stack], F)
             end
        end
    end,
    spawn(F).

and a version without the spawn and messages

take({accumulating, [Top | Stack]}, V) ->
    Digits = round(math:pow(10, round(math:log10(V) + 1))),
    {accumulating, [Top * Digits + V | Stack]};
take({replacing, [_ | Stack]}, V) ->
    {accumulating, [V | Stack]};
take({inserting, [Top | Stack]}, V) ->
    {accumulating, [V, Top, Top | Stack]};

Philip Schwarz wrote:

But what does violating command/query separation buy you? Unless the violation pays for itself, how about avoiding it by simply pushing the pop() (pun not intended) out of determineNewTop, and explicitly passing the popped value into the method?:

I like it. In fact, I made that change and you’ll see it in the next exercise.

Thanks!

What a coincidence that Uncle Bob’s solution should appear straight after the Haskell one. I say this partly because these are the two solutions that stand out the most (in my view), but more importantly, because while they are so different, they both stand out (again, in my view) as the most readable.

My first reaction when I saw Uncle Bob’s solution, straight after the extreme succinctness of Haskell, was something along the lines of: what a huge contrast…how very long Bob’s solution is compared to the Haskell one…how can it take so much code to implement the same functionality?...

But then I started going back to Bob’s code, again and again, and once I took into account the huge expressiveness gap between the functional and OO paradigms, I have to say that I think that Bob’s solution is just as remarkable as the Haskell one in terms of its extreme readability.

Which of the solutions you have seen would you prefer to maintain?

This is a master at work, producing extremely readable code.

Brett’s original snippet, the one he asked us to refactor, was a single method with 15 lines of code, and a Cyclomatic Complexity (CC) of 4.

Bob has replaced this with 14 methods: nine one-liners, three two-liners, one three-liner, and one six-liner (the magnitude method). While the magnitude method has a CC of 3, all other methods have a CC of 1.

The result is some seriously readable code. No conditionals. Most of the time you are looking at a one-line method! A few times, it is a 2 or 3 line method. Also, while there is still some relatively high CC and high line-count in the system, it has been relegated to a single method (magnitude), which thanks to a very clear name and responsibility, won’t necessarily need to be read to understand all the rest of the code.

James Ladd said:

I’d like to play devils advocate and say that the method [Brett’s original] doesn’t need refactoring, since it is only 13 statements and not hard to understand. Why should you refactor this method? Note that some of the current refactorings come out to more than 13 statements and don’t necessarily add anymore clarity.

In my view, while Uncle Bob’s solution is much longer, it is much more readable.

I’ll conclude with a quote from Uncle Bob’s Clean Code:

the ratio of time spent reading vs. writing is well over 10:1. We are constantly reading old code as part of the effort to write new code.

Because this ratio is so high, we want the reading of code to be easy, even if it makes the writing harder. Of course there’s no way to write code without reading it, so making it easy to read actually makes it easier to write.

There is no escape from this logic. You cannot write code if you cannot read the surrounding code. The code you are trying to write today will be hard or easy to write depending on how hard or easy the surrounding code is to read. So if you want to go fast, if you want to get done quickly, if you want your code to be easy to write, make it easy to read.

Philip Schwarz wrote:

In my view, while Uncle Bob’s solution is much longer, it is much more readable.

Sorry, but I just can’t agree with this. Breaking things down into numerous very short methods like that actively harms readability, and it’s long past time people stopped dogmatically repeating “small functions = good” as if it’s some self-evident truth.

Why do we use functions? We use them to define an abstraction, to hide implementation details that are irrelevant to higher-level code. What are you abstracting by writing a single-line function like the ones in Uncle Bob’s example? Probably nothing. The problem doesn’t get any simpler by doing this.

On the contrary, although each individual method becomes trivially simple to follow, a reader now has to laboriously scan through the whole verbose mess to find enough context to understand what is actually happening. This is not an improvement, it makes things much more complicated.

As a result, it took me a couple of minutes just to be sure of the entry point in Uncle Bob’s example that corresponded to the original function. This was not helped by having several methods on different but intimately related types sharing the same name, compounded by virtuality in one case. I’d read and understood the mechanics of the entire original code snippet in far less time.

As for the cyclomatic complexity, of course we want to keep the logic straightforward, but let’s be serious: we’re talking about a single real decision here, a simple dispatch based on what appears to be a choice of three possible modes. The consequences of that decision affect a single piece of shared state (the stack) and are defined by a single extra input value (the parameter to the take function). Distributing such simple logic across 14 different functions over more than 80 lines of code is madness.

Moreover, the decision is based on a simple value, not a type, so obscuring it by using an enumeration/virtuality trick where a simple switch or if-chain would do is unhelpful. Use an enumeration for the possible states, sure. Pull out a function that switches on the current state and dispatches to one function per state accordingly, by all means. But the rest is just trying to be clever, and I don’t see any advantage at all, never mind anything to justify the extra verbosity and clutter introduced.

In a nutshell, this problem isn’t even close to the size and complexity where patterns like State and Visitor pull their weight, nor is there any evidence that it is likely to become so any time soon. If dogma is appropriate to a problem like this, I suggest that You Ain’t Gonna Need It would have been a better choice.

@Chris

Thank you for taking the time to explain why you think that rather than improving readability, breaking code down into many very short methods actually makes it harder to read. I was hoping someone would care enough to respond, and find the energy to do it.

You made several points, which I will tackle in separate posts.

I’ll start with a minor one.

You said: it took me a couple of minutes just to be sure of the entry point in Uncle Bob’s example that corresponded to the original function.

I pasted Uncle Bob’s code, a single class called Register, in an empty Java project in the Eclipse IDE, and opened up the class with the default Project Explorer, which showed the method signatures of the class, and immediately revealed only three public methods: insert(), replace(), and take(int), the latter clearly corresponding to the original function.

@Chris wrote:

Sorry, but I just can’t agree with this. Breaking things down into numerous very short methods like that actively harms readability, and it’s long past time people stopped dogmatically repeating “small functions = good” as if it’s some self-evident truth

Could you please elaborate further?

Do you have a particular time in mind when people dogmatically kept repeating that equation as if it was self-evidently true…was it this decade? was it the previous one?

And who was it that did this?, do you have particular personalities/methodologies in mind?

I am genuinely interested, and would be grateful if you could tell me more about this, or just give me some pointers.

@Philip Schwarz

If you’d like to debate further, I’ll be happy to defend my argument, but please allow me to clarify a couple of things immediately just to avoid talking at cross-purposes.

Firstly, I am not against using small functions in general. However, I think the value of a function, at least from a readability perspective, is dictated by the amount of abstraction it enables rather than by the number of lines of code it contains. A corollary to this is that functions of literally only one or two lines, which don’t have space to raise the level of abstraction very much, are often a warning sign to me, like seeing a class full of direct get/set methods for individual private data items and not much else. Sometimes these mechanics are justified, but often they are clues that the responsibilities and abstractions are broken.

Secondly, regarding my observation about locating the start-up code, of course you can fire up an IDE and use it to help you navigate code. My point was that in the original code I didn’t have to, because I’d comprehended the mechanics entirely just from reading the code, probably in less time than it would take to load that IDE. I find it hard to reconcile this with the claim that the longer code that requires IDE support to navigate with reasonable efficiency is more readable.

Apologies if the first part of this post is a duplicate; I seem to be having some trouble replying here today.

@Philip Schwarz

If you’d like to debate further, I’ll be happy to defend my argument, but please allow me to clarify a couple of things immediately just to avoid talking at cross-purposes.

Firstly, I am not against using small functions in general. However, I think the value of a function, at least from a readability perspective, is dictated by the amount of abstraction it enables rather than by the number of lines of code it contains. A corollary to this is that functions of literally only one or two lines, which don’t have space to raise the level of abstraction very much, are often a warning sign to me, like seeing a class full of get/set methods for individual private data items and not much else. Sometimes these mechanics are justified, but often they are clues that the responsibilities and abstractions are broken.

Secondly, regarding my observation about locating the start-up code, of course you can fire up an IDE and use it to help you navigate code. My point was that in the original code I didn’t have to, because I’d comprehended the mechanics entirely just from reading the code, probably in less time than it would take to load that IDE. I find it hard to reconcile this with the claim that the longer code that requires IDE support to navigate with reasonable efficiency is more readable.

I notice that you’ve just replied again, asking about the “small functions = good” claim I criticised. The first source that comes to mind is Clean Code by Uncle Bob himself, which has this to say on the subject:

The first rule of functions is that they should be small. The second rule of functions is that they should be smaller than that. This is not an assertion that I can justify. I can’t provide any references to research that shows that very small functions are better. [...] What this experience has taught me, through long trial and error, is that functions should be very small.

Of course, there are numerous other sources, from web sites to in-house style guides, that claim that functions should be no longer than one screen/50 lines/some other arbitrary limit. Googling “functions should be short” appears to turn up plenty of examples.

This is in marked contrast to, for example, Code Complete by Steve McConnell, in which the author describes “a mountain of research” on the subject and cites six distinct sources that do not support the benefits of very small functions.

Apologies if this post is a duplicate; I seem to be having some trouble replying here today.

@Philip Schwarz

Regarding the “small functions = good” claim I criticised, the first source that comes to mind is Clean Code by Uncle Bob himself, which has this to say on the subject:

The first rule of functions is that they should be small. The second rule of functions is that they should be smaller than that. This is not an assertion that I can justify. I can’t provide any references to research that shows that very small functions are better. [...] What this experience has taught me, through long trial and error, is that functions should be very small.

Of course, there are numerous other sources, from web sites to in-house style guides, that claim that functions should be no longer than one screen/50 lines/some other arbitrary limit. Googling “functions should be short” appears to turn up plenty of examples.

This is in marked contrast to, for example, Code Complete by Steve McConnell, in which the author describes “a mountain of research” on the subject and cites six distinct sources that do not support the benefits of very small functions.

@Chris wrote:

Why do we use functions? We use them to define an abstraction, to hide implementation details that are irrelevant to higher-level code. What are you abstracting by writing a single-line function like the ones in Uncle Bob’s example? Probably nothing. The problem doesn’t get any simpler by doing this.

On the contrary, although each individual method becomes trivially simple to follow, a reader now has to laboriously scan through the whole verbose mess to find enough context to understand what is actually happening. This is not an improvement, it makes things much more complicated.

Object Mentor have their own school of thought about clean code. They don’t claim their particular school to be right. They do claim that if you follow their teachings, you will learn to write code that is clean and professional. They say that there are other schools of thought, and other masters, that have just as much claim to professionalism as they do. While they present their opinions as “their” absolutes (the way they practice their art, they acknowledge that many of their recommendations are controversial, and that not everyone will agree with them.

In Clean Code, Uncle Bob kicks off his recommendations for functions with three key principles: they should be small, they should only do one thing, and all their statements should be at the same level of abstraction.

About function size, he says:

the first rule of functions is that they should be small, and that the second rule is that they should be smaller than that.

His experience has taught him that functions should be very small. Describing a program written by Kent Beck, he says:

every function in this program was just two, or three, or four lines long. Each was transparently obvious. Each told a story, And each led you to the next in a compelling order. That’s how short your functions should be!

Going back to the three principles (Small, Do One Thing, Same Level of Abstraction), you will recognise these as being the three components of Kent Beck’s Composed Method pattern:

Q: How do you divide a program into methods?
A: Divide your program into methods that perform one identifiable task. Keep all of the operations in a method at the same level of abstraction. This will naturally result in programs with many small methods, each a few lines long.

When describing the pattern, Kent Beck says:

The opportunity to communicate through intention revealing message names is the most compelling reason to keep methods small. People can read your programs much more quickly and accurately if they can understand them in detail, then chunk those details into higher level structures. Dividing a program into methods gives you an opportunity to guide that chunking. It is a way for you to subtly communicate the structure of your system.

And here is Kent Beck’s Intention Revealing Message pattern:

Q: How do you communicate your intent when the implementation is simple?
A: Send a message to “self”. Name the message so that it communicates what is to be done rather than how it is to be done. Code a simple method for the message.

Here is my translation into Java of one of his Smalltalk examples:

Collection isEmpty()
{ 
    return this.size() == 0 
}

When describing the pattern, Kent Beck says:

What is going on? Communication. Most importantly, one line methods are there to communicate...Intention revealing messages are the most extreme case of writing for readers instead of the computer…methods that separate intention (what you want done) from implementation (how it is done) communicate better to a person.

Brett’s original ‘take’ method is NOT a composed method.

It doesn’t do just one thing (try describing it!): it is checking and modifying some state, doing some maths, manipulating a stack, and doing different things in different states.

Its statement are not at the same level of abstraction: on one line it is doing a low-level computation of the number of digits in a value, in others it using that number to compute a higher level function, in others it is manipulating a stack.

The method is not short: while it is not long, it is not a few lines.

Also, the method does not delegate to intention-revealing methods. It just exposes you in one go to all the gory details of how it does what it does.

If instead you take Uncle Bob’s ‘take’ method for the ‘inserting’ mode:

public void take(Register r, int v) {
  r.dup();
  r.enter(v);
  r.accumulate();
}

It is much more composed than the original method, and its calls to r.dup(), r.enter(...), and r.accumulate() (examples of the single-line methods you object to) are very similar in their effect to invocations of intention-revealing methods: by calling them, the ‘take’ method tells you what it is doing, rather than how it is doing it (e.g. by manipulating a low-level stack or setting a class variable).

Regarding your concern about ‘laboriously scanning through the whole verbose mess to find enough context to understand what is actually happening.’, this is acknowledged by the main proponents of small methods as a price worth paying.

In Refactoring, Fowler writes:

The object programs that live best and longest are those with short methods. Programmers new to objects often feel that no computation ever takes place, that object programs are endless sequences of delegation. When you have lived with such a program for a few years, however, you learn just how valuable all those little methods are. All of the payoffs of indirection – explanation, sharing, and choosing – are supported by little methods

There is still an overhead to the reader of the code because you have to switch context to see what the subprocedure does….the real key to making it easy to understand small methods is good naming. If you have a good name for a method you don’t need to look at the body.

The net effect is that you should be much more aggressive about decomposing methods. A heuristic we follow is that whenever we feel the need to comment something, we write a method instead. Such a method contains the code that was commented but is named after the intention of the code rather than how it does it. We may do this on a group of lines or on as little as a single line of code. We do this even if the method call is longer than the code it replaces, provided the method name explains the purpose of the code. The key here is not method length but the semantic distance between what the method does and how it does it.

In Smalltalk Best-Practice Patterns, Kent Beck writes:

Following the flow of control in programs with many small methods can be difficult. Novice Smalltalk programmers often complain that they can’t figure out where any “real” work is getting done. As you gain experience, you will need to understand the flow of control through several objects less often. Well chosen message names let you correctly assume the maning of invoked code.

And in Implementation Patterns, Kent Beck says:

Code readers need to solve several problems that provide opposing influences on the size of methods. When reading for overall structure, seeing lots of code at once is valuable. ... The same big method that helped me orient myself becomes a hindrance when I turn to trying to understand the code in detail….Simultaneously supporting browsing and digesting is the challenge of the code author who is dividing logic into methods. I find that my code reads best when I break it into relatively small methods (at least by C standards).

@Chris

You said:

This is in marked contrast to, for example, Code Complete by Steve McConnell, in which the author describes “a mountain of research” on the subject and cites six distinct sources that do not support the benefits of very small functions.

Thanks for that, I will dig my copy out.

@Chris

OK, so you have a copy of clean code…good…based on your opinion of one-line methods, I suspect that Heuristic G30 (Functions Should do One Thing) will give you a heart attack:

public void pay()
{
  for (Employee e : employees)
  {
    if (e.isPayDay())
    {
      Money pay = e.calculatePay();
      e.deliverPay(pay);
    }
  }
}

This bit of code does three things…This code would be better written as:

public void pay()
{
  for (Employee e : employees)
  {
    payIfNecessary(e);
  }
}

public void payIfNecessary(Employee e)
{
  if (e.isPayDay())
  {
    calculateAndDeliverPay(e);
  }
}

public void pay()
{
  Money pay = e.calculatePay();
  e.deliverPay(pay);
}

What do you think?

@Chris

I typed in that last method incorrectly…here is the correct version:

public void calculateAndDeliverPay(Employee e)
{
  Money pay = e.calculatePay();
  e.deliverPay(pay);
}

@Philip Schwarz

We seem to be overlapping here because of some funny delays in posting, so I don’t know if you’d seen my earlier posts that already addressed some of your points when you wrote your latest contribution. I’ll just briefly comment on a couple of your other points for now.

On the subject of Composed Method:

Divide your program into methods that perform one identifiable task. Keep all of the operations in a method at the same level of abstraction. This will naturally result in programs with many small methods, each a few lines long.

The first two principles I completely agree with. This is basic functional decomposition advice that has been around since the programming stone age (though alas many professional developers seem to have forgotten it shortly afterwards in their quest to objectify this and higher level language that).

But I can’t agree with the conclusion. I’ve worked on code in several fields, from mathematical modelling to instrument control, where it would be perfectly possible to have a function dozens, perhaps even hundreds, of lines long that did represent a single task with the implementation at the same level of abstraction throughout. Usually the control flow would be quite linear, but subdividing the logic further would just be an artificial break.

In any case, as I mentioned before, my objection is not to small functions as such, just to small functions that don’t really gain us any useful abstraction. To wit, I see a marked contrast between your comments on the Intention Revealing Message pattern with the isEmpty example and Uncle Bob’s take function that you quoted. In particular, I could immediately guess what isEmpty would mean from the calling code, and while it is only a single line long, it does represent a useful abstraction.

On the other hand, despite knowing the general problem area we’re discussing, I still don’t see why the names take, dup, enter and accumulate are particularly helpful here. take seems rather arbitrary: neither it nor the name Register provide enough context for it to be a meaningful concept without further information. dup betrays the underlying implementation, while enter randomly obscures the implementation despite operating at the same level of abstraction. The worst is accumulate, which doesn’t accumulate anything at all, despite having a common meaning along those lines when applied to a sequence in computer science literature; it actually changes the mode (and apparently in a way that only code related to the Register class is permitted to do, though the analogous functions for setting other modes are public). In fact, looking over the entirety of Uncle Bob’s code again, it is littered with one-liners that are never actually used, yet betray implementation details or at best rename something without providing any additional abstraction.

In short, reading Uncle Bob’s take function, I would have had to look up every single function it called anyway, to drop to a useful level of abstraction and work out what the underlying logic was, completely defeating the point of factoring out the individual routines. However, to work out what other code interacted with the take function, I would also have to analyse all code with access to the same instance of Register because the numerous small, public functions effectively expose the implementation anyway. Whatever happened to “Tell, don’t ask”?

As for the small methods vs. increased interactions issue, I don’t have much to add there. A few consultants can repeat the mantra as often as they like, but the evidence simply doesn’t support their conclusion. Increasing complexity is definitely a causal factor in making code harder to understand; this much is well supported by research. Moreover, it seems likely that there is a positive correlation between function length and function complexity, but it doesn’t follow that small functions automatically help readability. It does follow, however, that if the functions get so small that the complexity starts to increase because of all the added interactions, this harms readability.

@Philip Schwarz

I think the code you cited is an excellent example of taking a good rule of thumb to such an absurd extreme that it breaks.

I am amused by the fact that the one function in the proposed rewrite that does do something actually does two things; the name even describes (with no useful abstraction whatsoever) what those two things are. If iteration and alternation violate the Sacred Principle of Oneness enough to justify separating everything inside any pair of {} then surely sequencing does as well? I’m mildly surprised that the person who wrote that originally hasn’t yet died of old age waiting for an infinite recursion to finish! :-/

The tragic thing is that while messing around with dogmatic changes to achieve small functions for no particular benefit, the author has missed the wood for the trees. The first question I would ask about the original function is why this code is messing around so much with the details of another object. Tell, don’t ask:

public void pay()
{
  for (Employee e : employees)
  {
    e.pay();
  }
}

and on the Employee class:

public void pay()
{
  if (isPayDay())
  {
    deliverPay(payDue());
  }
}

Then we might consider further improving the function names, but cleaning those up requires knowledge of the context that hasn’t been provided in this example, so there’s not much we can do beyond renaming the calculation function to something more appropriate given that its purpose is defined by the value it returns.

Of course, this whole example is very artificial, as it’s unlikely that in a real system an Employee object would know how to make a payment without depending on other parts of the system anyway…

@Chris

You said:

I could immediately guess what isEmpty would mean from the calling code, and while it is only a single line long, it does represent a useful abstraction. On the other hand, despite knowing the general problem area we’re discussing, I still don’t see why the names take, dup, enter and accumulate are particularly helpful here. take seems rather arbitrary: neither it nor the name Register provide enough context for it to be a meaningful concept without further information.

OK..so while you find ‘isEmpty’ to be intention-revealing, the same is not true for Uncle Bob’s one-line methods.

If you look back 11 posts from Uncle Bob’s, you’ll see Brett’s firts follow-up post, in which he says:

By the way, this is the beginnings of a method to handle input for an RPN calculator. Input is more complex than I first imagined:

• Initially digits are accumulating.
• After pressing enter, the next digit will replace the top of the stack (the x register) because pressing enter duplicated the current top.
• After executing an operator, e.g., + – / * !, typing a digit duplicates the top of the stack (while another operator uses what’s there).

Based on that, the following is possible:

1. The accumulate method is called that way because it tells the Calculator to go into a state in which it accumulates digits.
2. The take method is called that way because it takes the user’s input to the calculator.
3. The Register class is called that way because the Calculator has a number of register, e.g. x.
4. The dup method maps to the problem domain (or system metaphor) concept of duplicating the top of a stack
5. The enter method maps to the problem domain concept of pressing the calculator’s enter button.

So Uncle Bob has created methods that map to problem-domain abstractions and are therefore intention-revealing to anyone who knows enough about the problem domain. To make the code readable, he has hidden code that tells us HOW the problem solution is implemented, behind code that models WHAT happens in the problem domain.

You seem (to me) to be saying that Uncle Bob’s one-line methods make the code harder to read because they are not intention-revealing (to you because you don’t know enough about the problem domain), so the code would be better off (more readable) if instead of telling you about WHAT happens in the problem domain, it just told you HOW the solution to the problem is implemented e.g. using stack operations.

I disagree. Uncle Bob has chunked implementation details and hid them behind intention-revealing method names, so that people who know enough about the problem domain (e.g. anyone intending to maintain the code) can read his code more quickly and accurately.

@Philip Schwarz

The thing is, calling a function “enter” like that isn’t intention revealing. On the contrary, it is describing not what the function does but when it should be called in terms of specific user actions. There is a place for event handlers like that, but it’s in your user interface modules. Why is any code in an internal class like this even aware of what the nature of the user interface is, never mind referring to specific user actions?

Again, there is a not-seeing-wood-for-trees problem here. If we were to expand the scope of the problem to include the user interactions - which is far beyond what the original function covered and not a mere refactoring, of course - then we should be creating an internal class to model the stack behaviour with interface methods representing domain concepts such as pushing a number onto the stack, applying an operator and showing the current top of the stack, and we should also have separate code to handle UI quirks like parsing single keystrokes and evaluating when they become triggers for certain meaningful actions in the problem domain.

Chris wrote:

On the contrary, it is describing not what the function does but when it should be called in terms of specific user actions.

I think you are a bit off the mark. Ultimately, the underling facade supporting some user interface needs to have the ability to represent logical operations from the perspective of the user.

Sure, this is not always a 1-to-1 mapping (and in fact it should not be so most of the time). However, there is a difference between “entering the digits of the number” and “starting the next number”.

So some questions I might ask:

• What are the logical interactions?
• How many steps do I want to make explicit in the design of my facade layer?
• How will one design impact multiple potential user interfaces?
• Can that one facade server all, or is it easy enough to adapt to make it a reasonable abstraction?
• Does the facade as designed preclude some kind of interaction?
• ...

Here is an example I came across on a project in 1997…

The project allowed flight rebookings on Palm 7’s, the web, WAP enabled phones and smart two-way pagers. It worked well, it was ready for use and it was in production (well it was in a beta sense).

Then along came a new 2-way pager that used SMTP as its communication between the pager and the base. The average response time was 1 minute. The minimum was 30 seconds and the max was 3+ minutes.

We had designed several logical steps to change a flight (end to end):

• Go to a URL/bookmark
• Log in
• Review itineraries
• Select itinerary to change
• Indicate desired change (earlier/later)
• Select one from a list of options
• Accept change fees
• Log out

Ignoring logout, that’s 7 steps. On this new device, that would, on average, require 7 minutes.

We had a conflicting requirement to make a system that was faster than making a phone call. 7 minutes was off the mark by several minutes.

So we had to adapt the underlying interaction. We left the underlying service as is but we developed an application for the two-way pager. This application maintained a cache of itineraries and credentials.

The new interaction was:

• Start application (no time, no hit)
• Select itinerary and indicate earlier or later booking (hit one)
• Select from option
• Accept change fees

This was considered too long (three minutes), so we made it even simpler:

• Select itinerary and provide a window (2 – 4 hours earlier)
• Auto select change fees if < $250

So this made it fast enough, but it took away too much from the experience and was rejected. We actually had to bring the manufacturer on site to tell them there really were no other options.

Why do I bring all of this up at all?

When you are building a system, a part of its design is user interaction. Part of that is designing a system that accommodates all of the points of interaction that make the best possible user experience, removing was is not necessary (or relegating it to an alternative).

In this case, enter() is the name given for the logical idea of completing input on one number.

Can this be removed? Yes. Can a calculator designed without this interaction work with many different user interfaces? Yes (I’ve done it). Does it map well to the domain? YES.

The HP calculators have been around since the late 60’s (see http://www.hpmuseum.org/

So to take it away in fact could make the underlying solution less close to the domain and in fact result in a worse solution.

One person’s intention is another person’s “specific user actions”.

Discussing the “enter” function is taking a phenomenological perspective, removing it and discussing “indication completion” is moving towards the ontological.

Context dictates one approach as “better” than another – at least for now.

public void take(int v) {

currentMode.take(stack, v);
  currentMode = new CurrentModeAccumulating();

}

...

class CurrentModeAccumulating : ICurrentMode {...}

class CurrentModeReplacing : ICurrentMode {...}

class CurrentModeInserting : ICurrentMode {...}

I took a quick look to the current solutions, and it seems mine is veeery close to Peter Bona’s.

abstract class Mode {
  public abstract Mode void take(Stack stack, int v);

  public static final Mode accumulating = new Mode() {
    private int calculatePowerOfTen(int v) {
      return (int)Math.pow(10, (int)Math.log10(v) + 1);
    }

    Mode void take(Stack stack, int v) {        
      int top = stack.pop();  
      stack.push(top * calculatePowerOfTen(v) + v);
      // i chose to use the temp because it's more explanatory this way
      // and because pop() has side effects and i don't want to include it
      // in an expression
      // but i could have easily written
      // stack.push(stack.pop() * calculatePowerOfTen(v) + v);
      // it would be the same regarding functionality
      // but i feel it's more readable this way
      return this;
    }
  };

  public static final Mode replacing = new Mode() {
    Mode void take(Stack stack, int v) {
      stack.pop();
      stack.push(v);
      return Mode.accumulating;
    }
  };

  public static final Mode inserting = new Mode() {
    Mode void take(Stack stack, int v) {
      stack.push(stack.peek());
      stack.push(v);
      return Mode.accumulating;
    }
  };
}
 

The originial take method becomes:

public void take(int v) {
  currentMode = currentMode.take(stack, v);
}

Mode is an implementation of the State pattern. I preffered your initial use of constans and also, I used annonymous classes for conciseness. You could as well have used proper classes, subclassed from Mode base class.

The difference from Peter’s solution are little readability refactorings, like eliminating on of the temps in the accumulating mode with a method.

Two points I’d like to make to your problem: you didn’t specify what the purpose of the refactoring is, so I could well say that the already implemented solution is fine as it is. And second, “using a few design patterns just because you can” is bad! :)

@Brett

I think perhaps we are talking at cross-purposes. I am not objecting to the name “enter” in itself, nor to the subtle distinction between “pressing enter” and “completing an input”. I am objecting to the use of any interface-level concepts in the domain model.

In a RPN calculator, the domain interface requires three basic operations: putting in a number, putting in an operation, and reading out the current value.

Internally, of course, it maintains some sort of stack, but the state of that stack is evident only through operations that implicitly change the values at the top of the stack and through reading the current top value.

Similarly, there is some logic associated with knowing when a sequence of digits typed by the user is a new number ready to put onto the stack, but this matters only to the UI code. We can see that things like typing an individual digit or pushing an enter button are not in themselves meaningful concepts in the domain, because they make a difference only when they result in pushing a new number onto the stack.

This structure would have been immediately obvious had there been separation between the domain model (probably a class with only three public methods plus constructor) and the UI code (with whatever state machine might be appropriate to handle the individual key presses). But because in Uncle Bob’s version there is just the one huge Register class trying to do everything with many tiny methods, there isn’t even this basic separation of concerns and the levels of abstraction are all mixed up.

http://www.superstonejerseys.com

think you ! http://www.yourvoguemall.com

I find it incredible that more blogs and forums are not as pleasant as this one.moncler downOften times when I land on a website page the articles and other content is so deficient that I move on without delay. That is not the case here. Thanks so much.moncler men

What does this picture suggest to you or what can you draw from it, if anything?

white iphone 4 is so charming and chic. As the icon of latest fashion, how can you miss it!

Another fantastic posting here at Object Mentor!! Thanks for sharing and keep up the great work.:)

We are the professional dresses manufacturer, dresses supplier, dresses factory, custom dresses.

website page the articles and other content is so deficient that I move on without delay. That is not the case here.

thank you very much.

internette görüntülü olarak okey oyunu oyna, gerçek kisilerle tanis, turnuva heyecanini yasa.

it needs a bokmark so i can come back to it later ,nice stuff

Great post. It appears that most of the steps are relying on the creativeness factor&.

other content is so deficient that I move on without delay. That is not the case here.beats by dre sale cheap beats by dre

Swarovski crystal jewelry, on top of that also known as these Austrian Swarovski Crystals, is definitely the most beneficial many costly type of Crystals Swarovski uk . As a result, a majority of these Swarovski Uk items need be adopted special care involving to allow them to last perpetually.

I am carrying this bag myself,Herren Winterjacken when I wear very girly stuff, mine is smaller than large,herren winter still can fit lap top and all I need…I had been looking for it for several years until I found it 3 years ago in UK.Belstaff Jacken I saw it for the first time in the movie “Interpreter”, carried by Nicole Kidmann, and then in “I, Legend” carried by Will Smith.http://www.herrenwinterjacken.com/

Slewing bearing called slewing ring bearings, is a comprehensive load to bear a large bearing, can bear large axial, radial load and overturning moment.

The results of ethnic psychology constitute, at the same time, our chief source of information regarding the general psychology of the complex mental processes. Wilhelm Wundt

I liked you blog so im going bookmark it with my prefered websites, you have posted an amazing posts so thank you I liked you blog so im going bookmark it with my prefered websites,

Women today are becoming more and more conscious of their fashion sense. It is not surprising, therefore, that Christian Louboutin black shoes women are investing their money on designer label clothes, accessories, and popular women shoe brands. Women Christian Louboutin wedding who know the value of their money often lean towards designer label items because branded items always offer wedding shoes by Christian Louboutin guarantees and warranties especially in the event of returns or exchanges and most important of all, you can never contest the quality of branded items.