patternjavascriptModerate
Fibonacci checker
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:
The convention for function names is
The same for variable names.
Even more importantly,
variables should be declared using the
About this code:
It would be really better to use braces even on single-line
Not doing so can sometimes lead to freaky bugs.
An even worse issue here is this duplicated code in both branches of the
It would be better to refactor the code in a way that you only have to write this logic once.
The same problem here:
This is more than just duplication of logic,
the calculation of
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
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.