OO Calculator -- a Tutorial

[Michael Matola]

Originally posted by Bryon Phinney:
A question I have is will the "Input translation" know about the Operator objects. Restating - should there be a OOCalc.in(Operator operator) method?

Depends. I've been thinking about this a little too, but it is several steps ahead of the game. At this point I think it's clear were working towards a factory method that will determine which operation to perform (or place on a stack for later execution) by using some symbolic constant to pick a concrete subclass of an abstract Operation. Whether those symbolic constants will be Strings in OOCalc, such as our current ADD, or, say, typesafe enumerations held in Operation that have specific subclasses of Operation as instances (as you've suggested) is really up in the air at this point. I guess at some point the decision will come down to what code we'll want to let know about the subclasses of Operation -- the Operation class itself or OOCalc's in() or getResult().

[Junilu Lacar]

I see Michael is really on to me now.
David, regarding the expansion of testAddition to handle successive additions, I think we should create a separate test for that. We've already got the simple addition going and we've already refactored to a reasonably acceptable level. We can now cross off one item in the To Do list.
The sequence of additions is something we eventually want to test so we add it to the ToDo list. We could choose to write a test for it next but I prefer to write a test for subtraction next. It is only a slight variation to the addition test and I have a feeling it will lead to duplication. We want to discover duplication ASAP so we can refactor our code. The more refactoring we do at this early stage, the less we have to do later. Right now, I just want the design to get to a point where it is easy to add the functionality of a new operation.
Our next steps would be:
1. pick another reminder from the ToDo list and write a test. Subtraction looks like it will make us copy-paste (bad programmer!) so this is what we'll do. We'll sow our wild oats while we're still too young to go to jail for them
2. write stubs to make the test compile and fail
3. quick and dirty implementation to make test pass
4. refactor and atone for our sins from #1 and #3.
[ March 26, 2002: Message edited by: Junilu Lacar ]

[Bryon Phinney]

Michael, thanks for the direction:

I've been thinking about this a little too, but it is several steps ahead of the game

I find it difficult to restrain myself and take small steps. Can anyone give some direction on what constitutes a small step. I see the value of it, but when I see a direction, I take the step not thinking of the size.

[Junilu Lacar]

Originally posted by Bryon Phinney:
Can anyone give some direction on what constitutes a small step.

What constitutes a small step will depend entirely on you and your comfort level in writing X lines of code before going on to the refactoring step.
The point is that you can take smaller steps if you find that you've done too much too soon. One sign that you may have made too big a step is when you get stuck and ask "Now what?"
The advantage of taking smaller steps IMO is that it gives you more chances to reflect on where the current design is going and you can see more possibilities/alternatives than you would have if you had taken a larger step. Refactoring is where you stop and reflect, then plan and prepare the way for your next step.
One interesting thing that seems to happen is that when you first start out using test-first programming, you are uncomfortable in taking such small steps. That's probably because, like many others, you are used to writing everything first before doing any testing. But as you get a hang of TDD(test-driven development)/TFP, as you become accustomed to the rhythm of test/stub out/implement quickly/refactor, you find yourself taking smaller and smaller steps because you get addicted to seeing that green bar and the confidence and satisfaction that it gives moving forward.
If you do see a way to confidently go from A to D without the smaller intermediate steps of C and D, by all means go ahead. Just make sure you don't blame me if you fall
Junilu

[Junilu Lacar]

Throw me a bone here guys. I was hoping somebody would have caught it by now but I'll give another hint: Duplication is a bad code smell. See if you can find any duplication in the refactored code in OOCalc, Step 9. Take a look at getResults() and performOperation().
[ March 26, 2002: Message edited by: Junilu Lacar ]

[Michael Matola]

Almost posted along similar lines last night (yeah, yeah, right), but what I was writing got so long I decided to set it aside for a day and come back to see if I could shape it up.
Very short version --
The main issue I had was that the " return 0.0 ; " at the bottom of performOperation() was unreachable, given the current clients of the code, and should be refactored away somehow. The duplication between getResults() and performOperation() didn't bother me as much as the unreachable return because I figured they'd eventually get combined when we implement the factory. Also, I was going to rail that in getResults() the "else" was redundant and the condition was the wrong way round, that it should be the following, so the unusual case returns earlier than the standard case.

if ( operation == null )
{
    return 0.0 ;
}
return performOperation() ;

[Steve Fahlbusch]

Greetings,
I just ran across this, and quickly tried to
come up to speed (and since I'm a little
under the weather I may not be seeing something).
You are utilizing a stack for the operands, and we know that any binary operator would take the process of:
pop, pop, doOP, push.
and a unary operator:
pop, doOP, push.

Are we assuming that we have have not discovered
this yet (and will so when we get there)? ie, the
5 + 5 + 5 {test}) or do we know this, but only
adding what is needed for each individual "requirement"
as it were?
Steve Fahlbusch

