5
\$\begingroup\$

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);
\$\endgroup\$
9
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\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 Aug 25 '20 at 1:34
6
\$\begingroup\$

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 , 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", ""));

3

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, calling document.write() on a closed (loaded) document automatically calls document.open(), which will clear the document.

4

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.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ The bitwise suggestion is unnecessary obfuscation. It is much clearer to use the modulus operator \$\endgroup\$ – Martin Smith Aug 21 '20 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 Aug 21 '20 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\$ – Sᴀᴍ Onᴇᴌᴀ Aug 25 '20 at 15:10
5
\$\begingroup\$

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 ); 
\$\endgroup\$
2
  • 2
    \$\begingroup\$ I joined this stackexchange just to upvote this answer. \$\endgroup\$ – JonathanZ supports MonicaC Aug 21 '20 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 Aug 21 '20 at 13:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.