Using TDD and trying to make a simple calculator.

[Junilu Lacar]

Note the use of final in the code. That is purposeful design. With this, I'm saying that I'm purposely limiting the ability to extend whatever has been marked as final. The apply method in BinaryOperation has been marked final because I don't want any subclasses to change this behavior. The apply method is a Template Method.

[Junilu Lacar]

Given that the operation is given the responsibility of determining whether there are enough operands it needs to perform (check the spelling in your code!) the calculation, can you find some places in your code where that responsibility has been improperly assigned? Only the Operation class should be checking if it has as many parameters as it needs.

Edit: Retrospecting on this a little bit, think about how the refactoring to extract a formula to the hasEnoughParameters() method allows you to realize that there is mis-assigned responsibility in your code. Would you have been able to see that otherwise? Does this show how good code leads to even better code? And how bad code can hide problems and that you need to refactor ruthlessly to eliminate bad code and bring out the goodness in the program?

In Aikido, this is part of shugyo

[Junilu Lacar]

Note also the comment that I added and how I changed it after extracting to a superclass. This is an example of one of the rare occasions where I would be compelled to write a JavaDoc comment up front, while the idea is still fresh in my mind.

Edit: Also note that I tried to avoid putting too much information about the implementation in the JavaDoc comment and that I marked part of the comment as an "Implementation Note" -- this is a common convention for JavaDoc when you are commenting about an implementation-specific detail. I did, however, leave out any mention of a Stack being involved.

See "How to Write JavaDoc comments"

I'm pretty sure I skipped right to the "pretty" code here. An alternate route might have been to eliminate the temporary variables in the template method and switch the order around in the individual calculate() methods. That, however, would not be as symmetrical.

//
// Alternate Route: (not as pretty -- I would eventually revert this)
//

public abstract class BinaryOperation {
    public final void apply(Stack stack) {
         if (hasEnoughOperands(stack) {
            stack.push(calculate(stack.pop(), stack.pop()));
         }
    }
    ...
}

public final class Addition ... {
    @Override 
    protected final double calculate(double op1, double op2) {
        return op1 + op2;
    }
}

public final class Subtraction ... {
    @Override 
    protected final double calculate(double op1, double op2) {
        return op2 - op1;
    }
}

...

You would probably do more refactoring to try to get more symmetry but it becomes a whack-a-mole problem.

By keeping the temporary variables in the apply() method, the Template Method can lock down the ordering of the operands and you can rename the parameters for calculate() accordingly. Doing this actually justifies having the two lines of code for temporary variables used to store the values popped from the stack. These two lines of code are needed for the program to work properly and you should have a test that will fail if another programmer comes in later and naively tries to "clean up" the code by eliminating the temporary variables.

[Junilu Lacar]

There is still another refactoring opportunity. See if you can find the Arrow Code antipattern in the code. It's a small "arrow" but you can still get rid of it. I would actually use a test for the error condition of not having enough operands on the stack to drive me to refactor out the Arrow Code.

See also, Introduce Guard Clause

[Michael Bruce Allen]

Junilu Lacar wrote:

Michael Bruce Allen wrote:Well, I did some searching around and learned of another way to do this. I like this way much better because it is more clear.

The thing is, you didn't really need to look anywhere outside for that. It was right there in the code. But I guess that recognition comes with experience.[/Quot]

Believe me man, I would have had to ask lots of questions before figuring this out. I didn't even really understand what an abstract method was until now.

Junilu Lacar wrote:
You took a distracting detour in making that method take a stack and two doubles. I guess that can happen though. I would have reverted when I saw how ugly it was instead of forging ahead from there. I'm trying to follow your commits and it seems you jumped the gun there somewhere and missed out on a few critical small steps in refactoring. The end result is almost the same but in this case, the means is just as important as the end.
...
I immediately see a pattern repeated in this code. A pattern indicates that there is an opportunity to generalize by extracting similarities and parameterizing differences. The first thing I see is that formula, stack.size() > 1.
...
I ask "What does that formula mean? What is the essence of it?" -- Answer: "I need two operands to perform this operation, so I'm checking that there are at least two operands available to pop from the stack."

https://github.com/CodeAmend/calculator/blob/temp/src/calculator/functions/basic/OperandStack.java
Within my temp branch the other day, I did this very thing. I called it stackHasEnough() lol

