• Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other Pie Elite all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Jeanne Boyarsky
  • Ron McLeod
  • Paul Clapham
  • Liutauras Vilda
Sheriffs:
  • paul wheaton
  • Rob Spoor
  • Devaka Cooray
Saloon Keepers:
  • Stephan van Hulst
  • Tim Holloway
  • Carey Brown
  • Frits Walraven
  • Tim Moores
Bartenders:
  • Mikalai Zaikin

Using TDD and trying to make a simple calculator.

 
Ranch Hand
Posts: 87
2
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Junilu Lacar wrote:I'll give you a nudge in the direction of refactoring though. All those calc.push() calls in the tests are starting to get noisy. That's a code smell. You have to pay attention to the cleanliness of your test code as well. Think of a way to cut that noise down by boiling down those calls to their essence. What is really happening when we call calc.push() multiple times in row? Talk it out with yourself. Try to explain it to yourself in more abstract terms. The refactoring I'm thinking about is "Extract Method" and perhaps a few renames.



Interesting enough, I thought of this also. I was thinking too much push push push. But if I didnt do that, I could send an list of number, OR some sort of constructor that excepted multiple perimeters. I will give this some thought. Here is where I am right now. I wanted to throw an exception and return an error. But I do not know how to do this, so this is the next question:

.

Even though, I feel that getting rid of the multiple push methods used in testing is more important. I wanted to ask before I forgot.
My current commits...
https://github.com/CodeAmend/calculator/commits/temp

mainly because of this lots_of_addition() test... I want to fix this push so I will think of this now...
 
Michael Bruce Allen
Ranch Hand
Posts: 87
2
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Im not going to begin to think this is anywhere near what you are talking about, but I tried something... and I came up with now questions...


I have a good sense this is not what you are talking about, but I am still curious. When I created a doubleArray in the test class it was fine. When I tried to instantiate the already declared private double[] within setUp() it said Array initializer is not allowed here. Why?

 
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
You don't have to explicitly call the method that you annotate with @Before. This happens behind the scenes when you run JUnit. Remove all calls to setUp() in the @Test methods.
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I'm not sure what you were trying to do with doubleArray[] but the code looks more confusing rather than clearer.

Know what your goal is for refactoring. There's a discussion that I imagined should revolve around those multiple push() calls that goes something like this:

All these calls to push() seems to be making the test code really busy. Seems like a lot of noisy details are drowning out what the test really wants to say. Kind of like not being able to see the forest because of the trees.

But those are the discrete calls that the client code would make to the calculator, aren't they? Does it make sense to lump them all together with an abstraction?

Well, let's try it and see if it makes sense. That's the only way to really tell.

One thing that's bugging me is the name push(). It hints too much at a stack in the implementation. Would it make sense to hide the fact that there's a stack involved?

What other term might we use then?

Not really sure but let's just try one that doesn't leak the fact that we're using a stack.

Ok, how about this:

Does that look like reasonable client code though? Would we really lump all the operands together like that?

I don't know, but it seems plausible. It sure makes the test code cleaner though, don't you think? Let's just go with it for now. If anything, we can add a comment for the benefit of other developers coming into this code so they can understand our motivation for refactoring the test code this way.

Yeah, but does anybody besides the original author really bother to read comments?

Well, at least we did our due diligence.
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Be suspicious of the code and tests when a new test passes right off the bat. Remember that you want to see it fail first. There are two possible reasons a new test doesn't fail:

1. Both the program code and the test code are correct.
2. Both the program code and the test code are incorrect in the same way.

The first case is obvious so let me just explain the second case. If your program produces the incorrect result of 7.0 but your test expects it to return 7.0, then both the program and the test are wrong. They just happen to be wrong in the same way. Either way, you can try changing either the test or the program code to see it fail. That is, you force a temporary imbalance between the test code and the program code to get the red bar. Then revert your change so you see the green bar again. I often do this as a sanity check when I don't see a red bar first. I'll usually change the test, so that it makes the wrong assertion, like intentionally using assertFalse instead of assertTrue just to see the red bar. Alternatively, you could purposely introduce a bug in the program code to force it to fail the new test case. Then revert to take out the bug and see the green bar again.

