support exp operator by YUH-Z · Pull Request #2 · neu-cs4530-fall2024/Week5-CodeReviewActivity

YUH-Z · 2024-10-04T15:24:39Z

implement e**x

markdstouffer · 2024-10-04T18:07:54Z

src/models/calculator.model.spec.ts

+    calculator.pressNumericKey(NumericKeys.ONE);
+    calculator.pressActionKey(ActionKeys.EQUALS);
+    const displayValue: string = calculator.display();
+    expect(displayValue).toEqual("2.718281828459045");


we should probably round these in the model, maybe 2-3 decimal points?

markdstouffer · 2024-10-04T18:08:28Z

src/models/calculator.model.spec.ts

+    expect(displayValue).toEqual("56721.58749302769");
+  });
+
+  // TODO(max): Test exp(negative)


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

markdstouffer · 2024-10-04T18:09:11Z

src/models/calculator.model.ts

+      ) {
+        this._operatorStack.pop();
+        const num: number = this.popNumber();
+        console.log("doing exp", num);


let's not leave c.logs in

markdstouffer · 2024-10-04T18:13:36Z

src/models/calculator.model.ts

+        this._numberStack = [result];
+      }
+      if (key !== OperatorKeys.EXP) {
+        // I do not understand what is going on here


it seems like we only have one case where only one operand is needed (EXP). maybe we just use an "else" here?

markdstouffer · 2024-10-04T18:14:23Z

src/models/calculator.model.ts

      if (key === OperatorKeys.PLUS || key === OperatorKeys.MINUS) {
        const op: OperatorKeys = this._operatorStack.pop();
        // We can always collapse the stack.
+        assert(this._numberStack.length === 2);


I don't like leaving assertions in code outside of tests - if we need to throw a meaningful error here, let's do that.

markdstouffer · 2024-10-04T18:15:26Z

src/models/calculator.model.ts

        if (this.getState() === State.ENTERING_FIRST_OPERAND) {
-          this._buffer = this.popNumber().toString();
+          if (
+            this._operatorStack[this._operatorStack.length - 1] ===


repeated conditional from above, maybe refactor out?

markdstouffer · 2024-10-04T18:16:20Z

src/enums/operator-keys.enum.ts

  MINUS = "-",
  MULT = "*",
  DIV = "/",
+  EXP = "exp",


is there a different symbol we can use to keep pattern? (1 char)

ben-weiss0 · 2024-10-04T18:35:35Z

src/models/calculator.model.ts

      if (key === OperatorKeys.PLUS || key === OperatorKeys.MINUS) {
        const op: OperatorKeys = this._operatorStack.pop();
        // We can always collapse the stack.
+        assert(this._numberStack.length === 2);


What if there were more than 2 numbers?

ben-weiss0 · 2024-10-04T18:38:00Z

src/models/calculator.model.ts

+        this._operatorStack[this._operatorStack.length - 1] === OperatorKeys.EXP
+      ) {
+        this._operatorStack.pop();
+        const num: number = this.popNumber();


What about for negative numbers?

ben-weiss0 · 2024-10-04T18:41:53Z

src/models/calculator.model.ts

      case ActionKeys.EQUALS: {
-        this.pushNumber();
+        this.pushBuffer();
        if (this.getState() === State.ENTERING_FIRST_OPERAND) {


Doesn't seem to account for large number of operands in a larger expression, including several exps

akpav · 2024-10-04T18:29:20Z

src/models/calculator.model.ts

+      ) {
+        this._operatorStack.pop();
+        const num: number = this.popNumber();
+        console.log("doing exp", num);


remove print statements

akpav · 2024-10-04T18:30:32Z

src/models/calculator.model.ts

+        this._numberStack = [result];
+      }
+      if (key !== OperatorKeys.EXP) {
+        // I do not understand what is going on here


can we remove this from this PR?

akpav · 2024-10-04T18:31:28Z

src/models/calculator.model.ts

      if (key === OperatorKeys.PLUS || key === OperatorKeys.MINUS) {
        const op: OperatorKeys = this._operatorStack.pop();
        // We can always collapse the stack.
+        assert(this._numberStack.length === 2);


remove assert messages

akpav · 2024-10-04T18:37:40Z

src/models/calculator.model.spec.ts

+    calculator.pressNumericKey(NumericKeys.ONE);
+    calculator.pressActionKey(ActionKeys.EQUALS);
+    const displayValue: string = calculator.display();
+    expect(displayValue).toEqual("2.718281828459045");


do we want to use .toEqual(string) when checking the value of floats? Could we use .toBeCloseTo instead?

KidusYohannesA · 2024-10-04T18:35:46Z

src/models/calculator.model.ts

+        this._numberStack = [result];
+      }
+      if (key !== OperatorKeys.EXP) {
+        // I do not understand what is going on here


Unhelpful comment

KidusYohannesA · 2024-10-04T18:40:44Z

src/models/calculator.model.ts

    if (this.getState() === State.ENTERING_FIRST_OPERAND) {
-      this.setState(State.ENTERING_SECOND_OPERAND);
+      if (
+        this._operatorStack.length > 0 &&


Lack of comments on the functionality of the new feature.

michaelp2002 · 2024-10-04T18:22:07Z

src/enums/operator-keys.enum.ts

  MINUS = "-",
  MULT = "*",
  DIV = "/",
+  EXP = "exp",


No need for the last comment, will give an error

michaelp2002 · 2024-10-04T18:26:03Z

src/models/calculator.model.spec.ts

+    const displayValue: string = calculator.display();
+    expect(displayValue).toEqual("56721.58749302769");
+  });
+


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

michaelp2002 · 2024-10-04T18:31:43Z

src/models/calculator.model.ts

      if (key === OperatorKeys.PLUS || key === OperatorKeys.MINUS) {
        const op: OperatorKeys = this._operatorStack.pop();
        // We can always collapse the stack.
+        assert(this._numberStack.length === 2);


Can be a double = operator instead

michaelp2002 · 2024-10-04T18:32:05Z

src/models/calculator.model.ts

+        this._numberStack = [result];
+      }
+      if (key !== OperatorKeys.EXP) {
+        // I do not understand what is going on here


What is the point of this comment?

dark7torm · 2024-10-04T18:25:35Z

src/models/calculator.model.ts

      if (key === OperatorKeys.PLUS || key === OperatorKeys.MINUS) {
        const op: OperatorKeys = this._operatorStack.pop();
        // We can always collapse the stack.
+        assert(this._numberStack.length === 2);


Don't need to be asserting this value, can check for it previously to maintain good style

dark7torm · 2024-10-04T18:27:30Z

src/models/calculator.model.spec.ts

+    calculator.pressNumericKey(NumericKeys.ONE);
+    calculator.pressActionKey(ActionKeys.EQUALS);
+    const displayValue: string = calculator.display();
+    expect(displayValue).toEqual("2.718281828459045");


All of the tests should round to a specific value, these toEqual functions can have roundoff error

dark7torm · 2024-10-04T18:31:09Z

src/models/calculator.model.spec.ts

+
+  // TODO(max): Test exp(negative)
+  // TODO(max): Support exp(exp(n))
+  // TODO(max): Test 1+2+3+4... are they even legal?


Leave TODOs out of the production branch

dark7torm · 2024-10-04T18:31:50Z

src/models/calculator.model.ts

  }

-  private pushNumber(): void {
+  private pushBuffer(): void {


If the pushNumber would be changed to a buffer pusher, change the functionality to actually get each value instead of keeping just the number

dark7torm · 2024-10-04T18:35:19Z

src/models/calculator.model.ts

+      ) {
+        this._operatorStack.pop();
+        const num: number = this.popNumber();
+        console.log("doing exp", num);


Don't need to log "doing exp" or "...it's" for every calculation that is made

dark7torm · 2024-10-04T18:37:32Z

src/models/calculator.model.spec.ts

+    const displayValue: string = calculator.display();
+    expect(displayValue).toEqual("56721.58749302769");
+  });
+


Add additional tests for more edge cases, such as exp(0) to ensure functionality is working correctly

dark7torm · 2024-10-04T18:40:49Z

src/models/calculator.model.ts

    if (this.getState() === State.ENTERING_FIRST_OPERAND) {
-      this.setState(State.ENTERING_SECOND_OPERAND);
+      if (
+        this._operatorStack.length > 0 &&


won't need to check that the length > 0 if the value at the length -1 is EXP

rupjaisinghani0904 · 2024-10-04T18:32:34Z

src/models/calculator.model.ts

+      ) {
+        this._operatorStack.pop();
+        const num: number = this.popNumber();
+        console.log("doing exp", num);


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

rupjaisinghani0904 · 2024-10-04T18:35:00Z

src/models/calculator.model.ts

+      }
+      if (key !== OperatorKeys.EXP) {
+        // I do not understand what is going on here
+        this.setState(State.ENTERING_SECOND_OPERAND);


Is it possible to remove this from this pull request.

rupjaisinghani0904 · 2024-10-04T18:35:50Z

src/models/calculator.model.ts

+          }
        } else if (this.getState() === State.ENTERING_SECOND_OPERAND) {
          const op: OperatorKeys = this._operatorStack.pop();
+          assert(this._numberStack.length === 2);


can you remove this check from this pr.

rupjaisinghani0904 · 2024-10-04T18:38:01Z

src/models/calculator.model.spec.ts

+    expect(displayValue).toEqual("56721.58749302769");
+  });
+
+  // TODO(max): Test exp(negative)


can you include these within the pull request

kelvinx24

Some implementation and architectural issues are present. Architectural issues extend further than this PR itself. The code needs to be finished, cleaned up, and all test cases must be written out. The existing tests look good.

kelvinx24 · 2024-10-04T17:58:42Z

src/models/calculator.model.spec.ts

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.

kelvinx24 · 2024-10-04T18:06:08Z

src/models/calculator.model.spec.ts

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.

kelvinx24 · 2024-10-04T18:06:13Z

src/models/calculator.model.spec.ts

+
+  // TODO(max): Test exp(negative)
+  // TODO(max): Support exp(exp(n))
+  // TODO(max): Test 1+2+3+4... are they even legal?


Unsure what this test will / suppose to be. Please let me know, or implement it like you have for some of the other tests!

kelvinx24 · 2024-10-04T18:09:59Z

src/models/calculator.model.ts

  }

-  private pushNumber(): void {
+  private pushBuffer(): void {


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.

kelvinx24 · 2024-10-04T18:26:27Z

src/models/calculator.model.ts

+          ) {
+            this._operatorStack.pop();
+            const num: number = this.popNumber();
+            const result: number = Math.exp(num);


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.

kelvinx24 · 2024-10-04T18:35:37Z

src/models/calculator.model.ts

+        this.pushBuffer();
        if (this.getState() === State.ENTERING_FIRST_OPERAND) {
-          this._buffer = this.popNumber().toString();
+          if (


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.

kelvinx24 · 2024-10-04T18:36:55Z

src/models/calculator.model.ts

  public pressOperatorKey(key: OperatorKeys): void {
-    this.pushNumber();
+    this.pushBuffer();
    if (this.getState() === State.ENTERING_FIRST_OPERAND) {


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.

kelvinx24 · 2024-10-04T18:37:23Z

src/models/calculator.model.ts

+        console.log("...it's", result);
+        this._numberStack = [result];
+      }
+      if (key !== OperatorKeys.EXP) {


As noted below, that means this if statement will also not be necessary

kelvinx24 · 2024-10-04T18:38:53Z

src/models/calculator.model.ts

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.

jli924 · 2024-10-04T18:42:08Z

src/models/calculator.model.spec.ts

    calculator.pressActionKey(ActionKeys.EQUALS);
    expect(calculator.display()).toEqual("1");
  });
+


add comments

jli924 · 2024-10-04T18:42:19Z

src/models/calculator.model.ts

+        this._numberStack = [result];
+      }
+      if (key !== OperatorKeys.EXP) {
+        // I do not understand what is going on here


specify what is not understood?

yumio7

please finish those tests you put under TODO and ensure there isnt a fatal flaw in exponent handling, otherwise lgtm

Balog19 · 2024-10-04T18:35:59Z

src/models/calculator.model.ts

      if (key === OperatorKeys.PLUS || key === OperatorKeys.MINUS) {
        const op: OperatorKeys = this._operatorStack.pop();
        // We can always collapse the stack.
+        assert(this._numberStack.length === 2);


Couldn't there be more than 2 numbers?

Balog19 · 2024-10-04T18:39:38Z

src/models/calculator.model.ts

+        // I do not understand what is going on here
+        this.setState(State.ENTERING_SECOND_OPERAND);
+      }
    } else if (this.getState() === State.ENTERING_SECOND_OPERAND) {


couldn't this be an else instead of an else if?

Balog19 · 2024-10-04T18:40:35Z

src/models/calculator.model.ts

@@ -23,7 +23,7 @@ export class CalculatorModel implements ICalculatorModel {
    this._buffer += key;


This code is very difficult to read because of the way the indentation and the brackets are done.

Balog19 · 2024-10-04T18:42:31Z

src/models/calculator.model.spec.ts

    calculator.pressActionKey(ActionKeys.EQUALS);
    expect(calculator.display()).toEqual("1");
  });
+


Some aspects of these tests could be separated so that there is less repetition.

stephanie-xu

Overall functionality looks ok but needs some more tests and resolving the comment stating you were confused

stephanie-xu · 2024-10-04T18:21:58Z

src/models/calculator.model.spec.ts

+
+  // TODO(max): Test exp(negative)
+  // TODO(max): Support exp(exp(n))
+  // TODO(max): Test 1+2+3+4... are they even legal?


what does the last to-do mean? Are you testing for exponent functionality or addition?

should also add a test for exp(0)

stephanie-xu · 2024-10-04T18:33:13Z

src/models/calculator.model.ts

+        this._numberStack = [result];
+      }
+      if (key !== OperatorKeys.EXP) {
+        // I do not understand what is going on here


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?

gus-s-42 · 2024-10-04T18:29:01Z

src/models/calculator.model.spec.ts

+
+  // TODO(max): Test exp(negative)
+  // TODO(max): Support exp(exp(n))
+  // TODO(max): Test 1+2+3+4... are they even legal?


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

gus-s-42 · 2024-10-04T18:30:32Z

src/models/calculator.model.spec.ts

    expect(calculator.display()).toEqual("1");
  });
+
+  it("should display e**1 with `exp(1)`", (): void => {


Calculator does not support "e", either implement e as a constant value or change your tests.

gus-s-42 · 2024-10-04T18:31:45Z

src/models/calculator.model.spec.ts

+    expect(displayValue).toEqual("2.718281828459045");
+  });
+
+  it("should display e**4 with `exp(4)`", (): void => {


It might be helpful to leave comments above tests describing their purpose.

gus-s-42 · 2024-10-04T18:36:28Z

src/models/calculator.model.ts

+        this._numberStack = [result];
+      }
+      if (key !== OperatorKeys.EXP) {
+        // I do not understand what is going on here


In the future, add comments if this variety in a code review or pull request instead of in the code itself.

kyle12803

Please provide a more descriptive summary of your changes.

kyle12803 · 2024-10-04T18:32:38Z

src/models/calculator.model.ts

+        // I do not understand what is going on here
+        this.setState(State.ENTERING_SECOND_OPERAND);
+      }
    } else if (this.getState() === State.ENTERING_SECOND_OPERAND) {


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

kyle12803 · 2024-10-04T18:41:17Z

src/models/calculator.model.ts

+        this._operatorStack.pop();
+        const num: number = this.popNumber();
+        console.log("doing exp", num);
+        const result: number = Math.exp(num);
+        console.log("...it's", result);


This logic could potentially be abstracted into its own method for computing the logic as it is the same as lines 119 to 121.

kyle12803 · 2024-10-04T18:42:24Z

src/models/calculator.model.spec.ts

+
+  // TODO(max): Test exp(negative)
+  // TODO(max): Support exp(exp(n))
+  // TODO(max): Test 1+2+3+4... are they even legal?


If this is not supported then we should handle it gracefully in the logic and add test cases to ensure it is handled.

kyle12803 · 2024-10-04T18:43:05Z

src/models/calculator.model.ts

+        this._numberStack = [result];
+      }
+      if (key !== OperatorKeys.EXP) {
+        // I do not understand what is going on here


Remove comment

reginarabkina

Leaving general comments regarding code format

reginarabkina · 2024-10-04T18:32:40Z

src/models/calculator.model.spec.ts

    calculator.pressActionKey(ActionKeys.EQUALS);
    expect(calculator.display()).toEqual("1");
  });
+


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!

reginarabkina · 2024-10-04T18:35:11Z

src/models/calculator.model.ts

+      ) {
+        this._operatorStack.pop();
+        const num: number = this.popNumber();
+        console.log("doing exp", num);


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

reginarabkina · 2024-10-04T18:37:31Z

src/models/calculator.model.ts

+    this.pushBuffer();
    if (this.getState() === State.ENTERING_FIRST_OPERAND) {
-      this.setState(State.ENTERING_SECOND_OPERAND);
+      if (


It would be helpful to distinguish the different cases this function is cycling through, by using comments denoting what the if-statement is checking.

faridekhalili · 2024-10-04T18:21:50Z

src/models/calculator.model.spec.ts

+    expect(displayValue).toEqual("56721.58749302769");
+  });
+
+  // TODO(max): Test exp(negative)


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

faridekhalili · 2024-10-04T18:22:13Z

src/models/calculator.model.ts

+        this._numberStack = [result];
+      }
+      if (key !== OperatorKeys.EXP) {
+        // I do not understand what is going on here


Have you considered Googling?

faridekhalili · 2024-10-04T18:22:46Z

src/models/calculator.model.ts

        if (this.getState() === State.ENTERING_FIRST_OPERAND) {
-          this._buffer = this.popNumber().toString();
+          if (
+            this._operatorStack[this._operatorStack.length - 1] ===


This code is so hard to read. Have I not asked you enough to cleanup your code?

akuatbekov23 · 2024-10-04T18:33:34Z

src/enums/operator-keys.enum.ts

  MINUS = "-",
  MULT = "*",
  DIV = "/",
+  EXP = "exp",


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

akuatbekov23 · 2024-10-04T18:38:18Z

src/models/calculator.model.spec.ts

    calculator.pressActionKey(ActionKeys.EQUALS);
    expect(calculator.display()).toEqual("1");
  });
+


Include a comment explaining the mathematical expectation (e^1 ≈ 2.718) to improve readability for future readers.

akuatbekov23 · 2024-10-04T18:38:40Z

src/models/calculator.model.spec.ts

+    const displayValue: string = calculator.display();
+    expect(displayValue).toEqual("2.718281828459045");
+  });
+


brief comment explaining the expected result (e^4 ≈ 54.598) would improve clarity.

akuatbekov23 · 2024-10-04T18:39:12Z

src/models/calculator.model.spec.ts

+    const displayValue: string = calculator.display();
+    expect(displayValue).toEqual("54.598150033144236");
+  });
+


Ensure that the calculator is handling operator precedence correctly, especially when mixing operations like exp and +.

akuatbekov23 · 2024-10-04T18:39:37Z

src/models/calculator.model.spec.ts

+    const displayValue: string = calculator.display();
+    expect(displayValue).toEqual("56721.58749302769");
+  });
+


Make sure to cover edge cases like exp(-1) or exp(0) in future tests.

akuatbekov23 · 2024-10-04T18:43:00Z

src/models/calculator.model.ts


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


Consider adding validation for parseFloat to handle potential edge cases like non-numeric values or multiple decimals.

MichaelGadhia

Great work and fine tuning should make this very useful!

MichaelGadhia · 2024-10-04T18:04:48Z

src/models/calculator.model.ts

  }

