I'm looking for critiques to see what I could have done better or different ways I could approach writing a script for finding even or odd numbers. I am new to programming with JavaScript, and programming in general. This is one of the first challenges I wrote for finding even or odd numbers.
var numList = [];
while(numList.length < 5){
numList.push(window.prompt());
}
var evenNumbers = [];
numList.forEach(function(element){
if (element % 2 === 0){
evenNumbers.push(element);
}
})
document.write(evenNumbers);
3 Answers 3
You can use Array.prototype.filter.
You can read like this: return an array that only has the elements which satisfies the return function expression.
This way, you can avoid instantiating an array and only after this iteration to make the push
.
var numList = [];
while(numList.length < 5){
numList.push(window.prompt());
}
var evenNumbers = numList.filter(function (element) {
return element % 2 === 0
});
// that's the same that
// var evenNumbers = numList.filter(element => element % 2 === 0);
document.write(evenNumbers);
Although, you can use arrow functions, that has a cleaner syntax.
-
\$\begingroup\$ Thanks for the reply, I did not know about arrow functions. I will start using them to make my syntax cleaner. Array-filter fits this situation and I will most likely be using it again if I end up in a similar scenario. Reading the MDN, forEach and filter both seem similar with exception that filter creates a new array. Filter seems to be more efficient and cleaner than forEach, especially in this case scenario as it seems more effective to the task at hand. \$\endgroup\$fire_hydrant– fire_hydrant2020年08月25日 01:34:33 +00:00Commented Aug 25, 2020 at 1:34
Different ways for finding even or odd numbers.
While the modulo operator works fine for testing if a number is even or odd, a faster technique (which I would not expect a beginner to know about) is to use bitwise AND - i.e. &
. Refer to this article for a thorough explanation of how it works.
function isEven(number) {
return !(number & 1);
}
for (let x = 0; x < 6; x++) {
console.log(x, ' is even: ', isEven(x));
}
Other review aspects
Filtering the array
As Lucas already mentioned Array.prototype.filter()
can be used to simplify the addition of elements into evenNumbers
. Array.prototype.reduce()
could be used though it wouldn’t be as concise as each iteration would need to return the cumulative array, and the initial value would need to be set to an array.
While you didn't ask specifically about performance, if you want the code to be as efficient as possible (e.g. it will be run millions (or more) times in a short amount of time, then avoid iterators - e.g. functional techniques with array.filter()
, array.map()
, as well as for...of
loops - use a for
loop.
Declaring variables
const
could be used instead of var
to avoid accidental re-assignment for both arrays, and if the variables were inside a block, the scope would be limited to the block. Note that "It's possible to push items into the array"1 even if it is declared with const
.
Promoting user for input
window.prompt()
"displays a dialog with an optional message prompting the user to input some text."2 . A friendly message could be passed as the first argument to give the user information about the expected input- e.g.
window.prompt("Please enter a number");
Additionally:
Please note that result is a string. That means you should sometimes cast the value given by the user. For example, if their answer should be a Number, you should cast the value to Number.
const aNumber = Number(window.prompt("Type a number", ""));
So the Number
constructor could be used to store numbers in the array.
numList.push(Number(window.prompt("Please enter a number")));
Sending output with document.write()
Note: Because
document.write()
writes to the document stream, callingdocument.write()
on a closed (loaded) document automatically calls document.open(), which will clear the document.
So don’t plan on using that function on scripts that run on webpages with DOM elements existing on the page, lest they get removed.
-
1\$\begingroup\$ The bitwise suggestion is unnecessary obfuscation. It is much clearer to use the modulus operator \$\endgroup\$Martin Smith– Martin Smith2020年08月21日 08:52:32 +00:00Commented Aug 21, 2020 at 8:52
-
\$\begingroup\$ Is the bitwise operation beneficial in terms of time consumption over modulus operator? @MartinSmith If this is true, I would say that it will be okay, since its wrapped inside a function which clearly explains whats happening by its name. But in the end I guess it comes down to personal preference \$\endgroup\$Olli– Olli2020年08月21日 10:24:29 +00:00Commented Aug 21, 2020 at 10:24
-
1\$\begingroup\$ @MartinSmith while it isn't something a beginner would be expected to be aware of (unless maybe he/she had experience with logic-based programming or low-level programming) the OP stated "I'm looking for critiques to see what I could have done better or different ways I could approach writing a script for finding even or odd numbers." \$\endgroup\$2020年08月25日 15:10:10 +00:00Commented Aug 25, 2020 at 15:10
Use the let ES6 instead of var
var numList = [];
let numList = [];
etc.
Use the filter method
numList.forEach(function(element){ if (element % 2 === 0){ evenNumbers.push(element); } })
As suggested by Lucas Wauke
let evenNumbers = numList.filter(element => element % 2 === 0);
Tell the user what to enter in the prompt
numList.push(window.prompt());
numList.push(window.prompt("Please enter a whole number"));
You should definitely work on your code style to make it better readable
Use indents
while(numList.length < 5){ numList.push(window.prompt()); }
while(numList.length < 5){
numList.push(window.prompt());
}
Leave whitespaces before braces
numList.forEach(function(element){ if (element % 2 === 0){ evenNumbers.push(element); } })
numList.forEach(function(element) {
if (element % 2 === 0) {
evenNumbers.push(element);
}
})
In my opinion its way better readable if you leave a whitespace within parentheses.
I personaly dont know many people doing this but try it out maybe it works for you
numList.forEach(function(element){ if (element % 2 === 0){ evenNumbers.push(element); } })
numList.forEach( function( element ) {
if ( element % 2 === 0 ) {
evenNumbers.push( element );
}
} )
This is especially helpful when working with many parentheses ( pseudo code ):
method(function(method(getter())).setSomething(getSomethingFromSomewhere(somewhere)))
method( function( method( getter() ) ).setSomething( getSomethingFromSomewhere( somewhere ) ) )
As you can see its pretty easy to see which parentheses belong together
Conclusion
Before
var numList = []; while(numList.length < 5){ numList.push(window.prompt()); } var evenNumbers = []; numList.forEach(function(element){ if (element % 2 === 0){ evenNumbers.push(element); } }) document.write(evenNumbers);
After
let numList = [];
while ( numList.length < 5 ) {
numList.push( window.prompt() );
}
let evenNumbers = numList.filter(element => element % 2 === 0);
document.write( evenNumbers );
-
2\$\begingroup\$ I joined this stackexchange just to upvote this answer. \$\endgroup\$JonathanZ– JonathanZ2020年08月21日 12:31:14 +00:00Commented Aug 21, 2020 at 12:31
-
3\$\begingroup\$ Actually 'const' is much more preferred than 'let' in this case. 'let' is used when you want to reassign a variable to a new value, for example when using an index in a loop. \$\endgroup\$Niels– Niels2020年08月21日 13:26:06 +00:00Commented Aug 21, 2020 at 13:26
Explore related questions
See similar questions with these tags.