The other thing that an immediate green bar might be telling you is that your new test is redundant, that another test already covers what this new test is covering. Watch for this one, too, since redundant test code is a form of duplication that you should eliminate.
 
Michael Bruce Allen
Ranch Hand
Posts: 87
2
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I guess I didnt know what you were asking. Cleaning up the test code and creating the operand method in the test code! That's pretty cool. I just learned a ton again. This forum is sky rocketing my learning!


I guess now, what is next, we know we can add a huge number of operands, so I can make multiplication, subtraction, and division next. That would be easy and a reasonable movement - unless the having methods that call these are not what I want. My next questions are really how to think about this. There are so many ways. It would just be nice to write some code that I can drop in the forum and you guys say, "exactly!", I know that is a bit far fetched right now. What should I think of next? What should be my next questions beings that subtract, multiply, and divide are easy implementations??? That could waste a lot of time making those if that is not the right direction.

My ideas next are...
-try to throw an error by calling more add methods.
-figure out if methods are what I want to do, like should these be a subclass of a higher abstraction? Can the operators be a subclass of a higher class as well?
-create the subtract multiply divide methods.

but I am just not sure...

I think I will clarify the project right now. I want it to be somewhat simple, so. It will be a RPN calculator that shows 10 levels deep of the stack. GUI simple with add subtract multiply divide. Keep it simple for my sanity. I think my goal was too much at first. I can always start another bigger project. Next I can make a algebraic version.




 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Michael Bruce Allen wrote:I guess I didnt know what you were asking. Cleaning up the test code and creating the operand method in the test code!


I wouldn't even stop there. Remember, these changes were made during the refactoring step. You should ruthlessly refactor. Extracting all those push calls to an operands() method was a good start but I would at least question if it was enough. Always ask "Is there a better name for this?" when you extract, rename, or change the level of abstraction. Put yourself in someone else's shoes and imagine you're coming in to the code for the very first time. Does that name make sense? Does it help convey the idea behind the code well?

Again, talking it out can help you find new names to try. Try explaining what the test is doing in non-technical or high-level terms. Instead of "We're pushing all these operands into the stack" you might dumb it down a little and say "We're keeping track of all these operands" Why? "So we can use them later when we need to perform an operation."

Ok, then based on that little exchange, would it make sense to rename from operands() to keepOperands() or trackOperands()?

Then you go and try them out in the code and see what it looks like. If you don't like it, then revert or ask the questions again and try to find another name.

Michael Bruce Allen wrote:That's pretty cool. I just learned a ton again. This forum is sky rocketing my learning!


This is a lot of fun for me, too, actually. I'm glad you're learning more things.
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Michael Bruce Allen wrote:
My ideas next are...
-try to throw an error by calling more add methods.
-figure out if methods are what I want to do, like should these be a subclass of a higher abstraction? Can the operators be a subclass of a higher class as well?
-create the subtract multiply divide methods.


These are all roads leading to the same destination so it might not matter much. Unlike roads, however, it's easy enough to backtrack and choose a different route when you're dealing with code. As you gain experience, you'll find that you develop a sense for which route might lead you to more architecturally significant decisions. My sense is that the first and second options would do that more than the third one.

IMO, it's generally better to address architecturally significant decisions earlier rather than later. Architecture is something that's foundational and difficult to change later. On the other hand, you also want to keep your options open for as long as possible. You don't want to commit to a complex architecture too early because nothing begets ugly code faster than complexity. Complexity is like a pair of bunnies, if you're not careful it can breed quickly and overwhelm your software.

One of the keys to flexibility is simplicity. Simpler designs tend to be easier to change than complex designs. But trying to make your design flexible tends to increase its complexity. To make a flexible design, you need to introduce abstractions. Each abstraction adds a layer of complexity. We saw this in Stephan's code earlier, when he suggested introducing some interfaces. Those seemed like good abstractions but they also entailed a big jump in complexity, a jump that you weren't ready to make at that point. We may still end up doing what Stephan suggested but we can't just go from first base straight to home plate, we need to touch all the other bases first on the way there.