-  private pushNumber(): void {
+  private pushBuffer(): void {


This is a better name that clarifies the use of the function! Could popNumber() be changed to popBuffer() to match?

MichaelGadhia · 2024-10-04T18:16:06Z

src/models/calculator.model.ts

-      this.setState(State.ENTERING_SECOND_OPERAND);
+      if (
+        this._operatorStack.length > 0 &&
+        this._operatorStack[this._operatorStack.length - 1] === OperatorKeys.EXP


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

MichaelGadhia · 2024-10-04T18:18:38Z

src/models/calculator.model.ts

+      }
+      if (key !== OperatorKeys.EXP) {
+        // I do not understand what is going on here
+        this.setState(State.ENTERING_SECOND_OPERAND);


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.

MichaelGadhia · 2024-10-04T18:21:57Z

src/models/calculator.model.ts

+        this.pushBuffer();
        if (this.getState() === State.ENTERING_FIRST_OPERAND) {
-          this._buffer = this.popNumber().toString();
+          if (


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.

ivng8

Finishing test cases and adding some seemingly redundant tests cases for subtract and minus for more code coverage.

ivng8 · 2024-10-04T18:30:53Z

src/models/calculator.model.spec.ts

+  });
+
+  // TODO(max): Test exp(negative)
+  // TODO(max): Support exp(exp(n))


These test cases should be implemented asap to have more code coverage

ivng8 · 2024-10-04T18:40:42Z

src/models/calculator.model.spec.ts

+  // TODO(max): Test exp(negative)
+  // TODO(max): Support exp(exp(n))
+  // TODO(max): Test 1+2+3+4... are they even legal?
 });


Testing minus and division although redundant might be necessary for full code coverage because the implementation utilizes a switch case

beartackler

Also, consider a more detail description 💯

beartackler · 2024-10-04T18:25:57Z

src/models/calculator.model.spec.ts

+  // TODO(max): Test exp(negative)
+  // TODO(max): Support exp(exp(n))
+  // TODO(max): Test 1+2+3+4... are they even legal?
 });


