21
\$\begingroup\$

I am very new to HTML and I've decided to attempt to put what I know into an HTML page that allows you to enter any amount of numbers, separated by commas, and would display the sum of all the numbers below.

<!DOCTYPE html>

<html>

<head>
  <title>Sum of two numbers</title>
</head>

<body>
  <h1>Enter any amount of numbers separated by a comma:</h1>
  <input id="nums" type="text" required="required" />
  <button onclick="sum()">Get sum</button>
  <h3 id="sum"></h3>

  <script lang="javascript">
    function sum() {
      sumofnums = 0;
      nums = document.getElementById("nums").value.split(",");
      for (i = 0; i < nums.length; i++) {
        sumofnums += parseInt(nums[i]);
      }
      document.getElementById("sum").innerHTML = sumofnums;
    }
  </script>
</body>

</html>

Are there any bad practices in my code?

\$\endgroup\$

10 Answers 10

15
\$\begingroup\$

HTML or XHTML? Which version? Be clear.

Your file is ambiguous. The usage of the empty element tag syntax <input ... */>* suggests (not mandates) that it's XHTML, whereas the absence of a namespace declaration suggests its not.

I recommend that documents are always clear about whether they're HTML or XHTML. And I recommend XHTML because parsing XHTML is simpler than parsing HTML, and HTML5 is defined in a way that XHTML5 documents are (more or less) a subset of HTML5 documents. The keyword is polyglot syntax.

Maybe in your context it's clear, but there is not sufficient information for us, the reviewers, whether your server would serve it as text/html or application/xhtml+xml.

Note that <!DOCTYPE html> does not sufficiently declare HTML5. The HTML5 specification says that an HTML5 document should have a doctype declaration of the form <!DOCTYPE html>. But it does not declare that everything that has such a doctype declaration is HTML5. It also does not (and cannot) prevent any other specification from also using the <!DOCTYPE html> declaration for some other variant of HTML than HTML5. Furthermore, it permits XHTML5 to also use the very same <!DOCTYPE html> declaration. See this.

I recommend to use the XHTML5 with the polyglot syntax, which is the "common subset" of HTML5 and XHTML5. It is basically HTML5 parsable with an XML parser. I recommend this for multiple reasons.

  • The parsing rules of XML are simpler, therefore automated processing of documents with tools such as XSLT is simpler / possible with the XML syntax.
  • You can create DTDs or XML Schemas that further validate your syntax. XML Schema with XML namespaces allows you to validate the mix of different XML-based languages such as for example XHTML and SVG within a single document.

(It's a pitty that the guys of WhatWG are so ignorant towards XML and its capabilities. It's a pitty that the guys of WhatWG had to reinvent the wheel and declare HTML5 as a markup language of its own right besides SGML and XML. That was completely unnecessary and it only confuses people, as we can see with this discussion about the <!DOCTYPE>.)

Declare encoding

It's recommended to always declare the encoding. And the encoding should be declared within the first 1024 bytes.

There are 4 ways how the server can declare the encoding of a (X)HTML document to the client:

  • HTTP Content-Type header: The server would send something like Content-Type: text/html; charset=UTF-8 or Content-Type: application/xhtml+xml; charset=UTF-8 to the client.
  • XML declaration (only for XHTML, not HTML), as the first line of the document: <?xml version="1.0" encoding="UTF-8"?>.
  • Meta element declaring the Content-Type, like this: <meta http-equiv="Content-Type" content="text/html; charset=UTF-8" /> resp. <meta http-equiv="Content-Type" content="application/xhtml+xml; charset=UTF-8" />
  • Meta element declaring the Charset, like this: <meta charset="UTF-8" />

See this for more information.

Document Outline

The outline of your document looks like this:

  • Enter any amount of numbers separated by a comma:
    • (level skipped)
      • (empty headline)

I doubt that this outline makes sense.

Keep in mind that <h1/>, <h2/> etc. are for headlines. If you just want a paragraph with big text but without the semantics of a headline, use a normal paragraph instead and style it with CSS.

