GuyARoss/review.md

## review.md

      
    Raw
  

              review.md
            
          
    app.js

I might try to avoid having these global variables, lets look into why we need them.
let prevResult = null;
let currentOperation;
So it looks like we are reassigning prevResult in the executeOperation method within the Calculator class. Do we reassign elsewhere?
Ah so we do it as part of our clear procedure.. so I guess the first thing I would do here is rather than just doing these reassignments
globally. So maybe an approach here would be to move the prevResult global variable as a property of the Calculator class. Then
using the constructor to set the default value to undefined.
So the class would end up looking like:
class Calculator {
    prevResult;
    constructor(prevResult = null) {
        this.prevResult = prevResult
    }
}
then we go to use it in the Calculator class we can just call this.prevResult. I think that we can also do the same thing with
the currentOperation global variable, so rather than defining it globally, we just define it as a property in the Calculator class and set the default value
within the constructor.
Some more notes about the Calculator class, I might avoid setting default values that come directly from some dependency as the DOM like on line 5
resultField = document.getElementById('result-field');
this tends to be a pretty big code "smell" (wiki) as it will now be pretty hard to use our calculator class anywhere in the future without
bringing along the dependency of the DOM.
I'd like to stand back real fast and think about what the "calculator" class should be doing. Is it managing the operations? manging the dom?
It should only have one purpose (Single Responsibility Principle and it should do that single thing well and extensively (Open Closed Principle).
So let's take a second, and think about what a calculator comprises of, and design our "Calculator" class around that. Also, let's just drop the DOM, for now, let's make the calculator first, and apply the dom after.
So if I were to list out what a calculator does it would look something like:

Takes some number
Takes some operator
Takes some other number
Applies operator to set of numbers
Outputs the "operated" number

Provided this, I need something to hold my numbers and I need some operator. So let's start with something very basic, and talk about how we can refactor.
class Calculator {
    values = []
    operator = undefined

    set operator(operator) {
        this.operator = operator
    }

    set number(number) {
        // removes first number, puts in the new number 
        this.values.shift() 
        this.values.push(number)
    }

    constructor() {
        values = [0, 0]
    }

    calculate() {
        return applyOperator(operator, values)
    }
}
Where my applyOperator function looks something like:
// you could also use a switch statement here, in this case I am just using a dict or a "truth table". Really just a stylistic thing.
const applyOperator = (operator, values) => ({
    [operator === '+']: values.reduce((a, c) => a + c),
    [operator === '-']: values.reduce((a, c) => a - c),
    [operator === '/']: values.reduce((a, c) => a/c),
    [operator === '*']: values.reduce((a, c) => a * c)
}).true
So maybe something like that, really what I am trying to demonstrate here is that you want to just have your class do what it is intended to do and nothing more, with as little dependencies as possible.
I might try to follow the same pattern of being as little dependencies into your classes as a minor refactor and see what you end up with. As an addition, I think it would be easier if you split out your classes into their own files, so your program is easier to read.