Current tests LGTM! I agree that testing negative exponents and exponents of exponents functionality could be a good idea.

beartackler · 2024-10-04T18:32:22Z

src/models/calculator.model.ts

+      if (key !== OperatorKeys.EXP) {
+        // I do not understand what is going on here
+        this.setState(State.ENTERING_SECOND_OPERAND);
+      }


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?

beartackler · 2024-10-04T18:35:10Z

src/models/calculator.model.ts

+          } else {
+            this._buffer = this.popNumber().toString();
+          }
        } else if (this.getState() === State.ENTERING_SECOND_OPERAND) {


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

edwinliiiii · 2024-10-04T18:26:00Z

src/models/calculator.model.spec.ts

+    expect(displayValue).toEqual("56721.58749302769");
+  });
+
+  // TODO(max): Test exp(negative)


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

edwinliiiii · 2024-10-04T18:41:38Z

src/models/calculator.model.ts

+        this._operatorStack.pop();
+        const num: number = this.popNumber();
+        console.log("doing exp", num);
+        const result: number = Math.exp(num);
+        console.log("...it's", result);


consider abstracting this to a helper function that returns result using the operatorStack and numberStack, logic is the same as lines 119-121

edwinliiiii · 2024-10-04T18:41:54Z

src/models/calculator.model.ts

