HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptModerate

Fibonacci checker

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
checkerfibonaccistackoverflow

Problem

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:





Fibonacci checker

body {
font-family: Algerian, Times, Georgian;
color: green;
padding-left: 100px;
}


Enter a Fibonacci number

Test Input



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";
}

Solution

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

Code Snippets

function TestFibo()
    {
        InputNum = document.getElementById("InputNum").value;
var inputNum = document.getElementById("InputNum").value;
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";
document.getElementById("IsFibo").innerHTML = InputNum + ...
var test1 = Math.sqrt(5 * Math.pow(InputNum, 2) + 4);
        var test2 = Math.sqrt(5 * Math.pow(InputNum, 2) - 4);

Context

StackExchange Code Review Q#74686, answer score: 15

Revisions (0)

No revisions yet.