[Junilu Lacar]

Seems like more than a few folks here already see where we're headed so just hold yer horses a little longer. We'll pick up the pace soon.

[Ilja Preuss]

Originally posted by Steve Fahlbusch:
Are we assuming that we have have not discovered
this yet (and will so when we get there)? ie, the
5 + 5 + 5 {test}) or do we know this, but only
adding what is needed for each individual "requirement"
as it were?

For me, it's the latter. Would it make a difference if it where the first?
In fact, I am not fully convinced that we *will* need a stack (in this form) - at least for the currently implemented features we could get away with only a results field. When I have more time (probably not before friday), I will try an alternative implementation and post it here - it might be interesting to see how the two evolve side by side...

[Ilja Preuss]

Originally posted by Michael Matola:
Almost posted along similar lines last night (yeah, yeah, right), but what I was writing got so long I decided to set it aside for a day and come back to see if I could shape it up.
Very short version --
The main issue I had was that the " return 0.0 ; " at the bottom of performOperation() was unreachable, given the current clients of the code, and should be refactored away somehow. The duplication between getResults() and performOperation() didn't bother me as much as the unreachable return because I figured they'd eventually get combined when we implement the factory. Also, I was going to rail that in getResults() the "else" was redundant and the condition was the wrong way round, that it should be the following, so the unusual case returns earlier than the standard case.

if ( operation == null )
{
    return 0.0 ;
}
return performOperation() ;

Now we have unreachable code in performOperation. We could get rid of the if-statement there. Perhaps we should?

[Ilja Preuss]

Originally posted by Michael Matola:

Depends. I've been thinking about this a little too, but it is several steps ahead of the game. At this point I think it's clear were working towards a factory method that will determine which operation to perform (or place on a stack for later execution) by using some symbolic constant to pick a concrete subclass of an abstract Operation. Whether those symbolic constants will be Strings in OOCalc, such as our current ADD, or, say, typesafe enumerations held in Operation that have specific subclasses of Operation as instances (as you've suggested) is really up in the air at this point. I guess at some point the decision will come down to what code we'll want to let know about the subclasses of Operation -- the Operation class itself or OOCalc's in() or getResult().

For me, I would already introduce the Operator class as Typesafe Enumeration - mainly because of communication.
Think about it this way - if you would publicize it, which method would need more documentation: in(String) or in(Operator)? Then choose the one which needs the less documentation.
As a side effect, in almost every case I find some data and/or behaviour to move into the Enumeration class afterwards, transmogrifying it into a State/Strategy.

[Ilja Preuss]

Originally posted by Terry McKee:
Second, I am by no means an expert, but it seems to me that having two return statements is not a good idea. I really believe that OO code should build on Procedural code. That means in this method there should be one entrance (the method call) and one exit (the return statement).

Does anybody know where the "one exit" rule is coming from? How is it motivated?

[Ilja Preuss]

Originally posted by Ilja Preuss:

Does anybody know where the "one exit" rule is coming from? How is it motivated?

Just found http://c2.com/cgi/wiki?SingleFunctionExitPoint
I really love this wiki!

[Junilu Lacar]

Originally posted by Ilja Preuss:
As a side effect, in almost every case I find some data and/or behaviour to move into the Enumeration class afterwards, transmogrifying it into a State/Strategy.

That's exactly what we'll do next so pull up a chair and get comfy because this is going to be a long one. These next few steps are actually just getting ready to do the "Replace Conditional with Polymorphism" that we first wanted to do but couldn't because it seemed like too big of a step.

[David Weitzman]

Originally posted by Junilu Lacar:
Throw me a bone here guys. I was hoping somebody would have caught it by now but I'll give another hint: Duplication is a bad code smell. See if you can find any duplication in the refactored code in OOCalc, Step 9. Take a look at getResults() and performOperation().
[ March 26, 2002: Message edited by: Junilu Lacar ]

I still think that getResult() should have no logic, as I said earlier. Maybe even eliminate performOperation() and run its logic when in(OOCalc.EQUALS) is called. That clears up OOCalc and introduces more procedural code that can be refactored.

[Junilu Lacar]

Originally posted by Ilja Preuss:
Just found http://c2.com/cgi/wiki?SingleFunctionExitPoint

It seems to me from reading this wiki page that Single Exit Point is more of a "tradition" in the lines of the mother who, while teaching her daughter the recipe for pot roast, was asked by the daughter why she cut a piece off the end before putting the meat in the pot. Knowing only that it was what her mother had always done, the mother phoned the grandmother and asked her why. The answer: "Our pot was too small and I couldn't get the lid on if I didn't."
Of course with Java and its garbage collector, we no longer have the same problems that "older" languages had with multiple returns.
Junilu

[Michael Matola]

Regarding a single return statement per method (not exactly the same as single exit point, in a language that can throw exceptions) --
This is the stuff of religious wars. My own take is that either extreme -- slavishly following the rule of only one return per method or simply returning wherever one likes with little regard to what the code is implying -- can sometimes produce torturous code.
I personally *tend* towards a single return, but I wouldn't presume to impose that on others. I like what Fowler has to say about this in his refactoring "Replace Nested Conditional with Guard Clauses" -- that's partly why I suggested reversing the one conditional.
Veering a little off-topic, the JavaRanch style guide -- which we're not following here -- advocates a single return. I found some of the recommendations in this style very odd at first, but I've grown to *really* like this style (except for the insistence on a single return).

[Terry McKee]

This exercise is really great. It is good to see how other developers approach the problem. The reason that I earlier said that I didn't think that using multiple return statements is because after looking at these comments I have to return to the real world where code is pretty much garbage. Where if you can find the start and end of a function you have come further than 95% of the other developers. Adding return statements within the nested If statements that go 14 levels deep has caused me to long for better things. Arrrrgh!
I digress...but anyway...I always thought that it was frowned upon to have multiple return statements, I guess not. I stand corrected.

[Michael Matola]

I have a little familiarity with Smalltalk, but I've shelved studying it seriously until my Java becomes marketable.
I read somewhere that in the core classes of Smalltalk (the parts of the language written in the language itself, much like the Java API), the average length of a method is 7 lines.
(I hope this statistic isn't made up.)
Pretty amazing, no? A far cry from nested ifs 14 levels deep!

[Junilu Lacar]

To emphasize some of the things we've discussed so far:
1. Write TESTS first with the goal of bringing out the "ideal" interface/design.
2. Add stubs to make the tests fail before writing code to make the tests pass.
3. Write quick and dirty first-cut implementations just to make the tests pass.
4. Refactor to eliminate duplication, dependencies and other bad code smells.
5. You don't have to refactor to perfection: do just enough to make the next step obvious or easy to make. The term Alistair Cockburn uses is "sufficiency". "Satisficient" is another term I think I've seen. (Did we actually discuss this? -- oh well, I'll mention it anyway)
6. You don't have to take small steps. Just remember that you can. Set a rhythm that is comfortable. If you waver or become unsure of your footing, take smaller steps.
7. Keep a to-do list of reminders

[Ilja Preuss]

I read somewhere that in the core classes of Smalltalk (the parts of the language written in the language itself, much like the Java API), the average length of a method is 7 lines.
(I hope this statistic isn't made up.)

Well, afaik it isn't.

Pretty amazing, no? A far cry from nested ifs 14 levels deep!
Yes, and it is doable in Java, too (even if it could seem to be otherwise when you take a look at monsters like GregorianCalendar).
Current Metrics of our hobby-project:
99 classes and interfaces
averaging
7.31 methods/class
2.19 statements/method (max. 17)
1.49 block depth (max. 5)

Notice that the stmts/method statistic is not directly comparable to smalltalk because of the interfaces which aren't available/necessary in smalltalk. On the other hand, javas anonymous inner classes are a killer for block depth...
At last, another wiki-link: http://c2.com/cgi/wiki?LotsOfLittleMethods
[ March 28, 2002: Message edited by: Ilja Preuss ]

[Ilja Preuss]

I have now posted my own implementation of OOCalc up to the point where I think I need additional tests.
Comments would be welcome!
Regards, Ilja

[Junilu Lacar]

Step 10
I'll fast forward this a little bit and take a big step in the listings. You'll see that I have implemented subtraction and multiplication with little or no refactoring. The purpose is to make the duplication painfully obvious as to force a refactoring.
Step 10 tests
Step 10 code
We now have a sufficient amount of duplication to see a pattern emerging. It's clear now that the current design will require a change to OOCalc every time a new operation is added, violating the Open/Closed Principle. The first step in eliminating this problem is to move the constants to another class.
We create a new class and call it CalcOp
[ March 29, 2002: Message edited by: Junilu Lacar ]

[Junilu Lacar]

Notice that Ilja has taken a slightly different tack from what I have. He's taking much smaller steps than I am at this point. I suspect that this is because he has much more experience doing this and can recognize certain smells that need refactoring better than I can.
Like I said before, you tend to take smaller steps as you get better at this approach. Beginners tend to take bigger steps because their "odor threshold" is greater than those with a more finely tuned sense for "bad code smells".
Interestingly enough though, I sense that we are heading towards very similar solutions.
Junilu

[David Weitzman]

I went ahead and finished my own implementation of OOCalc, and I'm beginning to realize how test-first programming is significantly different from drawing pretty UML diagrams before you write a line of code.
On the one hand you're aware that if you implement something certain way, it'll be more flexible in the future. On the other hand, you have to fight the urge to add things simply because they might be useful later. Getting the tests to pass is supposed to be quick and dirty, and then by the time you clean it up you have your refactoring hat on and can't get too fancy.
For a small project where getting the initial tests to pass quickly is possible, test-first programming is probably more productive since you always know what to do next. How does test-first programming scale to something more complex though?

[Junilu Lacar]

Originally posted by David Weitzman:
How does test-first programming scale to something more complex though?

In one of his articles on Iterative and Incremental Development, Robert Martin quotes Booch quoting Gall:

"A complex system that works is invariably found to have evolved from a simple system that worked...

So, when we talk about scaling to a more complex problem, we simply fall back to the good old "divide and conquer" approach. Then we do test-first development on the simpler pieces and slowly evolve towards the complex, ruthlessly refactoring along the way. Test-first forces us to start simple while refactoring helps pave the way for evolution and gives courage and capabilities to handle more and more complexity.

[Junilu Lacar]

BTW David,
I don't think that TDD precludes the use of UML models. The thing about UML though is that a lot of folks who start doing it soon forget its main intent--which is to facilitate communication--and instead focus on the UML itself. When that happens the design process soon degenerates into a useless and often drawn-out exercise in futility of trying to create the "correct" model. By referring to UML diagrams as "pretty pictures", I think you have either observed or experienced this first-hand before.
Again, the word "sufficiency" comes to mind. If you need to draw pictures, draw the bare minimum that you need to understand the problem enough and to decide how to make the next move. Then throw the diagram away.

[Junilu Lacar]

Step 11
What to test next? It seems to me that a sequence of additions might be a good thing to try now. So, we add a new test:
<pre>
// 3 + 4 + 5 = 12
public void testAddSequence() {
calc.in(3.0);
calc.in(Addition.getInstance());
calc.in(4.0);
calc.in(Addition.getInstance());
calc.in(5.0);
assertEquals(12.0, calc.getResult(), DELTA);
}
</pre>
We don't have to stub anything because we're not calling any new methods. We compile and test - 1 failure (Red light): "expected <12.0> but was <9.0>".
So, looking into OOCalc, I try a few things to make this work. To make a long story short, I found myself stuck. Everything I did seemed to break one or more tests.
What to do?
I backtracked. Instead of faking it in the production code, I decided to try to faked it in the ideal client code.
<pre>
// 3 + 4 + 5 = 12
public void testAddSequence() {
calc.in(3.0);
calc.in(Addition.getInstance());
calc.in(4.0);
calc.in(calc.getResult());
calc.in(Addition.getInstance());
calc.in(5.0);
assertEquals(12.0, calc.getResult(), DELTA);
}
</pre>
This gave me a green light. Now the problem is how to refactor this and push the code in the test class to the production class.
[ March 29, 2002: Message edited by: Junilu Lacar ]

[David Weitzman]

Object Mentor seems to have a lot of great articles! Thanks a lot for letting me know it existed, Junilu.
Right now I'm plowing through an example there of test first development of a CSV file reader that everyone should be sure to read after they're done with this tutorial.
I'll probably be at that site from dawn 'till dusk.
None of their classes are in my area though .

[Ilja Preuss]

Originally posted by Junilu Lacar:
So, when we talk about scaling to a more complex problem, we simply fall back to the good old "divide and conquer" approach. Then we do test-first development on the simpler pieces and slowly evolve towards the complex, ruthlessly refactoring along the way. Test-first forces us to start simple while refactoring helps pave the way for evolution and gives courage and capabilities to handle more and more complexity.

Yes. I liked what Ron Jeffries recently said on the XP mailing list. It went along the lines of "I never write complex programs - I only write simple programs with complex behaviour."

[Junilu Lacar]

Step 11 (continued)
Coming back to the refactoring of the test, I needed to find a way to push the "fix" from the test to OOCalc. I look at the surrounding code and see that I have two choices:
1. Push the code into in(double)
2. Push the code into in(Operation)
The reason I look at the surrounding code is that in trying to push the code down into OOCalc, I need to make sure that the sequence of method invocations is preserved.
That is, if I push the test code to in(Operation), I have to calculate the intermediate result before whatever is already happening in that method.
On the other hand, if I push the test code to in(double), I have to calculate the intermediate result after whatever is already happening in that method.
Try #1: push the test code to in(double):
<pre>
public void in(double operand) {
operandStack.push(new Double(operand));
in(getResult());
}
</pre>
I notice that this will result in an endless recursion. I compile and test anyway. Yep, I get a StackOverflow exception and break a bunch of tests in the process. I start looking for a condition that can prevent the StackOverflow. Going back to the test code, I see that the intermediate result is calculated after the second operand is passed in. So I add the condition:
<pre>
public void in(double operand) {
operandStack.push(new Double(operand));
if (operandStack.size() >= 2) {
in(getResult());
}
}
</pre>
Compile and test. DOH! Broke a bunch of tests! JUnit shows that I'm getting an EmptyStackException in testAddition(). Oops. The new code results in getResult() getting called twice in a row in testAddition() and the other broken tests.
OK, we'll try choice #2 then: move the code to in(Operation). I won't get recursion then and the condition still holds when we get the second Operation. So I copy the code to in(Operation) and delete it from in(double):
<pre>
public void in(Operation op) {
if (operandStack.size() >= 2) {
in(getResult());
}
_op = op;
}

public void in(double operand) {
operandStack.push(new Double(operand));
if (operandStack.size() >= 2) {
in(getResult());
}
}
</pre>
Compile and test. Green bar! Woo-hoo!
Just to be sure it wasn't just dumb luck on my part I extend the test a bit and add another operand to the sequence. Compile and test. Still Green
Just to make doubly sure, I make another test for a mixed sequence of operations. I don't expect the calc to handle precedence yet so I'll write my tests accordingly:
<pre>
// OOCalcTest.java
...
public void testMixedSequence() {
calc.in(1.0);
calc.in(Addition.getInstance());
calc.in(2.0);
calc.in(Multiplication.getInstance());
calc.in(3.0);
// no precedence logic yet so we won't expect 7.0 ...
assertEquals(9.0, calc.getResult(), DELTA);
}</pre>
Compile and test. Green bar!

[Terry McKee]

I think it would be interesting to use parenthesis in this exercise.

[Michael Matola]

I've caught up, but I have a two questions from a step ago.
1.Regarding how you've implemented the singletons -- Why the lazy initialization of _instance? I'm still a little fuzzy on threads, but doesn't the lazy initialization open up the possibility of multiple "singletons" getting created (or at least I recal reading that somewhere regarding this approach)? I thought the canonical Java singleton was more along the lines of the following:

public class Addition extends Operation
{
    
    private static Addition _instance = new Addition() ;
    
    public static Operation getInstance()
    {
    return _instance ;
    }
    
... etc.
}    

And besides, wouldn't lazy initialization count as a premature optimization, which would violate the principle of always starting with the simplest possible implementation to get the test to pass?

2. Code duplication.
The perform() methods of all subclasses of Operation share the same identical code:
double op1 = ( (Double)operandStack.pop() ).doubleValue() ;
double op2 = ( (Double)operandStack.pop() ).doubleValue() ;
Looks to me that instead of passing the Stack into perform(), we should be passing the doubles that we've already popped.
That change would give us the following perform() signature in Operation:
public abstract double perform( double op1 , double op2 ) ;

A sample implemenation in Addition would be:

    public double perform( double op1 , double op2 )
    {
        return op1 + op2 ;
    }

And the calling code in OOCalc would be:

    public double getResult()
    {
        if ( _op == null ) return 0.0 ;
        double op1 = ( (Double)operandStack.pop() ).doubleValue() ;
        double op2 = ( (Double)operandStack.pop() ).doubleValue() ;
        return _op.perform( op1 , op2 ) ;
    }

And we could also switch the order of assignment while popping:

    public double getResult()
    {
        if ( _op == null ) return 0.0 ;
        double [B]op2[/B] = ( (Double)operandStack.pop() ).doubleValue() ;
        double [B]op1[/B] = ( (Double)operandStack.pop() ).doubleValue() ;
        return _op.perform( op1 , op2 ) ;
    }

So we wouldn't later have to switch the operands in Subtraction and Division.

[Ilja Preuss]

Originally posted by Michael Matola:
2. Code duplication.
The perform() methods of all subclasses of Operation share the same identical code:
double op1 = ( (Double)operandStack.pop() ).doubleValue() ;
double op2 = ( (Double)operandStack.pop() ).doubleValue() ;
Looks to me that instead of passing the Stack into perform(), we should be passing the doubles that we've already popped.
That change would give us the following perform() signature in Operation:
public abstract double perform( double op1 , double op2 ) ;

A sample implemenation in Addition would be:

    public double perform( double op1 , double op2 )
    {
        return op1 + op2 ;
    }

And the calling code in OOCalc would be:

    public double getResult()
    {
        if ( _op == null ) return 0.0 ;
        double op1 = ( (Double)operandStack.pop() ).doubleValue() ;
        double op2 = ( (Double)operandStack.pop() ).doubleValue() ;
        return _op.perform( op1 , op2 ) ;
    }

And we could also switch the order of assignment while popping:

    public double getResult()
    {
        if ( _op == null ) return 0.0 ;
        double [B]op2[/B] = ( (Double)operandStack.pop() ).doubleValue() ;
        double [B]op1[/B] = ( (Double)operandStack.pop() ).doubleValue() ;
        return _op.perform( op1 , op2 ) ;
    }

So we wouldn't later have to switch the operands in Subtraction and Division.

But you just have moved the duplication, not removed it, don't you?
Perhaps the code is asking for an OperandStack class to emerge?
Regards, Ilja

[Ilja Preuss]

Perhaps the code is asking for an OperandStack class to emerge?
Uh, well, let's take smaller steps... :roll:
We can simply extract a method to remove the duplication:
<pre>
private double getDoubleFromStack() {
return ( (Double)operandStack.pop() ).doubleValue() ;
}

double op2 = getDoubleFromStack() ;
double op1 = getDoubleFromStack() ;
</pre>
OK, now we have substituted Duplication - the worst of all code smells - with two more harmless ones: Private Method and Greediness. Both are indicators that the method in fact belongs to an other class - Stack in this case, I would think.
In more dynamic languages (like Smalltalk), we just would move the method to Stack. In Java we can't, so we could either derive an OperandStack (which *might* be overkill for just one method) or live with the "Foreign Method" for the moment.
[ April 03, 2002: Message edited by: Ilja Preuss ]

[Junilu Lacar]

Guys, thanks for keeping the discussion going. I'm doing my tax returns this week so I won't have a lot to add until the weekend maybe. In the meantime, keep exploring your own refactorings and extensions.
It's good that you're seeing more ways to refactor. Just remember that you should refactor one thing at a time. It seemed like sound advice when we heard it back then but we have to realize that trying to "do it all right the first time" is hard and aiming to do that sometimes sets us up for failure. Our approach will be more like doing spring cleaning: Move the mess around a bit and clean up. Repeat until there's no more mess to move.
That reminds me: I have to do some spring cleaning at home too :roll: ... see you guys in a few days!
Junilu
[ April 03, 2002: Message edited by: Junilu Lacar ]

[Michael Matola]

Originally posted by Ilja Preuss:
But you just have moved the duplication, not removed it, don't you?

You and I are talking about different duplications, Ilja.
I wasn't complaining about the fact that these two lines
double op1 = ( (Double)operandStack.pop() ).doubleValue() ;
double op2 = ( (Double)operandStack.pop() ).doubleValue() ;
between them share the same code for popping, converting, and casting. (You've shown that there's probably not much to gain by trying to remove that duplication.)
I was complaining about the fact that these same two lines of code appear in four places, namely in the perfom() method of all four subclasses of Operation (Addition, Subtraction, Multiplication, Division).
Take a look at those perform() methods: all four of them first get the doubles from the stack, then perform the actual arithmetic. What I'm suggesting is that the code to get the doubles from the stack be moved elsewhere.
My above suggestion was to move that code into to the client method OOCalc's getResult() (which would involve a change in interface: perform() would accept two doubles as parameters instead of the stack).
Another possibility would be to refactor the common double-getting code from the various perform()s into the superclass Operation, with perform(Stack operandStack) becoming a concrete method that first gets the doubles, then calls the abstract perform( double operand1 , double operand2) which subclasses implement. (Vaguely template-methodesque.)
Hope I've clarified the point I was trying to make.

[Ilja Preuss]

Originally posted by Michael Matola:
You and I are talking about different duplications, Ilja.

I see... :roll:

I wasn't complaining about the fact that these two lines
double op1 = ( (Double)operandStack.pop() ).doubleValue() ;
double op2 = ( (Double)operandStack.pop() ).doubleValue() ;
between them share the same code for popping, converting, and casting. (You've shown that there's probably not much to gain by trying to remove that duplication.)

I don't think I have shown that. I am quite positive that we *should* remove it (well, *any* duplication). I am just not as fanatic about where to put the resulting method...

Take a look at those perform() methods: all four of them first get the doubles from the stack, then perform the actual arithmetic. What I'm suggesting is that the code to get the doubles from the stack be moved elsewhere.
My above suggestion was to move that code into to the client method OOCalc's getResult() (which would involve a change in interface: perform() would accept two doubles as parameters instead of the stack).

Yes, I think this is a good step.

Another possibility would be to refactor the common double-getting code from the various perform()s into the superclass Operation, with perform(Stack operandStack) becoming a concrete method that first gets the doubles, then calls the abstract perform( double operand1 , double operand2) which subclasses implement. (Vaguely template-methodesque.)

I don't like this. I don't think about an Operand operating on a Stack, I think about it operating on two operands. YMMV.
Regards, Ilja

[Junilu Lacar]

OK, so we seem to be at odds here about who has what responsibility. Here's my take on it:
Class: Operation
-------------------
Responsibilities:
- Knows how to perform calculation and produce a result
- (therefore it) Knows how many operands it needs to perform calculation
Collaborates with:
- (Whoever has the set of operands... OOCalc or Stack?)

Class: Calc
-------------------
Reponsibilities:
- Know operands and operators entered
- Know Operations available
- Control sequence of performing Operations
- Keep results of performing Operations
Collaborates with:
- Operation

Sometimes you can't help but think ahead and I have to admit that I was already thinking ahead to when we'd need single-value operations like %, 1/x, x^2, +/-, etc.
So in order for OOCalc to call Operation polymorphically, the easiest thing to do that I can think of would be to have OOCalc pass something (the Stack) from which an Operation can just take as many operands as it needs.
The alternative is to have OOCalc ask the Operation how many operands it needs then pass those operands to the appropriate overloaded perform(). But that smells: We'd have to write a case or if statement to see which one to call.
I like Ilja's idea of a wrapper class for the stack so we can simplify the client code and do away with all the ((double)stack.pop()).doubleValue() code.
BTW, ObjectsByDesign has a new project that's very similar to this but they're not doing it test-first: check out their Object-Oriented Calculator http://www.objectsbydesign.com/projects/calc/overview.html
Sorry to have let this thread lose some steam -- what with having to review some books for the bunkhouse, work my day job, do my taxes, mail in rebates, do spring cleaning, deal with allergies, and making sure these new bartenders got in...( :roll: excuses, excuses...)
Junilu

[Sudharsan Govindarajan]

Junilu!
Hats Off! This thread continues to be a great wealth of Info apart from learning jUnit. I've bookmarked it in my browser and watching it.
thanks
Sudharsan