+        this._numberStack = [result];
+      }
+      if (key !== OperatorKeys.EXP) {
+        // I do not understand what is going on here


unnecessary comment

edwinliiiii · 2024-10-04T18:42:37Z

src/models/calculator.model.ts

  }

-  private pushNumber(): void {
+  private pushBuffer(): void {


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

gartastrophe

review for implementation of e**

gartastrophe · 2024-10-04T18:25:58Z

src/enums/operator-keys.enum.ts

  MINUS = "-",
  MULT = "*",
  DIV = "/",
+  EXP = "exp",


could also make this a symbol, such as "**" instead of text to be consistent.

gartastrophe · 2024-10-04T18:27:38Z

src/models/calculator.model.spec.ts

+    const displayValue: string = calculator.display();
+    expect(displayValue).toEqual("56721.58749302769");
+  });
+


could also add tests for division and subtraction. they should behave similar to multiplication and division but worth checking.

gartastrophe · 2024-10-04T18:41:16Z

src/models/calculator.model.ts

+        this._operatorStack[this._operatorStack.length - 1] === OperatorKeys.EXP
+      ) {
+        this._operatorStack.pop();
+        const num: number = this.popNumber();


if we're changing pushNumber to pushBuffer, does popNumber also have to change?

gartastrophe · 2024-10-04T18:43:47Z

src/models/calculator.model.ts

+        this._numberStack = [result];
+      }
+      if (key !== OperatorKeys.EXP) {
+        // I do not understand what is going on here


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?

EdiLau · 2024-10-04T18:37:19Z

src/models/calculator.model.ts

      if (key === OperatorKeys.PLUS || key === OperatorKeys.MINUS) {
        const op: OperatorKeys = this._operatorStack.pop();
        // We can always collapse the stack.
+        assert(this._numberStack.length === 2);


Might be better to have a catch exception here so we don't crash and see a weird error message.

EdiLau · 2024-10-04T18:43:41Z

src/models/calculator.model.ts

        if (this.getState() === State.ENTERING_FIRST_OPERAND) {
-          this._buffer = this.popNumber().toString();
+          if (
+            this._operatorStack[this._operatorStack.length - 1] ===


Might want to leave this on the same line to avoid confusion.

EdiLau · 2024-10-04T18:44:10Z

src/models/calculator.model.ts

-    this.pushNumber();
+    this.pushBuffer();
    if (this.getState() === State.ENTERING_FIRST_OPERAND) {
-      this.setState(State.ENTERING_SECOND_OPERAND);


Might want to connect this into one "if" statement so we don't have nested "if" to avoid confusion

zachmarino234

Overall this looks solid! Left some comments and some notes to remove stuff. Can we schedule a time to discuss the pressOperatorKey function?

zachmarino234 · 2024-10-04T18:31:25Z

src/models/calculator.model.spec.ts

    expect(calculator.display()).toEqual("1");
  });
+
+  it("should display e**1 with `exp(1)`", (): void => {


zachmarino234 · 2024-10-04T18:32:34Z

src/models/calculator.model.ts

+        this._numberStack = [result];
+      }
+      if (key !== OperatorKeys.EXP) {
+        // I do not understand what is going on here


Delete before merging (also real)

zachmarino234 · 2024-10-04T18:34:55Z

src/models/calculator.model.spec.ts

+  // TODO(max): Test exp(negative)
+  // TODO(max): Support exp(exp(n))
+  // TODO(max): Test 1+2+3+4... are they even legal?


Implement these tests to ensure everything is working.

zachmarino234 · 2024-10-04T18:36:04Z

src/models/calculator.model.ts

    } else if (this.getState() === State.ENTERING_SECOND_OPERAND) {
      if (key === OperatorKeys.PLUS || key === OperatorKeys.MINUS) {
        const op: OperatorKeys = this._operatorStack.pop();
        // We can always collapse the stack.


Not sure if this is the best way to move forward with this function. Can we investigate collapsing the stack before we approve this?

zachmarino234 · 2024-10-04T18:41:59Z

src/models/calculator.model.spec.ts

+
+  // TODO(max): Test exp(negative)
+  // TODO(max): Support exp(exp(n))
+  // TODO(max): Test 1+2+3+4... are they even legal?


What does this mean?

manavg19 · 2024-10-04T18:32:54Z

src/models/calculator.model.spec.ts

    calculator.pressActionKey(ActionKeys.EQUALS);
    expect(calculator.display()).toEqual("1");
  });
+


should try to leave comments on the tests

manavg19 · 2024-10-04T18:37:11Z

src/models/calculator.model.ts

+      ) {
+        this._operatorStack.pop();
+        const num: number = this.popNumber();
+        console.log("doing exp", num);


could give some clarity on what these console logs are doing through comments

manavg19 · 2024-10-04T18:43:45Z

src/models/calculator.model.ts

  public pressOperatorKey(key: OperatorKeys): void {
-    this.pushNumber();
+    this.pushBuffer();
    if (this.getState() === State.ENTERING_FIRST_OPERAND) {


two consecutive ifs may be confusing here, should find a way to combine them

manavg19 · 2024-10-04T18:45:03Z

src/models/calculator.model.ts

      if (key === OperatorKeys.PLUS || key === OperatorKeys.MINUS) {
        const op: OperatorKeys = this._operatorStack.pop();
        // We can always collapse the stack.
+        assert(this._numberStack.length === 2);


throwing an error here may be helpful instead

oorthwein · 2024-10-04T18:41:09Z

src/models/calculator.model.spec.ts

+    expect(displayValue).toEqual("56721.58749302769");
+  });
+
+  // TODO(max): Test exp(negative)


Good tests so far! Make sure to write these tests before you implement next time

oorthwein · 2024-10-04T18:42:29Z

src/models/calculator.model.spec.ts

+
+  // TODO(max): Test exp(negative)
+  // TODO(max): Support exp(exp(n))
+  // TODO(max): Test 1+2+3+4... are they even legal?


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

brantpan1

Left a few small comments, overall good job!

brantpan1 · 2024-10-04T18:28:55Z

src/models/calculator.model.ts

  }

-  private pushNumber(): void {
+  private pushBuffer(): void {


Im not sure that this change should be in this PR, it doesn't help with implementing EXP. Maybe split this into two PRs?

brantpan1 · 2024-10-04T18:29:37Z

src/models/calculator.model.ts


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


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

brantpan1 · 2024-10-04T18:33:51Z

src/models/calculator.model.ts

+            this._buffer = result.toString();
+          } else {
+            this._buffer = this.popNumber().toString();
+          }


[nit] 2 calls of pop number, maybe only if case for more specific application?

brantpan1 · 2024-10-04T18:34:15Z

src/models/calculator.model.ts

        break;
      case ActionKeys.EQUALS: {
-        this.pushNumber();
+        this.pushBuffer();


See above, not sure this targets implementing exp.

brantpan1 · 2024-10-04T18:41:36Z

src/models/calculator.model.spec.ts

+  // TODO(max): Test exp(negative)
+  // TODO(max): Support exp(exp(n))


These should be implemented in this PR, already new testing implementation, so should be reasonable to add onto the end.

brantpan1 · 2024-10-04T18:42:36Z

src/models/calculator.model.ts

+        console.log("doing exp", num);
+        const result: number = Math.exp(num);
+        console.log("...it's", result);


Remove debug console logs possibly?

brantpan1 · 2024-10-04T18:44:13Z

src/models/calculator.model.spec.ts

+  it("should display e**1 with `exp(1)`", (): void => {
+    calculator.pressOperatorKey(OperatorKeys.EXP);
+    calculator.pressNumericKey(NumericKeys.ONE);
+    calculator.pressActionKey(ActionKeys.EQUALS);
+    const displayValue: string = calculator.display();
+    expect(displayValue).toEqual("2.718281828459045");
+  });
+
+  it("should display e**4 with `exp(4)`", (): void => {
+    calculator.pressOperatorKey(OperatorKeys.EXP);
+    calculator.pressNumericKey(NumericKeys.FOUR);
+    calculator.pressActionKey(ActionKeys.EQUALS);
+    const displayValue: string = calculator.display();
+    expect(displayValue).toEqual("54.598150033144236");
+  });
+
+  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");
+  });
+
+  it("should display (e**9)*7 with `exp(9)*7)`", (): void => {
+    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");
+  });


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

nicoleli26 · 2024-10-04T18:22:20Z

src/enums/operator-keys.enum.ts

  MINUS = "-",
  MULT = "*",
  DIV = "/",
+  EXP = "exp",


I think that "exp" should be consistent with the previous symbols. You can use "^".

nicoleli26 · 2024-10-04T18:30:05Z

src/models/calculator.model.spec.ts

    expect(calculator.display()).toEqual("1");
  });
+
+  it("should display e**1 with `exp(1)`", (): void => {


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.

nicoleli26 · 2024-10-04T18:31:42Z

src/models/calculator.model.spec.ts

+    expect(displayValue).toEqual("8110.083927575384");
+  });
+
+  it("should display (e**9)*7 with `exp(9)*7)`", (): void => {


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.

aidandomondon · 2024-10-04T18:32:52Z

src/models/calculator.model.spec.ts

+    const displayValue: string = calculator.display();
+    expect(displayValue).toEqual("56721.58749302769");
+  });
+


Add a test for $e^0 = 1$

aidandomondon · 2024-10-04T18:45:28Z

src/models/calculator.model.ts

+        this._numberStack = [result];
+      }
+      if (key !== OperatorKeys.EXP) {
+        // I do not understand what is going on here


Let's discuss this offline.

panharathsok3 · 2024-10-04T18:28:44Z

src/models/calculator.model.ts

        if (this.getState() === State.ENTERING_FIRST_OPERAND) {
-          this._buffer = this.popNumber().toString();
+          if (
+            this._operatorStack[this._operatorStack.length - 1] ===
+            OperatorKeys.EXP
+          ) {
+            this._operatorStack.pop();
+            const num: number = this.popNumber();
+            const result: number = Math.exp(num);
+            this._buffer = result.toString();
+          } else {
+            this._buffer = this.popNumber().toString();
+          }
        } else if (this.getState() === State.ENTERING_SECOND_OPERAND) {
          const op: OperatorKeys = this._operatorStack.pop();
+          assert(this._numberStack.length === 2);
          const right: number = this.popNumber();
          const left: number = this.popNumber();
          const result: number = this.compute(op, left, right);


Consider making the operands more scalable rather than using another else if statement when adding another operand.

panharathsok3 · 2024-10-04T18:46:45Z

src/models/calculator.model.ts

      if (key === OperatorKeys.PLUS || key === OperatorKeys.MINUS) {
        const op: OperatorKeys = this._operatorStack.pop();
        // We can always collapse the stack.
+        assert(this._numberStack.length === 2);


No need to add assert check inside the implementation

panharathsok3 · 2024-10-04T18:47:20Z

src/models/calculator.model.ts

+          }
        } else if (this.getState() === State.ENTERING_SECOND_OPERAND) {
          const op: OperatorKeys = this._operatorStack.pop();
+          assert(this._numberStack.length === 2);


No need to add assert check inside the implementation

LonghaoH

Looks good. Need to finish testing and some minor detailed changes in the implementation.

LonghaoH · 2024-10-04T18:31:01Z

src/models/calculator.model.spec.ts

+    expect(displayValue).toEqual("56721.58749302769");
+  });
+
+  // TODO(max): Test exp(negative)


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

LonghaoH · 2024-10-04T18:42:04Z

src/models/calculator.model.ts

+      }
+      if (key !== OperatorKeys.EXP) {
+        // I do not understand what is going on here
+        this.setState(State.ENTERING_SECOND_OPERAND);


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.

LonghaoH · 2024-10-04T18:46:04Z

src/models/calculator.model.ts

+      ) {
+        this._operatorStack.pop();
+        const num: number = this.popNumber();
+        console.log("doing exp", num);


console.log is not needed.

LonghaoH · 2024-10-04T18:46:33Z

src/models/calculator.model.ts

      if (key === OperatorKeys.PLUS || key === OperatorKeys.MINUS) {
        const op: OperatorKeys = this._operatorStack.pop();
        // We can always collapse the stack.
+        assert(this._numberStack.length === 2);


Can you clarify on what this line means?

tolimpo · 2024-10-04T18:35:06Z

src/models/calculator.model.spec.ts

+  // TODO(max): Test exp(negative)
+  // TODO(max): Support exp(exp(n))
+  // TODO(max): Test 1+2+3+4... are they even legal?


Please write all your tests before submitting for code review

tolimpo · 2024-10-04T18:37:43Z

src/models/calculator.model.ts

+        this._numberStack = [result];
+      }
+      if (key !== OperatorKeys.EXP) {
+        // I do not understand what is going on here


I don't understand either, let's find a time to chat about this.

tolimpo · 2024-10-04T18:47:42Z

src/models/calculator.model.ts

  }

-  private pushNumber(): void {
+  private pushBuffer(): void {


Good change. Even though a number is being pushed, it's coming from the buffer, so the buffer is being pushed.

tolimpo · 2024-10-04T18:49:56Z

src/models/calculator.model.ts

+      ) {
+        this._operatorStack.pop();
+        const num: number = this.popNumber();
+        console.log("doing exp", num);


These console logs are good for debugging, but they should be removed before submitting a pull request.

smithsrose1

please look over your code design, and consider using switch statements instead of multiple if statements!

smithsrose1 · 2024-10-04T18:44:03Z

src/models/calculator.model.ts

+          }
        } else if (this.getState() === State.ENTERING_SECOND_OPERAND) {
          const op: OperatorKeys = this._operatorStack.pop();
+          assert(this._numberStack.length === 2);


not sure it would be best to use an assert statement, may cause runtime errors

smithsrose1 · 2024-10-04T18:45:39Z

src/enums/operator-keys.enum.ts

  MINUS = "-",
  MULT = "*",
  DIV = "/",
+  EXP = "exp",


might not be the right value to use for EXP

should use e**

smithsrose1 · 2024-10-04T18:48:29Z

src/models/calculator.model.spec.ts

+    calculator.pressActionKey(ActionKeys.EQUALS);
+    const displayValue: string = calculator.display();
+    expect(displayValue).toEqual("8110.083927575384");
+  });


