-
Notifications
You must be signed in to change notification settings - Fork 1
support exp operator #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,4 +3,5 @@ export enum OperatorKeys { | |
| MINUS = "-", | ||
| MULT = "*", | ||
| DIV = "/", | ||
| EXP = "exp", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need for the last comment, will give an error There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that "exp" should be consistent with the previous symbols. You can use "^". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could also make this a symbol, such as "**" instead of text to be consistent. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It makes more sense for the exponential operator key to be "^". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. might not be the right value to use for EXP There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should use e** There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A "^" might be easier to press as an exponent key? |
||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The user may want to execute operations within the exp function i.e. exp(2 * 2). Therefore, we should aim to support this action by requiring the user to enter parentheses to start and end the function. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Write out all tests and run them before attempting to push please. We need to make sure all cases are covered and problems are detected early. The tests you have in TODO looks great for covering those other cases. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -269,6 +269,46 @@ describe("CalculatorModel", (): void => { | |
| calculator.pressActionKey(ActionKeys.EQUALS); | ||
| expect(calculator.display()).toEqual("1"); | ||
| }); | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be helpful to have some more insight on what you are testing by leaving some comments before the test. For example, you can say that you are specifically testing for e^1. At first glance, it is easy to miss the "e", so some may assume you are just testing 1 as the exponent! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should try to leave comments on the tests There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Include a comment explaining the mathematical expectation (e^1 ≈ 2.718) to improve readability for future readers. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add comments There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some aspects of these tests could be separated so that there is less repetition. |
||
| it("should display e**1 with `exp(1)`", (): void => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that it would be helpful to add comments to your code, create better titles for your tests, and define constants to make your code more readable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Calculator does not support "e", either implement e as a constant value or change your tests. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good test! |
||
| calculator.pressOperatorKey(OperatorKeys.EXP); | ||
| calculator.pressNumericKey(NumericKeys.ONE); | ||
| calculator.pressActionKey(ActionKeys.EQUALS); | ||
| const displayValue: string = calculator.display(); | ||
| expect(displayValue).toEqual("2.718281828459045"); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should probably round these in the model, maybe 2-3 decimal points? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All of the tests should round to a specific value, these toEqual functions can have roundoff error There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we want to use .toEqual(string) when checking the value of floats? Could we use .toBeCloseTo instead? |
||
| }); | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. brief comment explaining the expected result (e^4 ≈ 54.598) would improve clarity. |
||
| it("should display e**4 with `exp(4)`", (): void => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be helpful to leave comments above tests describing their purpose. |
||
| calculator.pressOperatorKey(OperatorKeys.EXP); | ||
| calculator.pressNumericKey(NumericKeys.FOUR); | ||
| calculator.pressActionKey(ActionKeys.EQUALS); | ||
| const displayValue: string = calculator.display(); | ||
| expect(displayValue).toEqual("54.598150033144236"); | ||
| }); | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure that the calculator is handling operator precedence correctly, especially when mixing operations like exp and +. |
||
| it("should display (e**9)+7 with `exp(9)+7)`", (): void => { | ||
| calculator.pressOperatorKey(OperatorKeys.EXP); | ||
| calculator.pressNumericKey(NumericKeys.NINE); | ||
| calculator.pressOperatorKey(OperatorKeys.PLUS); | ||
| calculator.pressNumericKey(NumericKeys.SEVEN); | ||
| calculator.pressActionKey(ActionKeys.EQUALS); | ||
| const displayValue: string = calculator.display(); | ||
| expect(displayValue).toEqual("8110.083927575384"); | ||
| }); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. consider rounding the values to a set decimal place |
||
|
|
||
| it("should display (e**9)*7 with `exp(9)*7)`", (): void => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could reduce redundancy by combining some tests because they are very similar. You could make a helper function to do this by having the numeric key be the input. |
||
| calculator.pressOperatorKey(OperatorKeys.EXP); | ||
| calculator.pressNumericKey(NumericKeys.NINE); | ||
| calculator.pressOperatorKey(OperatorKeys.MULT); | ||
| calculator.pressNumericKey(NumericKeys.SEVEN); | ||
| calculator.pressActionKey(ActionKeys.EQUALS); | ||
| const displayValue: string = calculator.display(); | ||
| expect(displayValue).toEqual("56721.58749302769"); | ||
| }); | ||
|
Comment on lines
+273
to
+307
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nit] Arrange, Act, Assert comments for more clarity. |
||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should test with all operators, missing "-" and "/" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could also add tests for division and subtraction. they should behave similar to multiplication and division but worth checking. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a test for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add additional tests for more edge cases, such as exp(0) to ensure functionality is working correctly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make sure to cover edge cases like exp(-1) or exp(0) in future tests. |
||
| // TODO(max): Test exp(negative) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we implement these before merging? otherwise maybe remove the TODOs There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you expect other people to do your job for you? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. did you mean to write tests for these TODO's? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should implement all the tests and make sure they pass. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you include these within the pull request There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good tests so far! Make sure to write these tests before you implement next time There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If TODO comments won't be implemented later on, do not keep. |
||
| // TODO(max): Support exp(exp(n)) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These test cases should be implemented asap to have more code coverage
Comment on lines
+309
to
+310
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These should be implemented in this PR, already new testing implementation, so should be reasonable to add onto the end. |
||
| // TODO(max): Test 1+2+3+4... are they even legal? | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unsure what this test will / suppose to be. Please let me know, or implement it like you have for some of the other tests! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what does the last to-do mean? Are you testing for exponent functionality or addition? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should also add a test for exp(0) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do not push comments of this variety, instead ask a programming lead. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leave TODOs out of the production branch
Comment on lines
+309
to
+311
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implement these tests to ensure everything is working.
Comment on lines
+309
to
+311
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please write all your tests before submitting for code review There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does this mean? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is not supported then we should handle it gracefully in the logic and add test cases to ensure it is handled. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is vague, what exactly is the proposed test here? |
||
| }); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Current tests LGTM! I agree that testing negative exponents and exponents of exponents functionality could be a good idea. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Testing minus and division although redundant might be necessary for full code coverage because the implementation utilizes a switch case |
||
|
|
||
| describe("CalculatorStateMachine", (): void => { | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please include more comments in the code regarding the new process. I suggest including comments for each state in the pressOperatorKey() method. Additionally, please do not attempt to push unfinished code, and comment out or delete console logs. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,7 +23,7 @@ export class CalculatorModel implements ICalculatorModel { | |
| this._buffer += key; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code is very difficult to read because of the way the indentation and the brackets are done. |
||
| } | ||
|
|
||
| private pushNumber(): void { | ||
| private pushBuffer(): void { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a better name that clarifies the use of the function! Could There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Method name change here is unnecessary I think. The method pushes a number to the field _numberStack, so the original name, "pushNumber", makes more sense. Nothing is being pushed to buffer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Im not sure that this change should be in this PR, it doesn't help with implementing EXP. Maybe split this into two PRs? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the pushNumber would be changed to a buffer pusher, change the functionality to actually get each value instead of keeping just the number There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. careful renaming functions, doubled checked and seems like all references are renamed 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good change. Even though a number is being pushed, it's coming from the buffer, so the buffer is being pushed. |
||
| const num: number = parseFloat(this._buffer); | ||
| this._numberStack.push(num); | ||
| this._buffer = ""; | ||
|
|
@@ -37,13 +37,28 @@ export class CalculatorModel implements ICalculatorModel { | |
| } | ||
|
|
||
| public pressOperatorKey(key: OperatorKeys): void { | ||
| this.pushNumber(); | ||
| this.pushBuffer(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See above, Im not sure this change actually helps implements EXP There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding validation for parseFloat to handle potential edge cases like non-numeric values or multiple decimals. |
||
| if (this.getState() === State.ENTERING_FIRST_OPERAND) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using an enumeration to represent operands / operations limit the number of operations possible for the user. Th e user will not be able to input an expression with more than 3 operations with the current implementation. I recommend removing state. This issue extends further than this PR itself. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. two consecutive ifs may be confusing here, should find a way to combine them |
||
| this.setState(State.ENTERING_SECOND_OPERAND); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might want to connect this into one "if" statement so we don't have nested "if" to avoid confusion |
||
| if ( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be helpful to distinguish the different cases this function is cycling through, by using comments denoting what the if-statement is checking. |
||
| this._operatorStack.length > 0 && | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lack of comments on the functionality of the new feature. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. won't need to check that the length > 0 if the value at the length -1 is EXP |
||
| this._operatorStack[this._operatorStack.length - 1] === OperatorKeys.EXP | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The evaluation might want to wait until |
||
| ) { | ||
| this._operatorStack.pop(); | ||
| const num: number = this.popNumber(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about for negative numbers? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we're changing pushNumber to pushBuffer, does popNumber also have to change? |
||
| console.log("doing exp", num); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's not leave c.logs in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove print statements There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you remove the console.log() statements on line 48 and 50 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For clarity purposes, when a user will review the logs, it would be helpful to see 'exp' written out in its entirety (e.g., 'exponent'). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't need to log "doing exp" or "...it's" for every calculation that is made There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could give some clarity on what these console logs are doing through comments There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. console.log is not needed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These console logs are good for debugging, but they should be removed before submitting a pull request. |
||
| const result: number = Math.exp(num); | ||
| console.log("...it's", result); | ||
|
Comment on lines
+46
to
+50
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This logic could potentially be abstracted into its own method for computing the logic as it is the same as lines 119 to 121.
Comment on lines
+46
to
+50
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. consider abstracting this to a helper function that returns result using the operatorStack and numberStack, logic is the same as lines 119-121
Comment on lines
+48
to
+50
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove debug console logs possibly? |
||
| this._numberStack = [result]; | ||
| } | ||
| if (key !== OperatorKeys.EXP) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As noted below, that means this if statement will also not be necessary |
||
| // I do not understand what is going on here | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it seems like we only have one case where only one operand is needed (EXP). maybe we just use an "else" here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you considered Googling? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we remove this from this PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the point of this comment? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Delete before merging (also real) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the code here is saying: if the first operator isn't the exp key, then skip to the second operator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unhelpful comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the future, add comments if this variety in a code review or pull request instead of in the code itself. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand either, let's find a time to chat about this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unnecessary comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. specify what is not understood? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. moving on if something goes wrong? theoretically this should not occur since this function only happens when the key is pressed but this at least provides a way to move past an error? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's discuss this offline. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should set the current state as entering into the second phase of operand if the key pressed is anything other than exponent. |
||
| this.setState(State.ENTERING_SECOND_OPERAND); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The exp function takes one operand and should evaluate it after There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to remove this from this pull request. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're on the right track. Essentially, this line suggests that if you're not doing the exp operation, the other operations (+, -, *, /) requires two operands. Hence, you'll need to evaluate the operator on two operands. |
||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A bit of confusion here with the state management. Consider refactoring to handle calculator inputs clearer. Consider similarities with |
||
| } else if (this.getState() === State.ENTERING_SECOND_OPERAND) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding a comment for why we do not check for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. couldn't this be an else instead of an else if? |
||
| if (key === OperatorKeys.PLUS || key === OperatorKeys.MINUS) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. consider using a switch statement instead of if statements |
||
| const op: OperatorKeys = this._operatorStack.pop(); | ||
| // We can always collapse the stack. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if this is the best way to move forward with this function. Can we investigate collapsing the stack before we approve this? |
||
| assert(this._numberStack.length === 2); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like leaving assertions in code outside of tests - if we need to throw a meaningful error here, let's do that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't need to be asserting this value, can check for it previously to maintain good style There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove assert messages There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can be a double = operator instead There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if there were more than 2 numbers? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Couldn't there be more than 2 numbers? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be better to have a catch exception here so we don't crash and see a weird error message. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. throwing an error here may be helpful instead There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you clarify on what this line means? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to add assert check inside the implementation |
||
| const right: number = this.popNumber(); | ||
| const left: number = this.popNumber(); | ||
| const result: number = this.compute(op, left, right); | ||
|
|
@@ -95,11 +110,22 @@ export class CalculatorModel implements ICalculatorModel { | |
| this._buffer += "."; | ||
| break; | ||
| case ActionKeys.EQUALS: { | ||
| this.pushNumber(); | ||
| this.pushBuffer(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See above, not sure this targets implementing exp. |
||
| if (this.getState() === State.ENTERING_FIRST_OPERAND) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't seem to account for large number of operands in a larger expression, including several exps |
||
| this._buffer = this.popNumber().toString(); | ||
| if ( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be factored out with the use of the same code above starting on line 42, or the previous usage above can be changed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is not necessarily to detect for only the exp function. This can cause issues later if more functions need to be implemented besides exp. I think reverting is the way to go here. |
||
| this._operatorStack[this._operatorStack.length - 1] === | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. repeated conditional from above, maybe refactor out? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code is so hard to read. Have I not asked you enough to cleanup your code? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might want to leave this on the same line to avoid confusion. |
||
| OperatorKeys.EXP | ||
| ) { | ||
| this._operatorStack.pop(); | ||
| const num: number = this.popNumber(); | ||
| const result: number = Math.exp(num); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not necessarily broken; however, the evaluation should be done when the equal sign is inputted. The implementation for evaluating this new function can be added to the compute() method. |
||
| this._buffer = result.toString(); | ||
| } else { | ||
| this._buffer = this.popNumber().toString(); | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nit] 2 calls of pop number, maybe only if case for more specific application? |
||
| } else if (this.getState() === State.ENTERING_SECOND_OPERAND) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems a bit akward - you pop the operator stack if EXP is found, compute the exponentiation, and replace the number stack with the result. Consider combining two numbers on the stack instead and handling like other operator keys |
||
| const op: OperatorKeys = this._operatorStack.pop(); | ||
| assert(this._numberStack.length === 2); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you remove this check from this pr. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure it would be best to use an assert statement, may cause runtime errors There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to add assert check inside the implementation |
||
| const right: number = this.popNumber(); | ||
| const left: number = this.popNumber(); | ||
| const result: number = this.compute(op, left, right); | ||
|
Comment on lines
114
to
131
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider making the operands more scalable rather than using another else if statement when adding another operand. |
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a different symbol we can use to keep pattern? (1 char)