7
\$\begingroup\$

From HackerRank's 10 Days of JavaScript, Day2: Loops:

Complete the vowelsAndConsonants function in the editor below. It has one parameter, a string,s, consisting of lowercase English alphabetic letters (i.e., a through z). The function must do the following:

First, print each vowel in s on a new line. The English vowels are a, e, i, o, and u, and each vowel must be printed in the same order as it appeared in s. Second, print each consonant (i.e., non-vowel) in s on a new line in the same order as it appeared in s.

The following code is in JavaScript. The code runs perfectly fine, just finding a way to make it better.

function vowelsAndConsonants(s) {
    let sp = s.split("");
    let arr1 =[];
    let arr2 =[];
    for(let i=0;i<sp.length;i++)
    {
        if(sp[i]=='a'||sp[i]=='e'||sp[i]=='i'||sp[i]=='o'||sp[i]=='u'){
            arr1.push(sp[i]);
        }else{
            arr2.push(sp[i]);
        }
    }
    for(let i = 0;i<arr1.length;i++){
        console.log(arr1[i]);
    }
    for(let i = 0;i<arr2.length;i++){
        console.log(arr2[i]);
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ Please clarify what your code is actually doing. Your description is very hard to decipher. \$\endgroup\$
    – Mast
    Jun 8 '20 at 11:06
  • 1
    \$\begingroup\$ Thanks for letting me know. I have edited the description so that it will be easier to decipher. \$\endgroup\$
    – Mohit
    Jun 8 '20 at 11:13
  • \$\begingroup\$ Much better, thank you. \$\endgroup\$
    – Mast
    Jun 8 '20 at 11:34
6
\$\begingroup\$

From a short review;

  • arr1 and arr2 are terrible names, we could call them consonants and vowels
  • accessing sp[i] so often is not efficient, I would store that value in a variable
  • when dealing with lists, consider going functional (using forEach, map, reduce, join etc.)
  • sp is not a great name
  • There is a nicer way to iterate over the characters of a string with for( of )

Finally, I wanted to provide the below as an example where I incorporate the above points;

function vowelsAndConsonants2(s) {
  const vowels = [],
    consonants = [];

  for(const c of s){
    if('aeiou'.includes(c)){
      vowels.push(c);
    }else{
      consonants.push(c);
    }
  }
  console.log(vowels.join("\n"));
  console.log(consonants.join("\n"));
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ in the example code c becomes a global variable because it isn't declared with a scope keyword - I'd recommend const... \$\endgroup\$ Jun 8 '20 at 16:15
  • 1
    \$\begingroup\$ Good point, fixed. \$\endgroup\$
    – konijn
    Jun 8 '20 at 18:04

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.