Also, skipping levels is not recommended. For a <h*N*/> element, the next heading should have a level not greater than N+1.

How to declare the scripting language

The correct way to declare the scripting language on a <script/> element is like this:

<script type="text/javascript">
    // ...
</script>

If you know that the User Agent supports HTML5 or behaves as expected, you can actually omit the declaration of the scripting language:

The default, which is used if the attribute is absent, is "text/javascript".

The lang attribute is actually meant for describing the content language, like this:

<h1 lang="en">Hi, <span lang="es">Hola</span>, <span lang="de">Hallo und <span lang="de-BY">Servus</span></span>!</h1>

What you did, actually declares the contents of the <script/> element to be in a non-existent natural language named JavaScript.

Naming Conventions

Because JavaScript and Java were somehow born together, they follow identical naming conventions. Which means, we also use camelCase for variables and functions in JavaScript.

Your variable sumofnums should be named sumOfNums.

Local vs Global Variables

In JavaScript, whenever you assign a variable without declaration, it is implicitly global. Local variables need to be declared explicitly, using the var keyword.

You should declare your variables, like this:

var sumOfNums = 0;
var nums = document.getElementById("nums").value.split(",");

The same is true for i. It can be declared at the for-loop, like in Java:

for (var i = 0; i < nums.length; i++) {
  sumOfNums += parseInt(nums[i]);
}

However, be aware that scopes in JavaScript are only global or function-local. Blocks do not define new scopes for variables.

User Input Validation

If the user inputs crap, you might want to tell the user instead of silently failing. JavaScript supports regular expressions, in case you want to go for that. But anyhow, you want to deal with parseInt() failing.

JavaScript supports exception handling quite similar to Java with a try-catch-finally, you might want to go for that and tell the user about bogus input.

The required attribute of <input/>

The required attribute of <input/> is for form submission. Because you use the <input/> element outside a form, there is no form submission, therefore the required attribute doesn't make sense.

Indentation

Consistent indentation is paramount, and you follow it. There is no official standard or convention for indentation in JavaScript.

While the community has taken a lot of rules from Java, including the 4 spaces indentation, some Software Craftsmen like Robert C. Martin suggest that indentation nowadays should actually be 2 spaces rather than 4.

Strict mode

JavaScript is in its core ECMA-262, aka ECMAScript. Strictly speaking, JavaScript was Netscape's name for its implementation of ECMAScript.

ECMA-262 defines a strict mode (10.1.1) which can be enabled with the use strict directive. The most notable differences are:

  • In strict mode, usage of the future reserved words implements, interface, let, package, private, protected, public, static and yield is an error. (7.6.1.2)
  • Octal literals are not supported. (7.8.3)
  • Octal escape sequences are not supported. (7.8.4)
  • functions and variables declared eval() code must be declared within a new variable environment that is accessible to the eval code only. (10.4.2.1)
  • The delete operator is limited. (11.4.1)
  • Assignment are stricter. (11.13.1)
  • eval and arguments are not allowed as variable names (12.2.1), including exception variables (12.14.1).
  • with is not allowed. (12.10) A full list can be seen in Annex C.

You might want to make it a habit to program in strict mode always. It ensures greater compatibility of your source code with future language directions.

