5
\$\begingroup\$

I've built a calculator. I would not use angular or bootstrap because I am becoming addicted to them. I also wanted to check for double '.' use, and to limit the screens max text length but did not make it in time.

I am new to JavaScript, CSS and HTML and would like to know how I am doing.

 <html>
    <body style="font-size: 40px;  ">


<div style=" max-width: 350px;" class="bordered">
<div class="cont bordered"> <label style="max-width:340px;"  id="screen">welcome!</label> </div>

    <div class="cont">
        <button class="bt" onclick="code(7)">7</button>
        <button class="bt" onclick="code(8)">8</button>
        <button class="bt" onclick="code(9)">9</button>
        <button class="bt" style="margin-left: 50px; " onclick="code('+')">+</button>
        <button class="bt" onclick="code('-')">-</button>
<button class="bt" onclick="clr()">C</button>

        </div>
<div class="cont">
        <button class="bt" onclick="code(4)">4</button>
        <button class="bt" onclick="code(5)">5</button>
        <button class="bt" onclick="code(6)">6</button>
        <button class="bt" style="margin-left: 50px; " onclick="code('*')">*</button>
        <button class="bt" onclick="code('/')">/</button>

        </div>
<div class="cont">
        <button class="bt" onclick="code(1)">1</button>
        <button class="bt" onclick="code(2)">2</button>
        <button class="bt" onclick="code(3)">3</button>

        <button class="bt" style="margin-left: 50px; " onclick="code('%')">%</button>
        <button class="bt" onclick="code('(')">(</button>
        <button class="bt" onclick="code(')')">)</button>

    </div>
<div class="cont">
    <button class="bt" onclick="code(0)" style=" padding-right: 25px; padding-left: 25px;"> 0 </button>

        <button class="bt" style="; margin-left:2px; padding:0 10px 0 10px;" onclick="code('.')">,</button>
    <button class="bt" onclick="compiler()" style=" padding-right: 30px; padding-left: 30px; margin-left: 50px;"> = </button>
</div>

</div>
</body>

<script >
    //regular expression for input controll and users value holder.
var rg = new RegExp(/\d/);
var codestr = "";
//"event listener" for button presses and input controll
var code = function(str){
    var tested = rg.test(codestr.substring(codestr.length-1));
    var sstring = (codestr.substring(codestr.length-1));
    (str=="." && !tested ? 
        codestr = codestr+"0"+str : ((rg.test(str) || str=="(" || str==")") ? 
            codestr = codestr+str : (tested || sstring=="(" || sstring==")") ? 
            codestr= codestr+str: restring(str) )) ;
    document.getElementById('screen').innerHTML = codestr;
};
function restring(str){ codestr = (codestr.substring(0,codestr.length -1))+str };
//user input interpreter ( calculator logic )
var compiler = function(){
     var rezPlz = new Function('return '+codestr)();
     codestr = ""+rezPlz;
    document.getElementById('screen').innerHTML = codestr;
};
//clear button
function clr(){codestr="";
    document.getElementById('screen').innerHTML = "0";}

</script>

</html>
<style rel="stylesheet" type="text/css">
button{
    font-size:40px;
    background-color: #FAFAFA;
}
.cont{
    margin: 5px;
    padding-top:6px
    }
.bt{
    display: inline;
    padding: 0 5px 0 5px;
    margin-top: 0px;
    border: solid;
    border-width: 1px;
    border-color: blue;
    }
.bt:hover{
    background-color: darkblue;
    color:#F0F0F0;  
    }
.bordered{
        border:solid; 
        border-width: 2px; 
        border-color : blue; 
        outline:solid; 
        outline-width:1px; 
        outline-color:aqua;
    }
</style>

I know it's not mobile friendly, but that's what I get for doing a no-bootstrap project in short time.

\$\endgroup\$
5
\$\begingroup\$

Event Delegation

You have soooo many events being attached to your element when you really only need one. Yes, one. It's a technique called event delegation.

document.getElementById('buttons').addEventListener('click', doStuff);

function doStuff(e) {
    if (e.target.tagName.toLowerCase() == 'button') {
        if (e.target.innerHTML == 'C') {
            clr();
        } else {
            code(e.target.innerHTML);
        }
    }
}

function code(operator) {
    console.log(operator);
}

Markup:

<fieldset id="buttons">
    <div>
        <button>7</button>
        <button>8</button>
        <button>9</button>
        <button>+</button>
        <button>-</button>
        <button>C</button>
    </div>
    <!-- etc... -->
</fieldset>

Unsemantic class names

