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" type="Text" size=24 value="0" readonly> <table><tr> <td><input type="Button" value=" 7 "></td> <td><input type="Button" value=" 8 "></td> <td><input type="Button" value=" 9 "></td><td colspan="2"><input type="Button" value=" C "></td></tr><tr> <td><input type="Button" value=" 4"></td> <td><input type="Button" value=" 5 "onclick="NumPressed(5)"></td> <td><input type="Button" value=" 6 "></td><td><input type="Button" value=" +/- "></td><td><input type="Button" value=" + "></td></tr><tr> <td><input type="Button" value=" 1 "></td> <td><input type="Button" value=" 2 "></td> <td><input type="Button" value=" 3 "></td><td><input type="Button" value=" * "></td><td><input type="Button" value=" - "></td></tr></table><input type="Button" value=" 0 "> <input type="Button" value=" . "> <input type="Button" value=" / "><input type="Button" value="About"></br><input type="Button" value=" = "> </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;elseFKeyPad.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);elseAccumulate = 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>
4 Answers4
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 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.
- \$\begingroup\$I don't know why someone uses spaces when there are other tools to align the text\$\endgroup\$phuclv– phuclv2014-06-05 11:04:32 +00:00CommentedJun 5, 2014 at 11:04
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" type="Text" size=24 value="0" readonly> <table> <tr> <td><input type="Button" value=" 7 "></td> <td><input type="Button" value=" 8 "></td> <td><input type="Button" value=" 9 "></td> <td colspan="2"><input type="Button" value=" C "></td> </tr> <tr> <td><input type="Button" value=" 4"></td> <td><input type="Button" value=" 5 "onclick="NumPressed(5)"></td> <td><input type="Button" value=" 6 "></td> <td><input type="Button" value=" +/- "></td> <td><input type="Button" value=" + "></td> </tr> <tr> <td><input type="Button" value=" 1 "></td> <td><input type="Button" value=" 2 "></td> <td><input type="Button" value=" 3 "></td> <td><input type="Button" value=" * "></td> <td><input type="Button" value=" - "></td> </tr> </table> <input type="Button" value=" 0 "> <input type="Button" value=" . "> <input type="Button" value=" / "> <input type="Button" value="About"></br> <input type="Button" value=" = "> </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 blockone 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 previouselse 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 thehead 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 thehead tag of your HTML file.
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 type="Button" value=" 7 ">Use :
<input type="Button" value=" 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
typevalue :<td><input type="Button" value=" 1 "></td>Here you used camel case :
<td colspan="2"><input type="Button" value=" C "onclick="Clear()"></td>But here you didn't for the id :
<input type="Button" value=" = ">
Verify it for correctness onhttp://validator.w3.org/#validate_by_input
CSS:
Bad syntax :
background-color: #202020 ;n
Thatn at the end was it intended? doesn't look valid to me.
Your gradient oninput[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.
#"="); 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.
- 2\$\begingroup\$
inputis 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– Corbin2014-06-05 02:26:54 +00:00CommentedJun 5, 2014 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– Dagg2014-06-05 02:47:29 +00:00CommentedJun 5, 2014 at 2:47 - \$\begingroup\$@Corbin Yeah :) ... looks like I'm a XML addict.\$\endgroup\$Tiberiu C.– Tiberiu C.2014-06-05 03:07:47 +00:00CommentedJun 5, 2014 at 3:07
- \$\begingroup\$@Dagg thanks for pointing that out, I misspelled it.\$\endgroup\$Tiberiu C.– Tiberiu C.2014-06-05 03:14:21 +00:00CommentedJun 5, 2014 at 3:14
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> <header>JAVASCRIPT CALCULATOR</header> <div> <h1>0</h1> </div> <table> <tr> <td><button value="7">7</button></td> <td><button value="8">8</button></td> <td><button value="9">9</button></td> <td><button>c</button></td> </tr> <tr> <td><button value="4">4</button></td> <td><button value="5">5</button></td> <td><button value="6">6</button></td> <td><button value="/">/</button></td> </tr> <tr> <td><button value="1">1</button></td> <td><button value="2">2</button></td> <td><button value="3">3</button></td> <td><button value="*">*</button></td> </tr> <tr> <td colspan="2"><button value="0">0</button></td> <td><button value="-">-</button></td> <td><button value="+">+</button></td> </tr> <tr> <td colspan="2"><button>clear</button></td> <td colspan="2"><button>=</button></td> </tr> </table> </div></body></html>
You mustlog in to answer this question.
Explore related questions
See similar questions with these tags.