8
\$\begingroup\$

I am making an app and I want somebody to check my code to maybe make it shorter, fix bugs, or add some things.

<!DOCTYPE html>
<html>
    <head>
    <title>Simple Calculator</title>
<meta name="viewport" content="width=device-width, initial-scale=1">
    <style>
input[type="button"]{
    background:-webkit-gradient( linear, left top, left bottom, color-stop(0.05, #606060), color-stop(1, #606060) );
    background:-moz-linear-gradient( center top, #606060 5%, #606060 100% );
    filter:progid:DXImageTransform.Microsoft.gradient(startColorstr='#606060', endColorstr='#606060');
    background-color:#606060;
    border:1px solid #606060;
    display:inline-block;
    color:#fff;
    font-family:Arial;
    font-size:50px;
    line-height:28px;
    text-decoration:none;
    text-align:center;
    margin-bottom:1.5px;
    margin-left: 1.5px;
    margin-right:1.5px ;
    margin-top:1.5px ;
    height: 75px; 
}
input[type="button"]{
  width: 184px;
}
#btnC{
        width:372.7px;
}
#btn0{
        width:374.7px;
}
#btn0,#btndecimal,#btndivide {
    margin-right: 0.1px;
}
#btn7,#btn4,#btn1,#btn0,#btnequals {
    margin-left: 0.01px;
}        
#btnequals {
    height: 61px;
    width: 944px;
    margin-top: 3px;
}
       input[type="button"]:active {
           position:relative;
            background:#989898;
} 
        input:focus {
            outline:0;
        }
   input[type="Text"] {
       padding-left: 10px;
       margin-bottom: 1.5px;
            font-size: 100px;
            background-color: #202020 ;n
            height:195px;
            width: 935px;
            border:none;
       color:white;
        }
        body {
            background-color: #080808 ;
            overflow: hidden;
        }
        #about {
        font-size: 45px;
        }
    </style>
</head>
<body>
    <FORM name="Keypad" action="">
<input name="ReadOut" id="output" type="Text" size=24 value="0" readonly>
    <table>
<tr>
  <td><input id="btn7" type="Button" value="  7  " onclick="NumPressed(7)"></td>
  <td><input id="btn8" type="Button" value="  8  " onclick="NumPressed(8)"></td>        
  <td><input id="btn9" type="Button" value="  9  " onclick="NumPressed(9)"></td>
<td colspan="2"><input id="btnC" type="Button" value="  C  " onclick="Clear()"></td>
</tr>
<tr>
  <td><input id="btn4" type="Button" value="  4" onclick="NumPressed(4)"></td>
  <td><input id="btn5" type="Button" value="  5   "onclick="NumPressed(5)"></td>        
  <td><input id="btn6" type="Button" value="  6  " onclick="NumPressed(6)"></td>
<td><input id="btnplusminus" type="Button" value=" +/- " onclick="Neg()"></td>
<td><input id="btnplus" type="Button" value="  +  " onclick="Operation('+')"></td>
</tr>
<tr>
  <td><input id="btn1" type="Button" value="  1  " onclick="NumPressed(1)"></td>
  <td><input id="btn2" type="Button" value="  2  " onclick="NumPressed(2)"></td>        
  <td><input id="btn3" type="Button" value="  3  " onclick="NumPressed(3)"></td>
<td><input id="btnmultiply" type="Button" value="  *  " onclick="Operation('*')"></td>
<td><input id="btnminus" type="Button" value="   -   " onclick="Operation('-')"></td>
</tr>
</table>
<input id="btn0" type="Button" value="  0  " onclick="NumPressed(0)">
  <input id="btndecimal" type="Button" value="   .  " onclick="Decimal()">      
<input id="btndivide" type="Button" value="   /   " onclick="Operation('/')">
<input id="about" type="Button" value="About" onclick="myFunction()"></br>
<input id="btnequals" type="Button" value="  =  " onclick="Operation('=')">
 </FORM>