The naming conventions you use are baffling. I would guess that bt is supposed to be an abbreviation for button, but it doesn't make any sense at all when you see that you're also using an element selector to style all buttons (and the only buttons on the page all have the bt class attached to them). I have no idea what cont is supposed to be.

The bordered class just describes the way the element looks, not what it's purpose is. What happens when your calculator needs a facelift and the element shouldn't be bordered anymore?

\$\endgroup\$
  • \$\begingroup\$ Great job explaining event delegation - I'll update my answer to point to this instead. \$\endgroup\$ – Scott Aug 12 '15 at 2:20
  • \$\begingroup\$ it would make sense to have 2 event listeners for clear and others, though. \$\endgroup\$ – njzk2 Aug 12 '15 at 3:18
  • \$\begingroup\$ @njzk2 Why? It makes sense to have separate functions for different behaviors, but I don't see why it makes sense to have separate event handlers. Less event handlers == more efficient. \$\endgroup\$ – cimmanon Aug 12 '15 at 10:55
  • \$\begingroup\$ Less event handlers == more efficient.? I don't see why? I would go document.getElementById('buttons').addEventListener('click', code); and document.getElementById('clear').addEventListener('click', clear); That's it. Just to avoid the ugly if (e.target.tagName.toLowerCase() === 'button') { \$\endgroup\$ – njzk2 Aug 12 '15 at 13:41
  • \$\begingroup\$ also, what if you really had 10, 20, 100 different behaviors? that would be a long handler, not very readable \$\endgroup\$ – njzk2 Aug 12 '15 at 13:42
3
\$\begingroup\$

I applaud you for wanting to move away from frameworks you have come to rely on - it will definitely help you in the long run when it comes to debugging in the future!

Here are a few things I noticed, and that could be improved with your calculator.


HTML

  1. Try to stay away from inline styles. Adding style attributes for single elements might seem like an easy way to quickly apply a style, but it makes your HTML messy and CSS hard to debug. Inline styles will override CSS, which eventually leads to using !important. You also don't give the browser the chance to cache that style, as it will for CSS documents which rarely change.
  2. Try to avoid using onclick attributes to hook up Javascript events to elements. This also falls under the category of Don't Repeat Yourself (DRY), since a lot of the events call the code function. It's possible to handle this with one event and one event listener that looks at the innerText of the caller!
  3. Courtesy of Ismael in the comments: You're missing a doctype! Fortunately, the HTML5 doctype is super easy. Just add <!doctype html> before your <html> tag.
  4. You need to add a <title> tag to your head section, which gives the page the name that appears in the taskbar and tab of a user's web browser.
  5. See if you can save your CSS in an external stylesheet (file.css for example) and reference it in the head section like so: <link rel="stylesheet" href="file.css">

Javascript

  1. There's a loooot of ternary action happening in the code function to the point of making it unreadable. I'm not quite sure what this function even does at first glance - I have to format it extensively to begin to see how it works. It looks like it eventually creates a function from a string, which I think is a pretty clever way to solve the classic calculator problem in JS.
  2. To implement the change I mentioned above about onclick and code, see @cimmanon's answer, which is much better than what I had here previously.

If you apply a specific class to all buttons that call code, you can get rid of that nasty switch completely.

  1. rezPlz is an interesting variable name ;)

And some nitpicky things:

  1. It's good practice to put semicolons after settings variables and expressions - var code = function() { ... }; is an example of this. However, you don't need a semicolon after explicit function declarations, such as function restring(str) { ... };. Source
  2. Because of your use of onclick, the code won't run if the script is found outside the body. In this case, it's between </body> and </html>, which is pretty weird. Including scripts this way (I believe) also forces the browser to wait until the script is done loading before it displays anything to the user!
  3. I'm not sure if it's because you wanted to include all your code in one code block for this question, but having CSS at the end of the document - especially in <style> tags - is just bizarre. This causes a FOUC! Source

Hopefully this helps!

\$\endgroup\$
  • \$\begingroup\$ One thing you can add is that this HTML is missing a doctype, a <head>, a <title> (required for HTML5) and also it has a style completely outside of the closing <html> tag. Other than that, +10! \$\endgroup\$ – Ismael Miguel Aug 12 '15 at 1:53
  • 1
    \$\begingroup\$ @IsmaelMiguel Perfect, I've updated my answer. Thank you! \$\endgroup\$ – Scott Aug 12 '15 at 1:58
  • \$\begingroup\$ this is beautiful answer,thanks for the time you had to waste :p the doctype and the css are messy because I just wanted to get it working, not really a production app :D And for the rest of the pointers, I will take my time with them tomorrow, when I get some sleep. Once again thank you for all this! both of you :) \$\endgroup\$ – Bellator Aug 12 '15 at 2:13
  • 1
    \$\begingroup\$ Actually, you "can't" add a title: in HTML5 you must have a title. And it must have something there (other than a single whitespace). The shortest HTML5 document that validates is <!DOCTYPE html><title>x</title>. Other than that, it looks great! \$\endgroup\$ – Ismael Miguel Aug 12 '15 at 8:15
  • \$\begingroup\$ @Bellator You're welcome. Scott made most of my work. I was going to address the points on the first comment in an answer, but he said almost everything I was going to. So, the right thing would be to just hand over my knowledge \$\endgroup\$ – Ismael Miguel Aug 12 '15 at 8:16
