Code Review - Javascript Calculator

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.

Oh wow this is awesome! Wasn't expecting anything this in depth! I'm about to go to sleep but I'll refactor with your advice in mind!

The reason I had those global variables was because when I tried to define them inside the constructor of the Calculator class it would only work when I was only doing the same type of operation, as soon as I did say a subtraction after an addition it wouldn't work. I knew it was wrong, but couldn't think of a better way.

I need to dive deeper into getters and setters, I'm having trouble getting my head around them and knowing when to use them. Only briefly learned about them so far.

The applyOperator function looks quite foreign to me, so I'll do some digging to learn more about that syntax. Again I know a bit about reduce but not super comfortable with it yet.

Thanks again! This is really helpful!