<script>
var FKeyPad = document.Keypad;
var Accumulate = 0;
var FlagNewNum = false;
var PendingOp = "";
function NumPressed (Num) {
if (FlagNewNum) {
FKeyPad.ReadOut.value  = Num;
FlagNewNum = false;
   }
else {
if (FKeyPad.ReadOut.value == "0")
FKeyPad.ReadOut.value = Num;
else
FKeyPad.ReadOut.value += Num;
   }
}
function Operation (Op) {
var Readout = FKeyPad.ReadOut.value;
if (FlagNewNum && PendingOp != "=");
else
{
FlagNewNum = true;
if ( '+' == PendingOp )
Accumulate += parseFloat(Readout);
else if ( '-' == PendingOp )
Accumulate -= parseFloat(Readout);
else if ( '/' == PendingOp )
Accumulate /= parseFloat(Readout);
else if ( '*' == PendingOp )
Accumulate *= parseFloat(Readout);
else
Accumulate = parseFloat(Readout);
FKeyPad.ReadOut.value = Accumulate;
PendingOp = Op;
   }
}
function Decimal () {
var curReadOut = FKeyPad.ReadOut.value;
if (FlagNewNum) {
curReadOut = "0.";
FlagNewNum = false;
   }
else
{
if (curReadOut.indexOf(".") == -1)
curReadOut += ".";
   }
FKeyPad.ReadOut.value = curReadOut;
}
function ClearEntry () {
FKeyPad.ReadOut.value = "0";
FlagNewNum = true;
}
function Clear () {
Accumulate = 0;
PendingOp = "";
ClearEntry();
}
function Neg () {
FKeyPad.ReadOut.value = parseFloat(FKeyPad.ReadOut.value) * -1;
}
function Percent () {
FKeyPad.ReadOut.value = (parseFloat(FKeyPad.ReadOut.value) / 100) * parseFloat(Accumulate);
}
</script>
<script>
function myFunction() {
    alert("TegTech 2014");
}
</script>
</body>
</html>
\$\endgroup\$
6
\$\begingroup\$

You have some layout bugs:

  • The labels for the "4" and "5" keys are misaligned due to careless use of whitespace. I recommend omitting the spaces altogether, e.g.:

    <input id="btn7" type="Button" value="7" …
    
  • You used a <table> for the first three rows of buttons, but not for the last two. As a result, the last two rows may be misaligned by a few pixels on some browsers. Also, the reflow behaviour when the window is too narrow is inconsistent.

Calculator screenshot with layout problems annotated

\$\endgroup\$
  • \$\begingroup\$ I don't know why someone uses spaces when there are other tools to align the text \$\endgroup\$ – phuclv Jun 5 '14 at 11:04
4
\$\begingroup\$

Formatting

You really need to format your code properly, it looks horrible.

this is what it should look like