0
\$\begingroup\$

ok, so I have a solution, I am still not happy with all the ifs. I left button class so new types of buttons can be introduced without big effort. now there is only one action listener and it categorizes buttons in 2 groups, i1 and i2 and they have different behavior. I guess its still not perfect but I am happier with this one!

<!doctype>
<html>

<head>
    <title>calcac</title>
    <meta charset="UTF-8">
    <style rel="stylesheet" type="text/css">
        button {
            font-size: 40px;
            background-color: #FAFAFA;
            min-width: 30px;
        }

        .cont {
            margin: 5px;
            padding-top: 6px
        }

        .b-1{
            padding: 0 5px 0 5px;
            margin-top: 0px;
            border: solid;
            border-width: 1px;
            border-color: blue;
        }

        .b-1:hover{
            background-color: darkblue;
            color: #F0F0F0;
        }

        .col-2 {
            margin-left: 50px;
        }

        .padded {
            padding-right: 25px;
            padding-left: 25px;
        }

        .bordered {
            border: solid;
            border-width: 2px;
            border-color: blue;
            outline: solid;
            outline-width: 1px;
            outline-color: aqua;
        }
    </style>
</head>

<body style="font-size: 40px;  ">
    <div style=" max-width: 400px;" class="bordered">
        <div class="cont bordered">
            <label id="screen">welcome!</label>
        </div>
        <fieldset id="buttons">
            <div class="cont">
                <button class="b-1" id="i1">7</button>
                <button class="b-1" id="i1">8</button>
                <button class="b-1" id="i1">9</button>
                <button class="b-1 col-2" id="i2">+</button>
                <button class="b-1" id="i2">-</button>
            </div>
            <div class="cont ">
                <button class="b-1" id="i1">4</button>
                <button class="b-1" id="i1">5</button>
                <button class="b-1" id="i1">6</button>
                <button class="b-1 col-2" id="i2">*</button>
                <button class="b-1" id="i2">/</button>
            </div>
            <div class="cont">
                <button class="b-1" id="i1">1</button>
                <button class="b-1" id="i1">2</button>
                <button class="b-1" id="i1">3</button>
                <button class="b-1 col-2" id="i2">%</button>
                <button class="b-1" id="i1">(</button>
                <button class="b-1" id="i1">)</button>
            </div>
            <div class="cont ">
                <button class="b-1 padded" id="i1">0</button>
                <button class="b-1 " id="i1">.</button>

                <button class="b-1 col-2 padded" id="i2">=</button>
                <button class="b-1" id="i2">C</button>
            </div>
        </fieldset>
    </div>
</body>

</html>

<script>
    var rg = new RegExp(/\d/);
    var codestr = "";
    var sstring = "";
    //ataching event listener to button pad section
    document.getElementById('buttons').addEventListener('click', calcEvent);
    //event listener logic
    function calcEvent(e) {
        var element = e.target.innerHTML;
        var elementClass = e.target.id;
        sstring = (codestr.substring(codestr.length - 1));
        //filtering input
        if (elementClass == 'i1') {
            code(element);
        } else if (elementClass == 'i2') {

            if (element == "=") {
                compiler();
            } else if (element == "C") {
                clr();
                return;
            } else if (sstring == "(" || sstring == ")" || rg.test(sstring)) {
                code(element);
            } else {
                restring(element);
            }
        }
        document.getElementById('screen').innerHTML = codestr;
    } //event linstener

    //update string functions
    function code(str) {
        if (str === '.' && !rg.test(sstring)) {
            codestr = codestr + '0' + str;
        } else {
            codestr = codestr + str;
        }
    }

    function restring(str) {
        codestr = (codestr.substring(0, codestr.length - 1)) + str
    } //update string

    // results and clear functions
    function compiler() {
        var rezPlz = new Function('return ' + codestr)();
        codestr = "" + rezPlz;
        document.getElementById('screen').innerHTML = codestr;
    }

    function clr() {
        codestr = "";
        document.getElementById('screen').innerHTML = "0";
    } //result & clear