This tension between simplicity, flexibility, and complexity seems like a Catch-22 situation but I find that focusing on simplicity first allows you to get on the virtuous side of this cycle. Keeping the design simple helps you keep more options open. It gives you time and opportunity to try different things. Once you've explored a number of options, it becomes easier to see where it makes more sense to commit to adding useful abstractions.
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

More about ruthless refactoring. It may not be clear to someone reading this test why there are two assertions. An astute programmer might figure out that the first assertion is about the expected intermediate value obtained from the first add() operation. However, that's highly subjective and relies on the reader having the ability to recognize that.

The question(s) you should ask when you see this code should be something like, "Why are there two assertions? Does the test clearly state the reason for having two assertions? Is there a hidden goal or assumption there? How can we make the test more explicit?"

To lighten the cognitive load and clarify the test's intent, you might refactor that test like so:

Note the whitespace added in the tests. This divides each test into three sections: Arrange, Act, Assert. In the "arrange" section, you set up the conditions needed for the test. The statements in the "act" section exercise the behavior being tested. The "assert" part is self-explanatory.
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
And even more ruthless refactoring:

The phrase "two most recent" implied that the list of operands was being processed in a certain way, with the last two being the "most recent". That's what's called a "leaky abstraction". I think "last two" is less leaky and more intuitive.

I know the two phrases mean pretty much the same thing but think about it: Given a list of numbers, which is clearer and more natural:

"Use the two most recent numbers in that list" or "Use the last two numbers in that list" ?
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Now I'm just being Genghis Khan ruthless:

And that's almost a literal translation of the code that's in the test. When I see that, I say "That'll do. Yup, that'll do."
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
And, of course, for symmetry, you have to do this:
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
What I've done above is basically elevate the unit test to a level of abstraction that approaches that of an acceptance test. There are acceptance test frameworks, like Cucumber, out there that allow you to write something similar to this:

I doubt this is exactly how you'd write in Cucumber but that's kind of the idea for the level of abstraction you want for acceptance tests, or as your mentor might call it, a "big integration test"
 
Michael Bruce Allen
Ranch Hand
Posts: 87
2
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Just a shot here, but can you give me a little line of code and ask me to refactor it? I just want to exercise a bit of what I am learning on something a little more static than my ever changing calc app.


