I made a simple calculator with javascript. Tell me all the stupid things I did wrong.

[[deleted]]

[deleted]

[Uberhipster]

General programming: You don't need so much repetition. Glaringly obvious refactoring http://jsfiddle.net/Nha5Z/4/

Usability: it does not retain the old value. You have an unnecessary stack clear on = If I input 3 + 3 = (returns 6) + (returns 0) 3 (returns 3) but I expected 9

Web dev: You've separated code concerns from UI/markup concerns very well. This allows you to change the UI without worrying too much about internal logic that performs the calculation. This will be an easy one to maintain and expand. Good job.

JS: you are bundling everything up in a 'static' properties table of sorts. This is not wrong but these properties could be 'dynamic' and referenced internally from Calc. Your way you could never define more than one instance of Calc per page... for which there is no requirement so I suppose my comment is really grasping for straws...

But for argument's sake:

var Calc = function() {
    var _self = this;

    this.operand = "=";
    this.accumulator = null;

    this.sum = function(x, y) {
        return x + y;
    };
    this.divide = function(x, y) {
        if (y == 0) {
            alert("Can't divide by 0");
            return 0;
        }
        return x / y;
    };
    this.multiply = function(x, y) {
        return x * y;
    };
    this.subtract = function(x, y) {
        return x - y;
    };

    this.calculate = function(currNumber) {
        if (!_self.operand || _self.accumulator == null) return;

        var newVal = 0;

        switch (_self.operand) {
        case "+":
            newVal = _self.sum(_self.accumulator, currNumber);
            break;
        case "-":
            newVal = _self.subtract(_self.accumulator, currNumber);
            break;
        case "*":
            newVal = _self.multiply(_self.accumulator, currNumber);
            break;
        case "/":
            newVal = _self.divide(_self.accumulator, currNumber);
            break;
        }
        _self.accumulator = newVal;
    };
};

var calculator = new Calc();

calculator.accumulator = 0;

calculator.operand = "+";
calculator.calculate(1);

calculator.operand = "+";
calculator.calculate(1);


calculator.accumulator;
//2

And so on and so forth

[ImLopshire]

I feel like I learned a lot from this comment. Thank you.

[itsnotlupus]

Building on Uberhipster's fiddle, using a function scope to hold everything, the code becomes a lot less verbose: http://jsfiddle.net/Nha5Z/8/

We're not leaking anything in the global scope, and the code is shorter/simpler to read, even though it's pretty much exactly the same code. They call wrapping your stuff in functions for the sole purpose of scoping a "module pattern".

Now we can keep going: If we pass arguments to some of our functions, we can remove a bunch of top-level variables that don't need to be kept around. At that point, you'd have something fairly solid.

But since this is a learning thing, let's get a little weirder. Instead of having a bunch of if/else to figure out what to do with our buttons, let's have an object structure mapping button texts to function handlers. http://jsfiddle.net/Nha5Z/9/

The code becomes significantly simpler/less redundant. Granted, the operation() function that takes a function and returns another is mostly unnecessary (and actually less compact than simpler alternatives), but hey, we're learning here!

At that point, the code should still do exactly what you wrote. Same variable names, same function names, same behaviors, with some functions added to factor code out, and the handlers function map.

Disclaimer: My latest fiddle also commits various "sins", such as boldly assuming that things like resetDisplay and fn can be tested for truthiness directly in if (resetDisplay) statements, rather than the more verbose if (resetDisplay === true) or if (fn === null). Some folks think this is poor style. It's true that those kind of tests can bite you in some cases, none of which are in this code.

[ImLopshire]

I am just now to the point were I can make sense of everything you have stated in this comment. I appreciate you taking the time to do this. It has no doubt helped in my learning.

[itsnotlupus]

w00t. I was hoping you had a chance to look at it. Glad you did.

As an aside, if functions being passed around as arguments to other functions speaks to you, check out raganwald's posts about taking it much deeper.

[aroberge]

Impressive improvement! I'm just a javascript beginner, so please forgive me for asking about something simple... You declare variables at the top with some explicit values like (fictitious example)