<!DOCTYPE html>
<html>
    <head>
        <title>Simple Calculator</title>
        <meta name="viewport" content="width=device-width, initial-scale=1">
        <style>
            input[type="button"]{
                background:-webkit-gradient( linear, left top, left bottom, color-stop(0.05, #606060), color-stop(1, #606060) );
                background:-moz-linear-gradient( center top, #606060 5%, #606060 100% );
                filter:progid:DXImageTransform.Microsoft.gradient(startColorstr='#606060', endColorstr='#606060');
                background-color:#606060;
                border:1px solid #606060;
                display:inline-block;
                color:#fff;
                font-family:Arial;
                font-size:50px;
                line-height:28px;
                text-decoration:none;
                text-align:center;
                margin-bottom:1.5px;
                margin-left: 1.5px;
                margin-right:1.5px ;
                margin-top:1.5px ;
                height: 75px; 
            }
            input[type="button"]{
            width: 184px;
            }
            #btnC{
                    width:372.7px;
            }
            #btn0{
                    width:374.7px;
            }
            #btn0,#btndecimal,#btndivide {
                margin-right: 0.1px;
            }
            #btn7,#btn4,#btn1,#btn0,#btnequals {
                margin-left: 0.01px;
            }        
            #btnequals {
                height: 61px;
                width: 944px;
                margin-top: 3px;
            }
            input[type="button"]:active {
                position:relative;
                background:#989898;
            } 
            input:focus {
                outline:0;
            }
            input[type="Text"] {
                padding-left: 10px;
                margin-bottom: 1.5px;
                font-size: 100px;
                background-color: #202020 ;n
                height:195px;
                width: 935px;
                border:none;
                color:white;
            }
            body {
                background-color: #080808 ;
                overflow: hidden;
            }
            #about {
                font-size: 45px;
            }
        </style>
    </head>
    <body>
        <FORM name="Keypad" action="">
            <input name="ReadOut" id="output" type="Text" size=24 value="0" readonly>
            <table>
                <tr>
                    <td><input id="btn7" type="Button" value="  7  " onclick="NumPressed(7)"></td>
                    <td><input id="btn8" type="Button" value="  8  " onclick="NumPressed(8)"></td>        
                    <td><input id="btn9" type="Button" value="  9  " onclick="NumPressed(9)"></td>
                    <td colspan="2"><input id="btnC" type="Button" value="  C  " onclick="Clear()"></td>
                </tr>
                <tr>
                    <td><input id="btn4" type="Button" value="  4" onclick="NumPressed(4)"></td>
                    <td><input id="btn5" type="Button" value="  5   "onclick="NumPressed(5)"></td>        
                    <td><input id="btn6" type="Button" value="  6  " onclick="NumPressed(6)"></td>
                    <td><input id="btnplusminus" type="Button" value=" +/- " onclick="Neg()"></td>
                    <td><input id="btnplus" type="Button" value="  +  " onclick="Operation('+')"></td>
                </tr>
                <tr>
                    <td><input id="btn1" type="Button" value="  1  " onclick="NumPressed(1)"></td>
                    <td><input id="btn2" type="Button" value="  2  " onclick="NumPressed(2)"></td>        
                    <td><input id="btn3" type="Button" value="  3  " onclick="NumPressed(3)"></td>
                    <td><input id="btnmultiply" type="Button" value="  *  " onclick="Operation('*')"></td>
                    <td><input id="btnminus" type="Button" value="   -   " onclick="Operation('-')"></td>
                </tr>
            </table>
            <input id="btn0" type="Button" value="  0  " onclick="NumPressed(0)">
            <input id="btndecimal" type="Button" value="   .  " onclick="Decimal()">      
            <input id="btndivide" type="Button" value="   /   " onclick="Operation('/')">
            <input id="about" type="Button" value="About" onclick="myFunction()"></br>
            <input id="btnequals" type="Button" value="  =  " onclick="Operation('=')">
        </FORM>
        <script>
            var FKeyPad = document.Keypad;
            var Accumulate = 0;
            var FlagNewNum = false;
            var PendingOp = "";
            function NumPressed (Num) {
                if (FlagNewNum) {
                    FKeyPad.ReadOut.value  = Num;
                    FlagNewNum = false;
                }
                else {
                    if (FKeyPad.ReadOut.value == "0")
                        FKeyPad.ReadOut.value = Num;
                    else
                        FKeyPad.ReadOut.value += Num;
                }
            }
            function Operation (Op) {
                var Readout = FKeyPad.ReadOut.value;
                if (FlagNewNum && PendingOp != "=");
                else
                {
                    FlagNewNum = true;
                    if ( '+' == PendingOp )
                        Accumulate += parseFloat(Readout);
                    else if ( '-' == PendingOp )
                        Accumulate -= parseFloat(Readout);
                    else if ( '/' == PendingOp )
                        Accumulate /= parseFloat(Readout);
                    else if ( '*' == PendingOp )
                        Accumulate *= parseFloat(Readout);
                    else
                        Accumulate = parseFloat(Readout);
                    FKeyPad.ReadOut.value = Accumulate;
                    PendingOp = Op;
                }
            }
            function Decimal () {
                var curReadOut = FKeyPad.ReadOut.value;
                if (FlagNewNum) {
                    curReadOut = "0.";
                    FlagNewNum = false;
                }
                else
                {
                    if (curReadOut.indexOf(".") == -1)
                        curReadOut += ".";
                }
                FKeyPad.ReadOut.value = curReadOut;
            }
            function ClearEntry () {
                FKeyPad.ReadOut.value = "0";
                FlagNewNum = true;
            }
            function Clear () {
                Accumulate = 0;
                PendingOp = "";
                ClearEntry();
            }
            function Neg () {
                FKeyPad.ReadOut.value = parseFloat(FKeyPad.ReadOut.value) * -1;
            }
            function Percent () {
                FKeyPad.ReadOut.value = (parseFloat(FKeyPad.ReadOut.value) / 100) * parseFloat(Accumulate);
            }
        </script>
        <script>
            function myFunction() {
                alert("TegTech 2014");
            }
        </script>
    </body>