consider rounding the values to a set decimal place

smithsrose1 · 2024-10-04T18:50:10Z

src/models/calculator.model.ts

+        this.setState(State.ENTERING_SECOND_OPERAND);
+      }
    } else if (this.getState() === State.ENTERING_SECOND_OPERAND) {
      if (key === OperatorKeys.PLUS || key === OperatorKeys.MINUS) {


consider using a switch statement instead of if statements

drntam · 2024-10-04T18:51:50Z

src/models/calculator.model.ts

+        this._numberStack = [result];
+      }
+      if (key !== OperatorKeys.EXP) {
+        // I do not understand what is going on here


Should set the current state as entering into the second phase of operand if the key pressed is anything other than exponent.

drntam · 2024-10-04T18:52:39Z

src/enums/operator-keys.enum.ts

  MINUS = "-",
  MULT = "*",
  DIV = "/",
+  EXP = "exp",


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

drntam · 2024-10-04T18:53:16Z

src/models/calculator.model.spec.ts

+    expect(displayValue).toEqual("56721.58749302769");
+  });
+
+  // TODO(max): Test exp(negative)


If TODO comments won't be implemented later on, do not keep.

support exp operator

afb00b5

markdstouffer reviewed Oct 4, 2024

View reviewed changes

ben-weiss0 reviewed Oct 4, 2024

