4
\$\begingroup\$

I've got the following code in my calculator project:

private ITerm CalculatePostfixExpression(IEnumerable<ITerm> input)
{
    var tempResult = new Stack<ITerm>();
    foreach (var term in input)
    {
        if (term is IOperand)
        {
            tempResult.Push(term as IOperand);
        }
        if (term is IOperator)
        {
            tempResult.Push(ProceedOperation(term as IOperator, tempResult));
        }
    }
    return tempResult.Peek();
}

Now it's violating the open-closed principle, so I'm asking for a help to improve my code.

It's kinda like a "Replace Conditional with Polymorphism" but I can't understand how to use it in my case.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! Good job on your first question. I don't know about your code base, but it could be beneficial to yourself and reviewers if you post more/all of your calculator project. \$\endgroup\$ – SirPython Feb 24 '16 at 20:22
1
\$\begingroup\$

First, note that with code like this that has to read in external input, and convert it into objects, you typically end up with some sort of factory class that's kind of the border where you often have difficulty applying the open-closed principle very well. That is, it often needs at least a little tweaking when the rest of the system changes--about the best you normally hope for is to restrict your changes to this one area.

So, what I'd probably do in this case is have a single "stack manipulator" interface, and have both an operand and operators that implement that interface.

// Note: this isn't intended to be compilable code, just pseudo code with
// syntax similar to Java/C#/C++/etc.
//
interface stack_manipulator { 
    void execute(stack);
};

class operand : implements stack_manipulator { 
    void execute(stack s) { 
        s.push(this);
    }
};

class add : implements stack_manipulator { 
     void execute(Stack s) { 
         // skipping error checking and such for clarity
         var a = s.pop();
         var b = s.pop();
         s.push(a + b);
     }
};

class sub : implements stack_manipulator {
    void execute(Stack s) {
        var a = s.pop();
        var b = s.pop();
        s.push(a - b);
    }
};

// ... and so on for other operators

Then your ITerm CalculatePostfixExpression would look something like this:

    var tempResult = new Stack<ITerm>();
    foreach (var term in input) {
        term.execute(tempResult);
    }
    return tempResult.Peek();

...and modifications to the other code won't require changes to this (as long as the other code adheres to the stack_manipulator interface. As hinted at above, however, somewhere you'll need a factory class that knows how to map from input characters to the individual types that implement the stack_manipulator interface.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.