</html>

While I was reformatting your code I found some code that I wasn't sure if it should be inside the if block or not because you had the if block one lined without brackets.

this bit of code here

function Operation (Op) {
    var Readout = FKeyPad.ReadOut.value;
    if (FlagNewNum && PendingOp != "=");
    else
    {
        FlagNewNum = true;
        if ( '+' == PendingOp )
            Accumulate += parseFloat(Readout);
        else if ( '-' == PendingOp )
            Accumulate -= parseFloat(Readout);
        else if ( '/' == PendingOp )
            Accumulate /= parseFloat(Readout);
        else if ( '*' == PendingOp )
            Accumulate *= parseFloat(Readout);
        else
            Accumulate = parseFloat(Readout);
        FKeyPad.ReadOut.value = Accumulate;
        PendingOp = Op;
    }
}

are these supposed to be inside the previous else block?

 FKeyPad.ReadOut.value = Accumulate;
 PendingOp = Op;

Separate Code into Files

You have plenty of CSS and Javascript, make a Stylesheet and a Javascript file and then import them in the head tag of the HTML file.

Putting all that code together in the HTML file makes this file cluttered and makes it difficult to maintain, so please make separate files for each and then link to them in the head tag of your HTML file.

\$\endgroup\$
2
\$\begingroup\$

HTML:


Update :

I was wrong about this one - stackoverflow.com/a/3558200/567864

Even though html is a permissive markup close all the tags - Some browsers may enter quirks mode and it makes the processing harder for the browser.

Instead of :

<input id="btn7" type="Button" value="  7  " onclick="NumPressed(7)">

Use :

<input id="btn7" type="Button" value="  7  " onclick="NumPressed(7)" />


Table shouldn't be used for layout, they are hard to maintain and style, use div instead.

Semantically speaking, the table tag is meant for listing tabular data. It is not optimized to build structure.


Maintain the same naming style:

  • Here you did upper case for the tag name: <FORM name="Keypad" action="">

  • Here you capitalize type value : <td><input id="btn1" type="Button" value=" 1 " onclick="NumPressed(1)"></td>

  • Here you used camel case : <td colspan="2"><input id="btnC" type="Button" value=" C "onclick="Clear()"></td>

  • But here you didn't for the id : <input id="btnequals" type="Button" value=" = " onclick="Operation('=')">


Verify it for correctness on http://validator.w3.org/#validate_by_input

CSS:


Bad syntax :

background-color: #202020 ;n

That n at the end was it intended? doesn't look valid to me.


Your gradient on input[type="button"] looks weird ... same color at the start and at the end with the background on the same color. That is not a gradient at all.


You used the same declaration twice :