Another question: I believe it was you that told me about Finite state machine and calculator. I am wondering if there should be some states even with this calculator. Lets say I have operands(8.0,4.0) and call add() add() the second add should be in a state because there is only one operand at that time. Perhaps add() can have a while(numberStack.length > 1) { // add code} But this isnt really a state. But that is what made me think of this. I am not sure how many states an RPN calculator has, probably not as many as a infix calculator.


The name operands doesn't really bother me. Besides it doesn't really do anything other then store a list of operands and calls the push function for each double. Why would I need to rename it?


 
Michael Bruce Allen
Ranch Hand
Posts: 87
2
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator



How do I test this? It will throw an EmptyStackException and it DOES fail with junit, but I never added an assert-type method. Just wondering the proper way to do this.
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Search for JUnit idiom for testing exceptions -- I'm going to be quiet for a while -- have some urgent things to finish at my day job. Keep on plugging away at it. You're doing great.

Also, for refactoring exercises, try searching for Refactoring Exercises, or Refactoring Dojo
 
Michael Bruce Allen
Ranch Hand
Posts: 87
2
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator


After I wrote this first test, the next test passed automatically. You mentioned that I should watch out for that. This makes me wonder what is wrong with this? It seems reasonable to me. I tested it because I had this feeling that numberStack.size() would be undefined if no operands was given. But this is more from lack of experience. I imagine. I guess it makes sense that once instantiated it would be size of 0.
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Michael Bruce Allen wrote:
Another question: I believe it was you that told me about Finite state machine and calculator. I am wondering if there should be some states even with this calculator. Lets say I have operands(8.0,4.0) and call add() add() the second add should be in a state because there is only one operand at that time. Perhaps add() can have a while(numberStack.length > 1) { // add code} But this isnt really a state. But that is what made me think of this. I am not sure how many states an RPN calculator has, probably not as many as a infix calculator.


Don't get confused with the abstractions we introduced to the tests and the actual implementation of the calculator logic. We just abstracted the test to bring out the intent. Each of those calls that we abstracted away will still be discrete events in the real implementation of the calculator program. There are just a few flow control structures that you need to fill in. This is where the state machine comes into play. Right now, you are just simulating the operation of the state machine in your tests with a specific sequence of calls you made to the calculator methods.

The thought process here is: "Let me pretend I'm the state machine. When I work with the calculator, I will do this, this, then this. Then when I check the calculator result, I should get this value." What the test code does is it gives an example of how client code would collaborate with the calculator object. You just have to use your imagination a little bit. Again, we're ignoring some details so that we can focus solely on the calculator behavior and less on the client code's behavior.
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Michael Bruce Allen wrote:
After I wrote this first test, the next test passed automatically. You mentioned that I should watch out for that. This makes me wonder what is wrong with this? It seems reasonable to me. I tested it because I had this feeling that numberStack.size() would be undefined if no operands was given. But this is more from lack of experience. I imagine. I guess it makes sense that once instantiated it would be size of 0.



Those tests don't have assertions. That's why they passed immediately. That's another reason tests will pass immediately: it's not asserting anything so JUnit assumes the best. Technically speaking, the test passes because no AssertionException was thrown.

This test will pass:

The comment doesn't help much and you can easily miss taking care of this. Some people will do this instead:

There are many who will question the practice of using fail for unimplemented JUnit tests.

If the failing test becomes an annoyance, you can tell JUnit to ignore it:

There is an @PendingImplementation annotation for JUnit provided by a third-party library. See https://github.com/ttsui/pending for more info. I haven't used this but it looks promising.
 
Michael Bruce Allen
Ranch Hand
Posts: 87
2
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
To be honest, I do not want to mess with error checking right now. It is just too much and I do not want to get overwhelmed.

I did find a way to create a operation interface that seems to make a lot of sense. I am not sure if it is overkill thought.
I created a new branch today and started from scratch and there is all my commits.
https://github.com/CodeAmend/calculator/commits/fresh-start

 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
It's great to see you get more reps in. You're doing exactly what I do: keep doing the same exercise over and over and explore other design options and refactoring.

Remember, practice doesn't make perfect. Only perfect practice makes perfect.

One thing that might help you: explain more of your intent in your commit comments. You can have more than one line in a commit comment. If you let Git open up an editor when you commit, the first line will be the commit summary and succeeding lines can go into the details of the commit. See this page: https://www.atlassian.com/git/tutorials/saving-changes/git-commit for a description of the canonical format for a good Git commit comment.

You asked for some sample code to refactor before but you've provided yourself with quite a number of good opportunities.

I smell duplication in your Calculator class. Do you smell it? Edit: Maybe duplication isn't the right name for the smell. More like noisy repetition.

The Operation interface seems reasonable. What was your motivation in creating that though?

Not sure I see the point for adding the OperandStack class though. What is the point of having this class? What are you gaining?

I smell a lot of duplication in the Operation implementations. Seems like the concept of unary/binary/ternary operations combined with a separation of concerns might help cut down some of that noise.

 
Michael Bruce Allen
Ranch Hand
Posts: 87
2
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Junilu Lacar wrote:It's great to see you get more reps in. You're doing exactly what I do: keep doing the same exercise over and over and explore other design options and refactoring.

Remember, practice doesn't make perfect. Only perfect practice makes perfect.

One thing that might help you: explain more of your intent in your commit comments. You can have more than one line in a commit comment. If you let Git open up an editor when you commit, the first line will be the commit summary and succeeding lines can go into the details of the commit. See this page: https://www.atlassian.com/git/tutorials/saving-changes/git-commit for a description of the canonical format for a good Git commit comment.



I did not know you can add more than one line. I have seen this come up when I was first learning Git but just didnt realize what was going on. Cool Thanks.

Junilu Lacar wrote:You asked for some sample code to refactor before but you've provided yourself with quite a number of good opportunities.


Alright then, lets first figure out if I am going in the right direction.

Junilu Lacar wrote:I smell duplication in your Calculator class. Do you smell it? Edit: Maybe duplication isn't the right name for the smell. More like noisy repetition.


Instantiating in a more simple way maybe? I did something down below...


Junilu Lacar wrote:The Operation interface seems reasonable. What was your motivation in creating that though?


There will potentially be many more operators and I want to be able to easily access and add to them. In a future calculator I would want user created possibilities. Perhaps there is a way so that I can just add a new operator and not need to change any more code? I was also thinking that perhaps an interface should be added for each new operator, this is quickly getting out of my experience though.

Junilu Lacar wrote:Not sure I see the point for adding the OperandStack class though. What is the point of having this class? What are you gaining?



I felt that the calculator was taking on a lot. I mean, if this calculator app was gong to be really advanced, would the stack be within the main calculator class? I am not sure the point, I felt I was folloing the S in SOLID.

Junilu Lacar wrote:I smell a lot of duplication in the Operation implementations. Seems like the concept of unary/binary/ternary operations combined with a separation of concerns might help cut down some of that noise.



How about this for a start?


 
Michael Bruce Allen
Ranch Hand
Posts: 87
2
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Now that I look it over again, I do not like OperandStack as a separate class. It does absolutely nothing except creates a bunch of new code that does not help the readability. I am going to remove it now...

https://github.com/CodeAmend/calculator/commits/fresh-start

 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
That certainly looks like an improvement. However, when I read it out loud, it still doesn't make sense. Why is the code applying a stack? "When asked to perform the X operation, the calculator creates a new XOperation object and applies the stack to it" doesn't sound quite right.

It makes more sense if you say "when asked to perform the X operation, the calculator does it with a new XOperation object."

I like to say: "Make the code say what it does and do what it says, no more, no less." and "Reveal intent, hide the details."

Here, the high level code tells what is going on, and the low-level code (the perform() method), tells how it's happening. Always try to separate the two.
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
All these ideas, both yours and mine, will eventually lead to a very nice implementation of a calculator that is easily extensible while adhering to the OCP - The Open-Closed Principle. You can't get there in one fell swoop though. It slowly morphs into that through many refactorings. I've only run through the morphing process in my head but in theory, it looks pretty darn good to me. But let's not get ahead of ourselves. Allow yourself to make one small step forward by building upon what you did in the previous small step.
 
Michael Bruce Allen
Ranch Hand
Posts: 87
2
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Junilu Lacar wrote:That certainly looks like an improvement. However, when I read it out loud, it still doesn't make sense. Why is the code applying a stack? "When asked to perform the X operation, the calculator creates a new XOperation object and applies the stack to it" doesn't sound quite right.

It makes more sense if you say "when asked to perform the X operation, the calculator does it with a new XOperation object."

I like to say: "Make the code say what it does and do what it says, no more, no less." and "Reveal intent, hide the details."

Here, the high level code tells what is going on, and the low-level code (the perform() method), tells how it's happening. Always try to separate the two.




I wasn't sure how to do this at that time (i was making it WAY too complicated), but now that I see this I understand. I see apply over and over again, so making a new method that takes each one of the Operations and passes them with apply.

I like what you did. Addition also sounds better than AddOperator. So here is my code and git
https://github.com/CodeAmend/calculator/commits/fresh-start

 
Michael Bruce Allen
Ranch Hand
Posts: 87
2
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
i just added a new drop feature as well.
https://github.com/CodeAmend/calculator/commits/fresh-start
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Michael Bruce Allen wrote:I wasn't sure how to do this at that time (i was making it WAY too complicated)


Don't get down on yourself about that. Everybody does it, even the most experienced developers from time to time, if not most of the time. It's best to remind yourself of this by asking questions like, "Is this the least amount of code I need to say this? Is there a better way to say this? Is it conversational without sounding too geeky? If it sounds too geeky, is there too much implementation detail leaking out here? How would I explain this to my grandmother who's not a geek?"
 
Michael Bruce Allen
Ranch Hand
Posts: 87
2
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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.


P.S. can you check your PM?
 
Michael Bruce Allen
Ranch Hand
Posts: 87
2
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Junilu Lacar wrote:

Michael Bruce Allen wrote:I wasn't sure how to do this at that time (i was making it WAY too complicated)


Don't get down on yourself about that. Everybody does it, even the most experienced developers from time to time, if not most of the time. It's best to remind yourself of this by asking questions like, "Is this the least amount of code I need to say this? Is there a better way to say this? Is it conversational without sounding too geeky? If it sounds too geeky, is there too much implementation detail leaking out here? How would I explain this to my grandmother who's not a geek?"



Yeah, I was thinking, well, what is the point of polymorphism if I cant use it here. So it felt weird not using it. the perfomr(Operation op) method!!! I really think I can catch this type of thing next time
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Notice that as you're refactoring, you're not writing a whole lot of new code. You're just "rephrasing" what the old code was saying. This "rephrasing" involves rename, extract, and making better abstractions. Notice also that you're probably moving responsibilities around, telling that object or method to handle a task rather than this object or method. The more you retrospect and become aware that this is what you're doing, the more sensitive you become to it. Keep going, you're doing great.
 
Michael Bruce Allen
Ranch Hand
Posts: 87
2
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I just do not know what to do now...

Would you be willing to ask me a few questions as if you were my boss giving me a project? I feel, I need a path to follow, or another goal.
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

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.


There is a relatively trivial way: https://docs.oracle.com/javase/tutorial/java/data/numberformat.html

As with the String input and parsing, don't worry about displaying the result for now. That's all infrastructure and window dressing. 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.

There is still a lot of duplication in your Operation implementations for addition, subtraction, etc. See what you can do to separate the concerns of working with the stack (popping/pushing numbers on/off) and performing the actual arithmetic. This will result in cleaner code and will lead you to being able to support Unary or Binary or Ternary or However-many-ary operations ;)
 
Michael Bruce Allen
Ranch Hand
Posts: 87
2
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Well, I feel better about this. At least there isnt so much duplicate code. The hint really helped though
https://github.com/CodeAmend/calculator/commits/fresh-start




 
Michael Bruce Allen
Ranch Hand
Posts: 87
2
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
One more refactor:
Is this ok? It sure removed a lot of code. It still isn't as clear as it was before. Before sending the to pop() returns through the method perimeter, it seemed easier to understand ||operation apply to stack|| now it is ||operation apply to stack pop() pop()|| Just not sure if this is the way it should look.

 
Michael Bruce Allen
Ranch Hand
Posts: 87
2
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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. I do not have to send to operands through a method perimeter!!!

Created BinarayOperation abstract class that implemented Operation. Each Operation type class no longer implements Operation, but now extends BinarayOperation. This code is much easier and now it would be much easier to extend to new calculation types in the future.
https://github.com/CodeAmend/calculator/commits/fresh-start



Example Operation class

 
Michael Bruce Allen
Ranch Hand
Posts: 87
2
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I made an evaluate method which takes String values. This also cut the code down quit a bit. I am not sure if this is a good way, but it gave me another idea. Can I send operators as well as operands in evaluate?
Would I have to make some sort of Node class the has two subclasses OpNode and ValueNode? Then I can polymorphically send a Node through evaluate? This idea came from
https://www.youtube.com/watch?v=4F72VULWFvc about 13 minutes into it.

 
Michael Bruce Allen
Ranch Hand
Posts: 87
2
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Just testing a bit here. https://github.com/CodeAmend/calculator/commits/test

I do not know exactly how to do this. I am getting confused quickly
am I at all on the right track?




I am trying to use a UnaryOperator I made but the if(stack.size() > 1) in the perform method in Calculator class is killing it.
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

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.

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.

Let me walk you through how I would have refactored and my thought process:

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."

So I might try this:

After extracting the conditional expression to its own method, I have clarified the intent of the higher level code. But I'm leaking the implementation detail that I need two operands. I'll rename that with something more abstract:

 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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:

Edit: Added implements Operation


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.
 
Consider Paul's rocket mass heater.
reply
    Bookmark Topic Watch Topic
  • New Topic