11
\$\begingroup\$

I wrote a simple Fibonacci checker using HTML, CSS, and JS. This is my first JS program, so be sure to tell me everything that is wrong with it. Check it out here (http://jsfiddle.net/z49fyypt/1/), and here is the runnable code snippet:

<!DOCTYPE html>
<head>
    <title>Fibonacci checker</title>
    <style>
        body {
            font-family: Algerian, Times, Georgian;
            color: green;
            padding-left: 100px;
        }
    </style>
</head>
<body>
    <h1>Enter a Fibonacci number</h1>
    <input id="InputNum" type="number" required="required" />
    <button onclick="TestFibo()">Test Input</button>
    <h2 id="IsFibo"></h2>

    <script>
        function TestFibo()
        {
            InputNum = document.getElementById("InputNum").value;

            var test1 = Math.sqrt(5 * Math.pow(InputNum, 2) + 4);
            var test2 = Math.sqrt(5 * Math.pow(InputNum, 2) - 4);

            if (test1 == Math.floor(test1) || test2 == Math.floor(test2))
                document.getElementById("IsFibo").innerHTML = InputNum + " is a Fibonacci number";
            else
                document.getElementById("IsFibo").innerHTML = InputNum + " is not a Fibonacci number";
        }
    </script>
</body>

\$\endgroup\$
0

8 Answers 8

15
\$\begingroup\$

About this code:

    function TestFibo()
    {
        InputNum = document.getElementById("InputNum").value;

The convention for function names is camelCase, not PascalCase.

The same for variable names. Even more importantly, variables should be declared using the var keyword when used for the first time:

var inputNum = document.getElementById("InputNum").value;

About this code:

if (test1 == Math.floor(test1) || test2 == Math.floor(test2))
    document.getElementById("IsFibo").innerHTML = InputNum + " is a Fibonacci number";
else
    document.getElementById("IsFibo").innerHTML = InputNum + " is not a Fibonacci number";

It would be really better to use braces even on single-line if-else statements. Not doing so can sometimes lead to freaky bugs.

An even worse issue here is this duplicated code in both branches of the if-else:

document.getElementById("IsFibo").innerHTML = InputNum + ...

It would be better to refactor the code in a way that you only have to write this logic once.

The same problem here:

        var test1 = Math.sqrt(5 * Math.pow(InputNum, 2) + 4);
        var test2 = Math.sqrt(5 * Math.pow(InputNum, 2) - 4);

This is more than just duplication of logic, the calculation of 5 * Math.pow(InputNum, 2) is performed twice. Although the performance concerns in this particular program are negligible, as a matter principle and good habits, it would be better to extract the common calculation to a local variable.


I like that your HTML passes all checks on http://validator.w3.org/check

\$\endgroup\$
2
  • \$\begingroup\$ I thought that functions and methods were always in PascalCase and variables were always camelCase? \$\endgroup\$
    – Malachi
    Dec 23, 2014 at 20:38
  • \$\begingroup\$ PascalCase is for class prototypes (actually I don't know the proper term for this, it's for the things that are like classes in other languages (but they are not exactly like them, because they are really prototypes)). \$\endgroup\$
    – janos
    Dec 23, 2014 at 20:39
6
\$\begingroup\$

you should always use Brackets especially when you have an if/else situation

        if (test1 == Math.floor(test1) || test2 == Math.floor(test2))
            document.getElementById("IsFibo").innerHTML = InputNum + " is a Fibonacci number";
        else
            document.getElementById("IsFibo").innerHTML = InputNum + " is not a Fibonacci number";
if (test1 == Math.floor(test1) || test2 == Math.floor(test2)) {
    document.getElementById("IsFibo").innerHTML = InputNum + " is a Fibonacci number";
} else {
    document.getElementById("IsFibo").innerHTML = InputNum + " is not a Fibonacci number";
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Exactly. This helps your code become much more maintainable if it ever reaches a larger scale. \$\endgroup\$ Jun 19, 2018 at 1:32
6
\$\begingroup\$

In general I found your code easy to read, and follow.

When implementing complicated math in an algorithm, it is important to document things carefully.

Why does the following make sense?

        var test1 = Math.sqrt(5 * Math.pow(InputNum, 2) + 4);
        var test2 = Math.sqrt(5 * Math.pow(InputNum, 2) - 4);

        if (test1 == Math.floor(test1) || test2 == Math.floor(test2))
            document.getElementById("IsFibo").innerHTML = InputNum + " is a Fibonacci number";

Where does this function come from, what does it mean?

As for the rest of the code, well done. (Oh, and I even quite like the Christmasy font!)

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

Your JavaScript code indentation is not according to convention. This:

    function TestFibo()
    {
        InputNum = document.getElementById("InputNum").value;

        var test1 = Math.sqrt(5 * Math.pow(InputNum, 2) + 4);
        var test2 = Math.sqrt(5 * Math.pow(InputNum, 2) - 4);

        if (test1 == Math.floor(test1) || test2 == Math.floor(test2))
            document.getElementById("IsFibo").innerHTML = InputNum + " is a Fibonacci number";
        else
            document.getElementById("IsFibo").innerHTML = InputNum + " is not a Fibonacci number";
    }

Should instead be:

    function TestFibo() {
        InputNum = document.getElementById("InputNum").value;

        var test1 = Math.sqrt(5 * Math.pow(InputNum, 2) + 4);
        var test2 = Math.sqrt(5 * Math.pow(InputNum, 2) - 4);

        if (test1 == Math.floor(test1) || test2 == Math.floor(test2))
            document.getElementById("IsFibo").innerHTML = InputNum + " is a Fibonacci number";
        else
            document.getElementById("IsFibo").innerHTML = InputNum + " is not a Fibonacci number";
    }

You can always use http://jsbeautifier.org/ to help make your code look more "standard".

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

This font selection is not that great, IMO:

       font-family: Algerian, Times, Georgian;

Algerian is a pretty uncommon font, and it's difficult to read. A more common pairing would be something like:

        font-family: "Times New Roman", Georgia, Serif;

test1 and test2 are not good variable names. Name them after what they mean, like testPlusFour and testMinusFour.

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

The colour of your font, as well as the font itself (which has already been mentioned), makes the text hard to read. Try something more simple with the font coloured as black:

<style>
    body {
        ...
        color: black;
        ...
    }
</style>
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Whoops! My mistake. I have edited the answer. \$\endgroup\$ Dec 23, 2014 at 20:52
5
\$\begingroup\$

Your function reports that -89 is a Fibonacci number, which I don't believe it is.

One way to prevent the problem is to specify a validation pattern:

<input id="InputNum" type="number" required="required" pattern="\d*" />

This is a good thing to do if you expect a nonnegative integer, because it enables numeric input on iOS devices.

I'd follow through with another check in TestFibo() itself, just in case an older browser ignores the validation pattern.

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

1. Missing html tag

You're missing <html> and </html>. Browsers will implicitly open them as soon as they see the head tag, but it's not correct.

<!DOCTYPE html>
<html>
    <head>
    <!-- ... -->
    </body>
</html>

2. Missing charset

For any HTML document you should atleast specify a charset to ensure characters are displayed correctly.

<head>
    <meta charset="utf-8">
    <!-- ... -->
</head>

3. Misleading ID IsFibo

You use <h2 id="IsFibo"></h2> for your output. The naming of that ID is not very good. Firstly, you use the same element to show a message when a number was not a Fibonacci number.

Also, this element should not be a heading. For what content would that be the heading?

And lastly, HTML classes and ID’s are generally lowercase, dash-delimited names, not PascalCase or camelCase.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.