I wrote a JavaScript and XHTML webpage. I think the format of the output is not better or the validation process is not as it is supposed to be. How can I improve the code? How can I make it more efficient?
There are two text boxes. First, it takes a sentence and returns the length. Second, it takes an integer and returns that number of classical Fibonacci numbers.
<!DOCTYPE html>
<html>
<head>
<titile></title>
</head>
<body>
<script type="text/javascript">
function getStrLen(){
var msgDiv = document.getElementById('textbox1');
document.getElementById('msg').textContent = msgDiv.value.split('').length;
}
var a = 0;
var b = 0;
var c = 1;
var ONE = 1;
function genFibonacci(times){
if(times <=0){return;}
a = b;
b = c;
c = a + b;
document.getElementById('fibseq').textContent += a.toString() + (times > ONE ? ", " : " ");
genFibonacci(times - 1, false);
}
function outFib(){
genFibonacci(document.getElementById('fibText').value);
}
</script>
<button id="lenClick" onclick="getStrLen()">String Length</button>
Enter a sentence or multiple ones: <input type="text" id="textbox1"></input>
<br/>
Number of characters in this sentence is: <span id="msg">
</span>
<br/>
<input type="text" id="fibText"></input>
<button id="fibClick" onclick="outFib()">Generate Fibonacci Sequence</button><div id="fibseq"></div>
</body>
<html>
2 Answers 2
Getting the length of a string
You split the string into characters and then you count the array's length. While this works, there's an even simpler way of doing it, using the length property of a String:
const length = msgDiv.value.length;
Changing getStrLen
You can cache the DOM elements. Then you don't need to re-query them, each time the function is called:
const strlenInput = document.getElementById('textbox1');
const strlenDisplay = document.getElementById('msg');
const getStrLen = () => {
strlenDisplay.textContent = strlenInput.value.length;
}
Now this alone won't work in the head-element of your document, as both elements are not available yet. You can either:
- wait until the DOM is ready to run that script, like
document.addEventListener('DOMContentLoaded', function(event) { [...] }, or - place the Javascript at the end of your page instead
This is explained in detail here: $(document).ready(function(){}); vs script at the bottom of page.
Creating the Fibonacci sequence
genFibonacci() takes one parameter, so you pass an unused parameter, when calling the function recursively:
genFibonacci(times - 1, false);
There's a flaw in your logic: You don't reset the values each time the button is clicked. If the user's input is 1 and they hit the generate button 5 times, the result is not 0 but:
0 1 1 2 3
With this in mind, let's try to change your algorithm, that it won't rely on external variables. We also don't necessarily need recursion here:
const getFibonacciValues = (n) => {
const values = [];
if (n >= 1) {
values.push(0);
}
if (n >= 2) {
values.push(1);
}
for (let i = 2; i < n; ++i) {
values.push(values[i - 2] + values[i -1]);
}
return values;
}
Now we have a function, which purpose is solely to create the values. It doesn't have a clue, how you're going to output them. outFib() is doing this for us now:
const outFib = () => {
const n = fibInput.value;
const values = getFibonacciValues(n);
fibDisplay.textContent = values.join(', ');
}
Now, this still has flaws, so we add a little bit of validation at least:
- use
parseIntto parse the value - check whether the input is at least
1 - check whether the input is a number
The final result could look like this:
const fibButton = document.getElementById('fibClick');
const fibInput = document.getElementById('fibText');
const fibDisplay = document.getElementById('fibseq');
const getFibonacciValues = (n) => {
const values = [];
if (n >= 1) {
values.push(0);
}
if (n >= 2) {
values.push(1);
}
for (let i = 2; i < n; ++i) {
values.push(values[i - 2] + values[i -1]);
}
return values;
};
const outFib = () => {
const n = parseInt(fibInput.value);
if (n < 1 || isNaN(n)) {
fibDisplay.textContent = 'Please insert a number greater than 0';
return;
}
const values = getFibonacciValues(n);
fibDisplay.textContent = values.join(', ');
};
fibClick.addEventListener('click', outFib);<input type="text" id="fibText">
<button id="fibClick">Generate Fibonacci Sequence</button>
<div id="fibseq"></div>Further improvements
This will fail for large inputs – try 2000 for example. You'll get a lot of Infinity.
You can take a look into bigNumber.js or you can set an upper limit, like if (n > 1000) { fibDisplay.textContent = 'Too large'; return; } or you could store all values for your maximum n.
Calculating all numbers over and over again is some overhead you could avoid. You could cache already calculated values, and only add new values if n > values.length. Alternatively you can store all numbers beforehand, which is an increase in memory usage, but your runtime becomes incredibly fast, as all you have left is:
const values = [0, 1, 1, 2, 3, ...];
const output = values.splice(0, n);
My first answer is looking at the JavaScript and as it became fairly long, I'll add another one containing some notices regarding style and markup.
Formatting
Try to indent your code properly:
function getStrLen(){ var msgDiv = document.getElementById('textbox1'); document.getElementById('msg').textContent = msgDiv.value.split('').length; } function outFib(){ genFibonacci(document.getElementById('fibText').value); }
Even if you can shorten things like here:
if(times <=0){return;}
I would format it like this to increase readability:
if (times <= 0) {
return;
}
Variable declaration
Try to use const, let and var keywords when appropriate. You can read more about the differences especially between var und let here: What's the difference between using "let" and "var" to declare a variable in JavaScript?
.
Try to use descriptive variable names. It will be hard to guess in a week what these are:
var a = 0; var b = 0; var c = 1;
HTML
input does not have a closing tag:
<input type="text" id="textbox1"></input>
This is simply:
<input type="text" id="textbox1">
Form elements should actually be wrapped in a form-element or be attached to a form using the form-attribute. While this isn't an issue in all browsers, technically it's invalid HTML.
Explore related questions
See similar questions with these tags.