View reviewed changes

akpav suggested changes Oct 4, 2024

View reviewed changes

KidusYohannesA reviewed Oct 4, 2024

View reviewed changes

michaelp2002 reviewed Oct 4, 2024

View reviewed changes

dark7torm reviewed Oct 4, 2024

View reviewed changes

rupjaisinghani0904 suggested changes Oct 4, 2024

View reviewed changes

kelvinx24 reviewed Oct 4, 2024

View reviewed changes

jli924 reviewed Oct 4, 2024

View reviewed changes

yumio7 suggested changes Oct 4, 2024

View reviewed changes

Balog19 reviewed Oct 4, 2024

View reviewed changes

stephanie-xu reviewed Oct 4, 2024

View reviewed changes

gus-s-42 reviewed Oct 4, 2024

View reviewed changes

kyle12803 suggested changes Oct 4, 2024

View reviewed changes

reginarabkina reviewed Oct 4, 2024

View reviewed changes

faridekhalili reviewed Oct 4, 2024

View reviewed changes

akuatbekov23 reviewed Oct 4, 2024

View reviewed changes

MichaelGadhia reviewed Oct 4, 2024

View reviewed changes

ivng8 reviewed Oct 4, 2024

View reviewed changes

beartackler suggested changes Oct 4, 2024