\$\endgroup\$
  • 6
    \$\begingroup\$ I don't think it is ambiguous at all. It is HTML5, as marked by the header. Self-closing input elements are valid. I really wish that you correct your answer, because it is otherwise very good. \$\endgroup\$ – ANeves Dec 30 '14 at 20:50
  • 3
    \$\begingroup\$ I don't refer to W3Schools. Never did. And never will. And the header does not say it's HTML5. The header says it's HTML, and the HTML5 specification says that's allowed for HTML5. The HTML5 specification does not say that this header discriminates it as HTML5 from all other versions of HTML and XHTML. \$\endgroup\$ – Christian Hujer Dec 30 '14 at 20:59
  • 4
    \$\begingroup\$ <!DOCTYPE html> does not unambiguously declare HTML5. The combination of <!DOCTYPE html> served as Content-Type: text/html suggests nowadays that it is HTML5. <!DOCTYPE html> can also be used for XHTML, and <!DOCTYPE html> served as Content-Type: application/xhtml+xml would be XHTML5. <!DOCTYPE html> is, from a perspective of SGML and XML, no way precise enough to claim that it is HTML5 and HTML5 only. Actually, <!DOCTYPE html> precisely means this: "some form of HTML without further formal declaration". \$\endgroup\$ – Christian Hujer Dec 30 '14 at 22:43
  • 2
    \$\begingroup\$ "The usage of the empty element tag syntax <input ... /> suggests (not mandates) that it's XHTML" <- Repeating this over and over is not going to make it any more true than it was when you said it the first time. Send the source through the W3C validator and see what it says. "Error found while checking this document as HTML5!" (hint: and it's not because of the self closing tags). While you do make some good points, the rest of your review is controversial BS as the others have already pointed out. It's unclear as to why the "Strict Mode" section is relevant. \$\endgroup\$ – cimmanon Jan 1 '15 at 17:08
  • 4
    \$\begingroup\$ I feel that this discussion of the differences between HTML5 and XHTML5 and whatnot is unnecessary; I haven't heard of XHTML since the days of HTML 4.0. You even say that the current code could be XHTML5 with the right Content-Type header, so the code is not incorrect. Why pontificate on these obscure, mostly irrelevant issues to a beginning programmer? \$\endgroup\$ – gengkev Jan 2 '15 at 6:37
14
\$\begingroup\$

Your JS has a few problems. In the first place, you should always define your variables with the var keyword, or you can have major problems debugging your program (using var to declare variables):

sumofnums = 0;

should become:

var sumofnums = 0;

You do your braces correctly, but sumofnums would be easier to read as sumOfNums. Also, JSLint thinks we should merge the first two var declarations. Additionally, you should use a four spaces per level of indentation, not two:

function sum() {
    var sumOfNums = 0, nums = document.getElementById("nums").value.split(",");

    for (var i = 0; i < nums.length; i++) {
        sumOfNums += parseInt(nums[i]);
    }

    document.getElementById("sum").innerHTML = sumOfNums;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ I'd say whether indentation is 4 spaces or 2 spaces is rather a matter of taste, as there is no official convention for that regarding (X)HTML and JavaScript, AFAIK. \$\endgroup\$ – Christian Hujer Dec 30 '14 at 19:46
  • 1
    \$\begingroup\$ 4 is pretty standard across most of the development world: in cases where there's no other standard, it's a safe default \$\endgroup\$ – Jon Story Dec 30 '14 at 20:53
  • 1
    \$\begingroup\$ Even more important would be to localize i. \$\endgroup\$ – 200_success Dec 30 '14 at 23:22
11
\$\begingroup\$

You have a problem in your HTML, according to the official validator at W3C:

This is not valid:

<script lang="javascript">

It should be this:

<script type="text/javascript">

Because lang attributes are supposed to specify the language of the webpage, they are not allowed to be longer than 8 characters, according to the W3C validator: Bad value javascript for attribute lang on element script: Subtags must not exceed 8 characters in length.

The correct use for this attribute would be to use it as this for a Spanish webpage:

lang="es"

In American English:

lang="en-us"
\$\endgroup\$
  • \$\begingroup\$ Just to be precise, the lang attribute specifies the language of the element content. This only happens to be the entire page if the lang attribute appears on the root element (which usually is <html/>). But it can also be used for mixing languages within a single document. \$\endgroup\$ – Christian Hujer Dec 30 '14 at 22:34
  • 2
    \$\begingroup\$ I think it was a botched attempt to use the deprecated <script language="javascript"> syntax of HTML 3.2. \$\endgroup\$ – 200_success Dec 30 '14 at 23:22
10
\$\begingroup\$

Comments w.r.t the algo and not the javascript practises:

  • Don't trust the user. Suppose I enter, or 0,, shouldn't you provide me 0 rather than NaN?
  • Provide valid error message for error conditions like empty input or invalid cases as above.
  • Does parseInt take care of characters? Does it throw exceptions? Should you catch it in the outer loop? Should you provide any error message or continue by ignoring it?
\$\endgroup\$
6
\$\begingroup\$

parseInt only allows integers and truncates all decimal entries, so entering .5,.5 throws a NaN, and 0.5,0.5 returns 0 instead of 1. Either modify your instructions to tell the user you only accept integers or (even better) change to parseFloat to allow non-integer numbers.

\$\endgroup\$
6
\$\begingroup\$

Referencing this informative post, there are a number of issues with your HTML5:

  1. There's a potential security flaw if you do not include an encoding. All content on the page is decoded using the specified encoding. If the decoding uses the wrong charset it may lead to scripts with errors, and allow unexpected consequences with things like cross-site-references when URL's are decoded differently depending on system defaults and expectations. For example, Chris Shiflett shows how a mismatch between the actual encoding of the page, and the way the system interprets the page, can lead to XSS attack vectors. Specifying the encoding as part of the page header would remove that vector.
  2. For accessibility reasons you should include the language

Your header for the HTML5 should be:

<!DOCTYPE html>
<html lang=en>
<head>
  <meta charset=utf-8>
  <title>Sum of two numbers</title>
</head>
.....
\$\endgroup\$
  • \$\begingroup\$ The question clearly was XHTML. Your answer is HTML. You should fix your answer to match the question. \$\endgroup\$ – Christian Hujer Dec 30 '14 at 19:45
  • 7
    \$\begingroup\$ @ChristianHujer - I am not convinced. The DOCTYPE is for HTML5, (for XHTML it would need to be: <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd"> ). The question is tagged html and not xhtml. Are you sure? \$\endgroup\$ – rolfl Dec 30 '14 at 19:47
  • \$\begingroup\$ I am not 100% sure, it is a guess based on the usage of EmptyElemTag <input ... */>*. \$\endgroup\$ – Christian Hujer Dec 30 '14 at 19:51
  • 4
    \$\begingroup\$ <input ... /> can be used in HTML5, I don't know why it would make it XHTML. I think DOCTYPE is more important than EmptyElemTag. See dev.w3.org/html5/html-author/#void \$\endgroup\$ – Marc-Andre Dec 30 '14 at 20:53
  • \$\begingroup\$ <input.../> makes this part HTML5 polyglot, which is HTML5 and XHTML5 at the same time (just XML Namespace is missing). And there's no reason to break the XML-compatible Polyglot syntax by adding non-XML stuff for no reason. \$\endgroup\$ – Christian Hujer Dec 31 '14 at 23:42
5
\$\begingroup\$

Header

There are things you can do to improve your header. This:

<head>
  <title>Sum of two numbers</title>
</head>

Should at least have this added for a charset:

<head>
  <title>Sum of two numbers</title>
  <meta charset="utf-8">
</head>

CSS

You could use some CSS to improve/customize how your documented is presented. This would make it where it would look more similar across browsers/platforms. For example:

<body style="font-family: Times New Roman, serif; font-color: black;">

You could also make it into a separate .CSS file, so if you added more documents it would be reused.

<link href="my-pretty-website.css" rel="stylesheet">
\$\endgroup\$
  • \$\begingroup\$ The question clearly was XHTML. Your answer is HTML. You should fix your answer to match the question. \$\endgroup\$ – Christian Hujer Dec 30 '14 at 19:44
  • 2
    \$\begingroup\$ @ChristianHujer You should probably delete all your comments about the question being XHTML when it's clearly HTML5 (according to the DOCTYPE). \$\endgroup\$ – MiniRagnarok Dec 30 '14 at 21:06
  • \$\begingroup\$ Specifying the foreground color without specifying the background color is a very bad habit. If the user has a user stylesheet which defines foreground white and background black, and you only define foreground black, the user gets black on black - nice! \$\endgroup\$ – Christian Hujer Dec 30 '14 at 22:31
  • \$\begingroup\$ Plus, the CSS property name for the foreground color is color, not font-color. \$\endgroup\$ – Christian Hujer Dec 30 '14 at 22:31
  • 1
    \$\begingroup\$ @ChristianHujer What's XHTML5? programmers.stackexchange.com/questions/149839/… \$\endgroup\$ – MiniRagnarok Dec 31 '14 at 14:32
5
\$\begingroup\$

If you go very deep into details of programming with all finesse, amongst all the things that the others have mentioned, always use ++i over i++ if it's the third argument of for loops.

The reason is actually a bit tricky to explain for people not yet deeply into programming, especially in JavaScript, since it's lacking the call by reference concept that'd come in handy for the explaination. But I'll try:

i++ and ++i are unary operators. They are compiled into (or in case of JavaScript interpreted as) statements what to do with the variable value. While ++i increases the value and then uses the new value of i, i++ first uses the value of i but then increases it afterwards.

Imagine these operators were functions with a return value, and furthermore that these functions were able to modify the value of their arguments in a way that these changes persisted even if you leave the function again. That concept is called call by reference. Now, with this concept, we can have a look at how the unary operators would look like.

First the one for ++i:

function operatorPlusPlusVar ( i ) {
    i = i + 1; // Assign the new, incremented value to i
    return i; // Return the new value
}

Now the one for i++:

function operatorVarPlusPlus ( i ) {
    var oldValue = i;
    i = i + 1; // Assign the new, incremented value to i
    return oldValue; // Return the old value
}

As you can see, the i++ statement needs to store the original value of i so it can return the old value in spite of i having its new value assigned to it already when leaving the function. That means, on execution, you need one to find a space in the RAM to store the old value (var oldValue), and then copy the old value to that memory spot (= i), on top of everything that ++i has to do as well.

Since the third part of a for loop is executed after the loop's body was run, and you're not using the return value of the ++ operator (you want i to be incremented, you don't care what the ++ statement itself returns since you don't use its return value), you're basically wasting memory and run time if you use i++ instead of ++i.

Does that show in performance? No, not in this example. Does it show in memory usage? No, not in this example. Why am I telling you this, then? Because I hope to motivate you to always look under the hood of programming, and it's never wrong to know exactly what you do and why. Cheers and a happy new year!

\$\endgroup\$
  • 2
    \$\begingroup\$ Most compilers and runtime environments today are smart enough to see that the value of i in i++ is not used and therefore compile / interpret i++ as if it were written ++i. This is verified with GCC (4.5.2 with -O0), javac (you can use javap -c to see) and Keil C251 (a Compiler which is known to be crap otherwise). So, while what you're writing is technically correct, it is irrelevant today and a case of premature optimization. \$\endgroup\$ – Christian Hujer Dec 31 '14 at 2:30
4
\$\begingroup\$

Strict mode

The first JS suggestion is always to set your JS scripts to strict mode by making their first line "use strict";.

This will immediately avoid such problems as your accidental pollution of the global namespace, for example.

JSLint

Then pass your JS code through JSLint, and address its suggestions.

Number parsing

Your number parsing is not very solid.

I suggest using the unary operator +: var number = + nums[i]; sumofnums += number;, but there are many options: https://stackoverflow.com/questions/1133770/how-do-i-convert-a-string-into-an-integer-in-javascript

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

JSLint recommends the following:

JSLint does not recommend use of the for statement. Use array methods like forEach instead. The for option will suppress some warnings. The forms of for that JSLint accepts are restricted, excluding the new ES6 forms.

The reduce method would be best in this situation:

For example:

<!DOCTYPE html>

<html>

<head>
  <title>Sum of two numbers</title>
</head>

<body>
  <h1>Enter any amount of numbers separated by a comma:</h1>
  <input id="nums" type="text" required="required" />
  <button onclick="sum()">Get sum</button>
  <h3 id="sum"></h3>

  <script lang="javascript">
    function reducer(accumulator, current) {
      return Number(accumulator) + Number(current);
      }
    
    function sum() {
      var nums = document.getElementById("nums").value.split(",");
      var sumofnums = nums.reduce(reducer);
      document.getElementById("sum").innerHTML = sumofnums;
    }
  </script>
</body>

</html>

References

\$\endgroup\$

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.