</script>
\$\endgroup\$
  • \$\begingroup\$ If you want a further review of your new code, open a new question with it. One thing: ids in a HTML document must be unique. \$\endgroup\$ – RoToRa Aug 19 '15 at 10:59
0
\$\begingroup\$

I have some points for your updated code that could be changed/improved:

CSS You don't need to add the button class b-1 to every button in your markup. It's better to add the class to the parent container and address the button with a css selector >button. Less markup.

HTML IDs of elements have to be unique so it's not recommended adding the id i1 to multiple buttons. Instead add a data-atrribute for your button. e.g. data-type, data-id or everything that makes the intention clear will work.

JS For your if construct you could also use a switch-instruction. That's more readable.

Please have a look at the code below or in this jsfiddle.

var rg = new RegExp(/\d/),
    codestr = "",
    sstring = "";

//ataching event listener to button pad section
document.getElementById('buttons').addEventListener('click', calcEvent);
//event listener logic
function calcEvent(e) {
    var element = e.target.innerHTML,
    	elementClass = e.target.dataset.type,
        braceTest;
    
    sstring = (codestr.substring(codestr.length - 1));
    braceTest = (sstring == "(" || sstring == ")" || rg.test(sstring));
    
    //filtering input
    if (elementClass == 'number') {
        console.log('element added');
        code(element);
    }
    if (elementClass == 'action') {
        switch(element) {
            case '=':
                compiler();
                break;
            case 'DEL':
                codestr = codestr.substring(0, codestr.length - 1);
                break;
            case 'AC':
                clr();
                //return;
                break;
            default:
                if ( braceTest ) {
                    console.log('brace detected or digit');
                    code(element);
                }
                else {
                    restring(element);
                    console.log(codestr, 'rest', element);

                }
        }
    }
    
    document.getElementById('screen').innerHTML = codestr;
} //event linstener

//update string functions
function code(str) {
    if (str === '.' && !rg.test(sstring)) {
        codestr = codestr + '0' + str;
    } else {
        codestr = codestr + str;
    }
}

function restring(str) {
    codestr = (codestr.substring(0, codestr.length - 1)) + str
} //update string

// results and clear functions
function compiler() {
    var rezPlz = new Function('return ' + codestr)();
    codestr = "" + rezPlz;
    document.getElementById('screen').innerHTML = codestr;
}

function clr() {
    codestr = "";
    document.getElementById('screen').innerHTML = "0";
} //result & clear
.wrapper {
    max-width: 400px;
}

button {
    font-size: 40px;
    background-color: #FAFAFA;
    min-width: 30px;
}
.cont {
    margin: 5px;
    padding-top: 6px
}
.b-1>button {
    padding: 0 5px 0 5px;
    margin-top: 0px;
    border: solid;
    border-width: 1px;
    border-color: blue;
}
.b-1>button:hover {
    background-color: darkblue;
    color: #F0F0F0;
}
.col-2 {
    margin-left: 50px;
}
.padded {
    padding-right: 25px;
    padding-left: 25px;
}
.bordered {
    border: solid;
    border-width: 2px;
    border-color: blue;
    outline: solid;
    outline-width: 1px;
    outline-color: aqua;
}
<div class="wrapper bordered">
    <div class="cont bordered">
        <label id="screen">welcome!</label>
    </div>
    <fieldset id="buttons">
        <div class="cont b-1">
            <button data-type="number">7</button> <!-- ids have to be unique -->
            <button data-type="number">8</button>
            <button data-type="number">9</button>
            <button class="col-2" data-type="action">+</button>
            <button data-type="action">-</button>
        </div>
        <div class="cont b-1">
            <button data-type="number">4</button>
            <button data-type="number">5</button>
            <button data-type="number">6</button>
            <button class="col-2" data-type="action">*</button>
            <button data-type="action">/</button>
        </div>
        <div class="cont b-1">
            <button data-type="number">1</button>
            <button data-type="number">2</button>
            <button data-type="number">3</button>
            <button class="col-2" data-type="action">%</button>
            <button data-type="number">(</button>
            <button data-type="number">)</button>
        </div>
        <div class="cont b-1">
            <button class="padded" data-type="number">0</button>
            <button data-type="number">.</button>
            <button class="col-2 padded" data-type="action">=</button>
            <button data-type="action">DEL</button>
            <button data-type="action">AC</button>
        </div>
    </fieldset>
</div>

\$\endgroup\$
  • \$\begingroup\$ It looks much cleaner this way, thanks for the tip on ids! \$\endgroup\$ – Bellator Aug 19 '15 at 9:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.