View reviewed changes

edwinliiiii suggested changes Oct 4, 2024

View reviewed changes

gartastrophe reviewed Oct 4, 2024

View reviewed changes

EdiLau reviewed Oct 4, 2024

View reviewed changes

zachmarino234 suggested changes Oct 4, 2024

View reviewed changes

manavg19 reviewed Oct 4, 2024

View reviewed changes

oorthwein reviewed Oct 4, 2024

View reviewed changes

brantpan1 reviewed Oct 4, 2024

View reviewed changes

nicoleli26 suggested changes Oct 4, 2024

View reviewed changes

aidandomondon reviewed Oct 4, 2024

View reviewed changes

panharathsok3 reviewed Oct 4, 2024

View reviewed changes

LonghaoH reviewed Oct 4, 2024

View reviewed changes

tolimpo reviewed Oct 4, 2024

View reviewed changes

smithsrose1 reviewed Oct 4, 2024

View reviewed changes

drntam reviewed Oct 4, 2024

View reviewed changes

		@@ -23,7 +23,7 @@ export class CalculatorModel implements ICalculatorModel {
		this._buffer += key;

		// TODO(max): Test exp(negative)
		// TODO(max): Support exp(exp(n))

Conversation

YUH-Z commented Oct 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kelvinx24 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!