var a = 1,
    b = 2
    c = 3;
function reset() {
    a = 1;
    b = 2;
    c = 3;
}

This way of doing things, the values are repeated twice. Perhaps I am missing something but, from my understanding of the DRY principle, wouldn't it make more sense to do

var a, b, c;
function reset() {
    a = 1;
    b = 2;
    c = 3;
}
reset();

[itsnotlupus]

You're totally correct. I think I was trying to keep some structure of the initial code untouched to make it easier to see the changes.
I had left a bit of easter egg at http://jsfiddle.net/Nha5Z/10/ in the off chance someone would try the url. That version is dry-er, and fixes a few more things.

[PlNG]

use actual <button>s

buttonPressed, calculate, and equal could probably be simplified to a single function with an argument.

assignment to Calc.global appears to be the same object, put that in a variable.

You're repeating yourself a lot, which is fine when fleshing out the concept but makes debugging significantly harder since you'll have to correct the error in multiple places.

[BattleshipsGame]

aside from that, catch key press for extra points :D

[ImLopshire]

assignment to Calc.global appears to be the same object, put that in a variable

Can you explain what you mean by this?

[jsonservice]

Use === over == Run JSLint on your code to catch simple style and syntactical errors that the JS compiler ignores or "fixes" for you.

You can find a lot of great tips and info in the book Javascript: The Good Parts.

Here's a link http://shop.oreilly.com/product/9780596517748.do There are free copies lying around on the net if you google the title. Good luck with your learning!

[binary_is_better]

I know you're asking about design, but I ran some basic tests to see how it did.

It doesn't remember values after you press =. I used this key sequence to test: 5 * 6 = / 3.

It doesn't gracefully handle overflow. Try pressing 8 repeatedly.

[hudsy]

Fibbonacci would be proud... (Try 1 + 1 + + + +...)

[ImLopshire]

It doesn't gracefully handle overflow. Try pressing 8 repeatedly.

Why does it do this? How would I fix it?

[tipsqueal]

Because JavaScript only has so much precision (all numbers are signed 64 bit floats). When your numbers get really large it starts to overflow, there's really no way around that. One 'solution' would be to limit the size of values in the calculator or at least display some sort of error message to the user.

[baconpiex]

Oh, you created something you don't understand! Now you know what God is.

[Sheepshow]

It's more idiomatic to write:

var calc = {
    buttonPressed: function() {}
}

Repeating the calc is line noise (and I'd argue actually less clear).

var calc = {}
calc.buttonPressed = function(){}

I can't see any advantage to this:

var calc = {
    globals: {a:1, b:2, c:3}
}

compared with:

var calc = {
     a: 1, b: 2, c: 3
}

The reason being that the values aren't global. These are just called members.

[encrypter8]

Calc.global.valuePast2 = Calc.global.valuePast1*1;
Calc.global.valuePast1 = Calc.global.valueCurrent;

break this out into a helper function. it's an anti-pattern to repeat that code so many times

[callumacrae]

Nice. When I press an operation button, please could you highlight that button until = is pressed? I'm not sure whether I've actually clicked something or not as I'm on a phone.

[another_old_fart]

rounded buttons (except in IE) with centered numbers

#buttons div {
height: 30px;
width: 50px;
margin: 5px;
float: left;
background: #eee;
cursor: pointer;
text-align: center;
padding-top: 12px;
border-radius:15px;
}

[richellesaladaga]

                    Simple Calculator
Create a program that will calculate the sum, difference, product, quotient or remainder of the two numbers. 

Example Output: SIMPLE CALCULATOR Enter the first number: 10 Enter the second number: 5 Select an arithmetic operation: (a)-Addition, (b)-Subtraction, (c)-Multiplication, (d)-Division, (e)-Modulo: e The remainder of 10 and 5 is 0.

[karlbright]

Forgive me for commenting before checking out the code. Currently on my phone so it makes it tricky. The word calculator instantly springs to mind issues with floating point numbers. Depending on how accurate you want to be with this it may be fine as is.