    private boolean stackHasEnough() {
        if (numberStack.size() > 1)
            return true;
        return false;
    }

    public void add() {
        if(stackHasEnough()) {
            double op1 = numberStack.pop();
            double op2 = numberStack.pop();
            this.numberStack.push(op1 + op2);
        }

    }

I remembered doing this before, but I felt I didnt like the code. One thing I did not realize is the importance of code clarity. I thought I understood, but because I wanted to change this, I never refactored it (this time), If I would have just renamed it, I might have gotten a better idea.

[Michael Bruce Allen]

Junilu Lacar wrote:Next up, separation of concerns/responsibility. Obtaining the operands is a separate concern from using the operands. I'll separate these two a little bit:

...

The other operation classes would undergo the same transformation. After that, there is still a lot of duplication. This is where I see that a Binary operation is a generalization of all four operation classes. So I extract to a superclass:

...

There are even more opportunities to make this code cleaner by using some of the language constructs introduced in Java 8 but you can look into that later.

I like this, but what makes this a refactoring focus? What principal? I mean, they were all fine before, they were able to access the abstract method just the same. Now this cleans up all the class files, but why would you have a class within a class (I hope you don't think I am arguing here, I like this a lot, I just want to know the driving force)?

[Michael Bruce Allen]

Junilu Lacar wrote:
...
Edit: Retrospecting on this a little bit, think about how the refactoring to extract a formula to the hasEnoughParameters() method allows you to realize that there is mis-assigned responsibility in your code. Would you have been able to see that otherwise? Does this show how good code leads to even better code? And how bad code can hide problems and that you need to refactor ruthlessly to eliminate bad code and bring out the goodness in the program?

In Aikido, this is part of shugyo

This is awesome and I will take this very seriously from now on.

[Junilu Lacar]

Michael Bruce Allen wrote:I like this, but what makes this a refactoring focus? What principal?

It's a refactoring focus because I didn't change the tests or add any new ones. I'm changing the code around without changing the expectations for its externally observable behavior. I'm trying to improve the design of the software without changing it's behavior (the canonical definition of "refactoring")

Michael Bruce Allen wrote:I mean, they were all fine before, they were able to access the abstract method just the same. Now this cleans up all the class files, but why would you have a class within a class

I supposed it's a case of beauty being in the eye of the beholder but for me, that code was not really "fine before" -- there was something about it that was offensive to my sense of good code.

I'm not sure I understand what you're referring to as a "class within a class" -- those are all separate classes. If you're talking about the hierarchy of Operation -> BinaryOperation -> Addition|Subtraction|Multiplication|Division, then it's really about SOLID design principles. The L, in particular, which stands for Liskov Substitution Principle, has to do with Polymorphism and guides proper design for inheritance. It could have been something to do with the Law of Demeter, or GRASP. I really should take more time to try to explain my motivations to myself but a lot of the time, I just look at code and say "That's not quite right, let's see if this makes it better..." and then I go refactor. The only refactorings I can clearly express my motivations for doing are the three basic ones I already mentioned before:

1. Rename - clarify intent
2. Extract - remove duplication / clarify intent / generalize & parameterize
3. Compose method - single level of abstraction, separation of concerns, hiding details, clarifying intent

I think that these help mitigate or eliminate 80% of the problems you'll find in poorly written code.

[Junilu Lacar]

Michael Bruce Allen wrote:
https://github.com/CodeAmend/calculator/blob/temp/src/calculator/functions/basic/OperandStack.java
Within my temp branch the other day, I did this very thing. I called it stackHasEnough() lol

    private boolean stackHasEnough() {
        if (numberStack.size() > 1)
            return true;
        return false;
    }

You have to try to stop writing code like that. This is all you need. And that code you have is in the wrong place. It's a mis-assigned responsibility because that rule only applies to binary operations.

    private boolean stackHasEnough() {
        return numberStack.size() > 1;
    }

[Junilu Lacar]

So here's a nuance that you may not have been able to discern:

    private boolean stackHasEnough() {
        return numberStack.size() > 1;
    }

versus

    private boolean hasEnoughOperands(Stack<Double> stack) {
        return stack.size() > 1;
    }

Visually, the top snippet looks like it might be cleaner. There's no parameter and there's arguably less code.

However, the phrase "stack has enough" references something that's not in the immediate vicinity of the method in the first snippet, namely, the stack. Your eyes have to scan the rest of the class to find where the stack is declared and initialized. Also, it hides some of its intent by implying what is being checked for sufficiency. What does the stack have enough of? You automatically fill in "Operands" at the end because you've been working on this problem for a while and you know how it works. But anyone who is unfamiliar with the inner workings of this program will have to pause, even for just a half a second, to back away and figure out that you have an implied "Operands" there. It's a small smell, but my nose is very sensitive to these things so I change it when I smell it.

On the other hand, the second version is very well-contained. The only thing that is referenced inside the method is the parameter, honoring the Law of Demeter. Well, there's the constant value 1, too, and I suppose you could argue than you should clarify the intent of 1 by replacing it with a symbolic name, perhaps like so:

    private static final int NOT_ENOUGH_OPERANDS = 1;

    private boolean hasEnoughOperands(Stack<Double> stack) {
        return stack.size() > NOT_ENOUGH_OPERANDS;
    }

I guess I'd be fine with something like that if my partner really feels strongly about doing that refactoring.

[Michael Bruce Allen]

Stephan van Hulst wrote:
@Before
public void setUp() {
this .calculation = new ReversePolishCalculation();
super.calculation = this.calculation;
}

// Tests specific to ReversePolishCalculation.

}[/code]

What is this super.calculation = this.calculation?
What why the need to call super? What is going on here?

[Junilu Lacar]

Michael Bruce Allen wrote:
What is this super.calculation = this.calculation?
What why the need to call super? What is going on here?

In Stephan's example, he defines a superclass to hold the tests for all Calculation objects in general. These tests should hold true for any subclass of Calculation as well. That code is making sure that the general tests that use the calculation reference declared in the superclass is actually referring to an object, which in this case is the ReversePolishNotationCalculation object created in the subclass.

I don't see a good path to this kind of code if I were doing TDD, not with just the code given. I would need to see two tests for different implementations of Calculation, maybe an RPNCalculationTest and an InfixCalculationTest. Then I'd have to see similarities between the two that would motivate me to pull up the similar tests/behavior into a common superclass and make the two original ones subclasses of it. Pulling up methods to a superclass without seeing actual duplication seems highly speculative to me, something that I try to avoid as much as possible. I have to at least see the duplication in the code before I will refactor it away. I try not to preempt the duplication. Just let it happen, recognize it, then deal with it if it's really that bad.

[Junilu Lacar]

