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.
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!
Michael Bruce Allen wrote:That's pretty cool. I just learned a ton again. This forum is sky rocketing my learning!
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.
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.
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.
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.
Junilu Lacar wrote:You asked for some sample code to refactor before but you've provided yourself with quite a number of good opportunities.
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.
Junilu Lacar wrote:The Operation interface seems reasonable. What was your motivation in creating that 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?
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.
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.
Michael Bruce Allen wrote:I wasn't sure how to do this at that time (i was making it WAY too complicated)
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?"
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.
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.
Consider Paul's rocket mass heater. |