Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/enums/operator-keys.enum.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,5 @@ export enum OperatorKeys {
MINUS = "-",
MULT = "*",
DIV = "/",
EXP = "exp",

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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for the last comment, will give an error

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes more sense for the exponential operator key to be "^".

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might not be the right value to use for EXP

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should use e**

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A "^" might be easier to press as an exponent key?

}
40 changes: 40 additions & 0 deletions src/models/calculator.model.spec.ts

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

The 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
Expand Up @@ -269,6 +269,46 @@ describe("CalculatorModel", (): void => {
calculator.pressActionKey(ActionKeys.EQUALS);
expect(calculator.display()).toEqual("1");
});

Choose a reason for hiding this comment

The 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!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should try to leave comments on the tests

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add comments

Copy link

Choose a reason for hiding this comment

The 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 => {

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

The 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");

Choose a reason for hiding this comment

The 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?

Choose a reason for hiding this comment

The 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

Copy link

Choose a reason for hiding this comment

The 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?

});

Choose a reason for hiding this comment

The 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 => {
Copy link

Choose a reason for hiding this comment

The 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");
});

Choose a reason for hiding this comment

The 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");
});

Choose a reason for hiding this comment

The 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 => {

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Arrange, Act, Assert comments for more clarity.


Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should test with all operators, missing "-" and "/"

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test for $e^0 = 1$

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we implement these before merging? otherwise maybe remove the TODOs

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you expect other people to do your job for you?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you mean to write tests for these TODO's?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should implement all the tests and make sure they pass.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you include these within the pull request

Choose a reason for hiding this comment

The 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

Copy link

Choose a reason for hiding this comment

The 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))
Copy link

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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?

Choose a reason for hiding this comment

The 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!

Choose a reason for hiding this comment

The 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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should also add a test for exp(0)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not push comments of this variety, instead ask a programming lead.

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please write all your tests before submitting for code review

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this mean?

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is vague, what exactly is the proposed test here?

});

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

The 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 => {
Expand Down
36 changes: 31 additions & 5 deletions src/models/calculator.model.ts

Choose a reason for hiding this comment

The 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
Expand Up @@ -23,7 +23,7 @@ export class CalculatorModel implements ICalculatorModel {
this._buffer += key;
Copy link

Choose a reason for hiding this comment

The 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 {

Choose a reason for hiding this comment

The 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 popNumber() be changed to popBuffer() to match?

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

The 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?

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

careful renaming functions, doubled checked and seems like all references are renamed 👍

Copy link

Choose a reason for hiding this comment

The 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 = "";
Expand All @@ -37,13 +37,28 @@ export class CalculatorModel implements ICalculatorModel {
}

public pressOperatorKey(key: OperatorKeys): void {
this.pushNumber();
this.pushBuffer();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above, Im not sure this change actually helps implements EXP

Choose a reason for hiding this comment

The 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) {

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

The 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);
Copy link

Choose a reason for hiding this comment

The 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 (

Choose a reason for hiding this comment

The 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 &&

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lack of comments on the functionality of the new feature.

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The evaluation might want to wait until EQUALS is pressed, rather than taking the value immediately if the previous key was EXP.

) {
this._operatorStack.pop();
const num: number = this.popNumber();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about for negative numbers?

Choose a reason for hiding this comment

The 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);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's not leave c.logs in

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove print statements

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you remove the console.log() statements on line 48 and 50

Choose a reason for hiding this comment

The 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').

Choose a reason for hiding this comment

The 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

Copy link

Choose a reason for hiding this comment

The 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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

console.log is not needed.

Copy link

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove debug console logs possibly?

this._numberStack = [result];
}
if (key !== OperatorKeys.EXP) {

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered Googling?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we remove this from this PR?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the point of this comment?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete before merging (also real)

Choose a reason for hiding this comment

The 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
is this your intended functionality, if you didn't understand what was happening?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unhelpful comment

Copy link

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary comment

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

specify what is not understood?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove comment

Choose a reason for hiding this comment

The 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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's discuss this offline.

Copy link

Choose a reason for hiding this comment

The 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);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exp function takes one operand and should evaluate it after EQUALS is pressed, so these two states for operands might not work here. After pressing EXP the code should then accept one operand entered after it to then evaluate.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to remove this from this pull request.

Copy link

Choose a reason for hiding this comment

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

}

Choose a reason for hiding this comment

The 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 case ActionKeys.EQUALS: line 113 - 125. Maybe abstract this out?

} else if (this.getState() === State.ENTERING_SECOND_OPERAND) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a comment for why we do not check for the OperatorKeys.EXP operator here.

Copy link

Choose a reason for hiding this comment

The 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) {

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

The 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);

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

The 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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove assert messages

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be a double = operator instead

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if there were more than 2 numbers?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't there be more than 2 numbers?

Copy link

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throwing an error here may be helpful instead

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify on what this line means?

Choose a reason for hiding this comment

The 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);
Expand Down Expand Up @@ -95,11 +110,22 @@ export class CalculatorModel implements ICalculatorModel {
this._buffer += ".";
break;
case ActionKeys.EQUALS: {
this.pushNumber();
this.pushBuffer();

Choose a reason for hiding this comment

The 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) {

Choose a reason for hiding this comment

The 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 (

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

The 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] ===

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

repeated conditional from above, maybe refactor out?

Choose a reason for hiding this comment

The 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?

Copy link

Choose a reason for hiding this comment

The 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);

Choose a reason for hiding this comment

The 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();
}

Choose a reason for hiding this comment

The 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) {

Choose a reason for hiding this comment

The 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);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you remove this check from this pr.

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

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

Expand Down