input[type="button"]{

Namespace your style - In case you want to add this to a bigger page your style may affect more than you intended.

JavaScript:


You shouldn't use an empty if with an else just to reverse it.

if (FlagNewNum && PendingOp != "="); 
else
{

Your functions are meaningless if taken apart, you should consider grouping them in an namespace, object or model. Also being global scoped may not go well within a larger page.


Use a standard naming convention : http://en.wikipedia.org/wiki/Naming_convention_%28programming%29 reduce the effort needed to read and understand source code by others and enhances source code appearance.

User Experience:


When it comes to calculators, one can expect to be able to enter values by keyboard as well and not just with the mouse.

\$\endgroup\$
  • 2
    \$\begingroup\$ input is not a paired tag that has to be closed. Also, <blah /> in HTML 5 is just a long way of saying <blah>. Only XHTML uses it as as close tag. stackoverflow.com/a/3558200/567864 \$\endgroup\$ – Corbin Jun 5 '14 at 2:26
  • \$\begingroup\$ On top of that, XHTML should use <blah /> with a space, not <blah/> (as in this answer), to remain HTML compatible (the "/" can be parsed as an attribute with no value). <blah/> is just XML, not XHTML and certainly not HTML. \$\endgroup\$ – Dagg Jun 5 '14 at 2:47
  • \$\begingroup\$ @Corbin Yeah :) ... looks like I'm a XML addict. \$\endgroup\$ – Tiberiu C. Jun 5 '14 at 3:07
  • \$\begingroup\$ @Dagg thanks for pointing that out, I misspelled it. \$\endgroup\$ – Tiberiu C. Jun 5 '14 at 3:14
0
\$\begingroup\$

Here is my solution:

var number = "",
  total = 0,
  regexp = /[0-9]/,
  mainScreen = document.getElementById("mainScreen");

function InputSymbol(num) {
  var cur = document.getElementById(num).value;
  var prev = number.slice(-1);
  // Do not allow 2 math operators in row
  if (!regexp.test(prev) && !regexp.test(cur)) {
    console.log("Two math operators not allowed after each other ;)");
    return;
  }
  number = number.concat(cur);
  mainScreen.innerHTML = number;
}

function CalculateTotal() {
  // Time for some EVAL magic
  total = (Math.round(eval(number) * 100) / 100);
  mainScreen.innerHTML = total;
}

function DeleteLastSymbol() {
  if (number) {
    number = number.slice(0, -1);
    mainScreen.innerHTML = number;
  }
  if (number.length === 0) {
    mainScreen.innerHTML = "0";
  }
}

function ClearScreen() {
  number = "";
  mainScreen.innerHTML = 0;
}
body, div, header, h1, p, table, tr, td {
    margin: 0px;
    padding: 0px;
}

header {
    letter-spacing: 6px;
    font-size: 14px;
}

table {
    width: 100%;
}

button {
    width: 100%;
    height: 50px;
    font-size: 18px;
}

.container {
    margin-top: 100px;
    margin-left: 100px;
    padding: 10px;
    font-family: 'Roboto', sans-serif;
    text-align: center;
    max-width: 400px;
    background-color: silver;
    border: 1px solid black;
    border-radius: 5px;
}

.screen {
    margin-top: 10px;
    margin-bottom: 10px;
    padding: 10px;
    background-color: #2d2929;
    color: white;
    text-align: right;
    font-family: 'Prompt', sans-serif;
}
<html>

<body>
  <div class="container">
    <header>JAVASCRIPT CALCULATOR</header>
    <div class="screen">
      <h1 id="mainScreen">0</h1>
    </div>
    <table>
      <tr>
        <td><button value="7" id="7" onclick="InputSymbol(7)">7</button></td>
        <td><button value="8" id="8" onclick="InputSymbol(8)">8</button></td>
        <td><button value="9" id="9" onclick="InputSymbol(9)">9</button></td>
        <td><button onclick="DeleteLastSymbol()">c</button></td>
      </tr>
      <tr>
        <td><button value="4" id="4" onclick="InputSymbol(4)">4</button></td>
        <td><button value="5" id="5" onclick="InputSymbol(5)">5</button></td>
        <td><button value="6" id="6" onclick="InputSymbol(6)">6</button></td>
        <td><button value="/" id="104" onclick="InputSymbol(104)">/</button></td>
      </tr>
      <tr>
        <td><button value="1" id="1" onclick="InputSymbol(1)">1</button></td>
        <td><button value="2" id="2" onclick="InputSymbol(2)">2</button></td>
        <td><button value="3" id="3" onclick="InputSymbol(3)">3</button></td>
        <td><button value="*" id="103" onclick="InputSymbol(103)">*</button></td>
      </tr>
      <tr>
        <td colspan="2"><button value="0" id="0" onclick="InputSymbol(0)">0</button></td>
        <td><button value="-" id="102" onclick="InputSymbol(102)">-</button></td>
        <td><button value="+" id="101" onclick="InputSymbol(101)">+</button></td>
      </tr>
      <tr>
        <td colspan="2"><button onclick="ClearScreen()">clear</button></td>
        <td colspan="2"><button onclick="CalculateTotal()">=</button></td>
      </tr>
    </table>
  </div>
</body>

</html>

\$\endgroup\$

protected by Community May 19 '15 at 7:54

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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