    private static final int NOT_ENOUGH_OPERANDS = 1;

    private boolean hasEnoughOperands(Stack<Double> stack) {
        return stack.size() > NOT_ENOUGH_OPERANDS;
    }

That code might actually be enough to make me start entertaining the idea of bringing back the OperandStack class or something very similar to it. I might experiment with:

---------------
public interface Operation {
     ...
+    public int operandsNeeded();

---------------
public abstract BinaryOperation implements Operation {

+    @Override
+    public final int operandsNeeded() {
+        return 2;
+    }

     ...

-    public void apply(Stack<Double> stack) {
+    public void apply(OperandStore operandStore) {

-        if (hasEnoughOperands(stack)) {
+        if (operandStore.hasEnoughOperandsFor(this)) {
         ...
     } 

---------------

/** Keeps the operands to be used for calculation */
public class OperandStore {

     private Stack<Double> stack = new Stack<>();
     ...

     public boolean hasEnoughOperandsFor(Operation op) {
         return this.stack.size() >= op.operandsNeeded();
     }  

     // other methods that hide the fact that we're really using a Stack
     ...

Again, I'd be very careful in actually doing this though and use tests to make me feel a strong motivation to do it. One way might be if you want to have an OperandStore that's easier to programmatically set up in a test environment versus one that would take values after parsing a String, as you would when actually running the program. The caveat here is that making your code flexible to handle different sources for input adds one or more layers of abstraction and complexity.

The conversation might go something like this:

Don't you think defining that constant is too much, seeing that it's really used in only one place?

What about if we make the method explain what the value is?

Oh, like have something like public int operandsNeeded() { return 2; }?

Yeah! The method name reveals the intent - "this method tells how many operands this operation needs" and the method body simply gives the literal value. That's clear enough, right? And we got rid of the noise created by the single-use constant.

Ok, sounds reasonable but where would we put that?

Well, who needs to know this and when?

We have this guard clause that checks if there are enough operands, right? We can change that a little bit ...

... and so on

See how all I needed to make the code potentially read better was to rename and move things around?

But then again, the above conversation could be nipped in the bud by someone saying:

"That's getting way too complicated too fast. Let's slow down and take smaller steps."

The options are all there for you to choose.

I kind of like how that code is shaping up though. It seems a lot clearer to me and the responsibilities are falling in place rather nicely.

[Stephan van Hulst]

Junilu Lacar wrote:

Michael Bruce Allen wrote:
What is this super.calculation = this.calculation?
What why the need to call super? What is going on here?

In Stephan's example, he defines a superclass to hold the tests for all Calculation objects in general. These tests should hold true for any subclass of Calculation as well. That code is making sure that the general tests that use the calculation reference declared in the superclass is actually referring to an object, which in this case is the ReversePolishNotationCalculation object created in the subclass.

I don't see a good path to this kind of code if I were doing TDD, not with just the code given. I would need to see two tests for different implementations of Calculation, maybe an RPNCalculationTest and an InfixCalculationTest. Then I'd have to see similarities between the two that would motivate me to pull up the similar tests/behavior into a common superclass and make the two original ones subclasses of it. Pulling up methods to a superclass without seeing actual duplication seems highly speculative to me, something that I try to avoid as much as possible. I have to at least see the duplication in the code before I will refactor it away. I try not to preempt the duplication. Just let it happen, recognize it, then deal with it if it's really that bad.

To make more concrete what Junilu said: I initially envisioned a calculation working with expression objects, and saw some parallels between different kinds of calculators. As mentioned before, I was way ahead of things, and approaching it this way is a mistake.

[Junilu Lacar]

Stephan van Hulst wrote:I was way ahead of things, and approaching it this way is a mistake.

Stephan, I wouldn't go that far. To call the approach a "mistake" is too harsh. As your tagline says, the mind is strange and wonderful thing and while starting from a high-level concept like that may not be ideal for many people, who's to say it won't work for others?

It could be an entirely valid idea for a design and I wouldn't be surprised if, in one way or another, we'd arrive at a very similar solution. However, in the context of doing TDD, it's an idea that I'd be inclined to keep in the back of my mind to serve as a possible goal rather than a foundation to build upon. Kind of like the difference between a lighthouse and an outboard motor. One guides you to your destination, the other helps drive you there.

And honestly, I do have kind of a vague picture in my head of what this calculator would probably look like in the end. It's a picture that's drawn from past experience with this kind of problem. It's vague but there nonetheless and I keep it in the periphery of my mind's eye so I can sense when I might be falling off the edge unto the rough shoulder.

[Michael Bruce Allen]

I seem to have this disconnect with what the end goal is. What is the end goal? At first it was to read a string and convert to postfix. Then it was a simple rpn calculator. Now I just do not know what.

What I would like to do is, have a solid goal. Perhaps just build a simple RPN calc to get finished. Then ill start another project that has a more advanced goal. Right now if this is just an rpn calculator, then what realy do we have left other than just preempt a few possible bugs?

[Michael Bruce Allen]

public void applyTo(Stack<Double> stack) {
        if(hasEnoughOperands(stack))
            stack.push(calculate(stack.pop(), stack.pop()));
    }

When looking at that code, is it better to say stack.hasEnoughOperands()?? If this is the case, then the NumberStack class was not such a bad thing. I know you mentioned it recently, but what was your motivation?

[Michael Bruce Allen]

I decided to go at the calculator from the beginning today and see what I come up with. I really like the idea that I came up with today.

perform(Operation) can except new Addition(), new Subtraction(), and new Number(double)

I am unsure of the interface name Operation since a number is an operand. So any ideas on a name here?
Also, it seemed after I made the subtraction() method, i refactored constantly. I was unsure if I was supposed to be writing tests. But really it was for the same thing, the tests still worked after the refactor look at recent test below, and this is where I stopped trying to write tests and I just kept refactoring. What do you think of this?

    @Test
    public void operation_object_add_top_of_stack() {
        input(3.0, 6.0);
        //calc.perform(new Addition());
        //above method added to add()
        calc.add();
        assertEquals(9.0, calc.accumulator(), TOLERANCE);
    }

My commits
https://github.com/CodeAmend/calculator/commits/new-skills

public class Calculator {

    Stack<Double> stack = new Stack<>();

    public double accumulator() {
        if(stack.size() > 0)
            return stack.peek();
        return 0;
    }

    public void input(double value) {
        perform(new Number(value));
    }

    public void add() {
        perform(new Addition());
    }

    public void subtract() {
        perform(new Subtraction());

    }

    public void perform(Operation operation) {
        operation.applyTo(stack);
    }

}

Number class looks like this...

public class Number implements Operation {

    private double number;

    Number(double number) {
        this.number = number;
    }

    @Override
    public void applyTo(Stack<Double> stack) {
        stack.push(number);
    }
}

[Michael Bruce Allen]

I am now going through a course on Generics and next on Tree Data Structures and recursion. So I imagine I am going to be way better at understanding how to handle a lot of this calculator code-wise.

[Stephan van Hulst]

I'm not a fan of making a Number an Operation. It doesn't matter if your implementation knows how to apply itself to a stack, conceptually, a number just isn't an operation. For it to make sense, it should be named OperandPush or something like that.

Good luck with your courses! Generics are a lot of fun (to me anyway!) and recursion is just plain AWESOME when you get the hang of it.

Don't let me overwhelm you (yet again), but once you get the hang of trees and recursion, you can play around with Expression trees for a possible implementation of an infix notation calculator.

[Michael Bruce Allen]

No, you are not overwhelming me. I appreciate ALL the insight. The courses are great so far. I have a question on Generics already
I posted it in the Java In General

[Michael Bruce Allen]

Junilu Lacar wrote:

Michael Bruce Allen wrote:There is something really bothering me: Double
I do not like using this for the calculator, it seems too defined. I have heard of BigDecimal and BigInteger and thought about making a Number class. There needs to be an easy way to convert these to a string if needed as well. It should be able to take a number as large as I want. I do not know how to test for this though, since adding more too many numbers results in compiling error. This is where I am unsure.

As for the Double type, I need to see code that says Double is not appropriate, code that's in the form of a failing test.

This is where I have a problem testing. If the number is too large it will just have a compiling error. So I am not sure how to test it.