1
\$\begingroup\$

I completed a short coding challenge and just want to know if I should use shorter methods of which I am unaware, or make it more readable.

 /*
 *
 * Complete the vowelsAndConsonants function.
 * Print your output using 'console.log()'.
 */
function vowelsAndConsonants(s) {
var strConsonants = "";
var strVowels = "";
var i;

for (i in s) {
    if (s.charAt(i) == "a" || s.charAt(i) == "e" || s.charAt(i) == "i" || 
s.charAt(i) == "o" || s.charAt(i) == "u") {
        strVowels += s.charAt(i);
    }
    else if (s.charAt(i) != "a" || s.charAt(i) != "e" || s.charAt(i) != "i" 
|| s.charAt(i) != "o" || s.charAt(i) != "u") {
        strConsonants += s.charAt(i);
    }
}

// console.log(strVowels);

i = 0;
for (i in strVowels) {
    console.log(strVowels.charAt(i));
}

// console.log(strConsonants);

i = 0;
for (i in strConsonants) {
     console.log(strConsonants.charAt(i));
}

}
\$\endgroup\$
5
\$\begingroup\$

Instead of providing you a "better" solution, I will try to give you some feedback the code.


Block indentation

function vowelsAndConsonants(s) {
var strConsonants = "";
var strVowels = "";
var i;
// ..

To work with code it is important that it is formatted in a readable way. There are some style guides out there for example from google

function vowelsAndConsonants(s) {
  var strConsonants = "";
  var strVowels = "";
  var i;
  // ..

Type Embedded in Name

var strConsonants = "";
var strVowels = "";

In this variable names the type is embedded in the name

Avoid placing types in method names; it's not only redundant, but it forces you to change the name if the type changes.


Array iteration and for...in

For your task is the order important:

Input string, output vowels and consonants to log, separately but in order

You can read on MDN about the for..in loop,that it do not guaranties a traversal in order:

Note: for...in should not be used to iterate over an Array where the index order is important. [...]

[...] iterating over an array may not visit elements in a consistent order. Therefore, it is better to use a for loop with a numeric index (or Array.prototype.forEach() or the for...of loop) when iterating over arrays where the order of access is important.


Make the Else-Statement Implicit

if (s.charAt(i) == "a" || 
    s.charAt(i) == "e" || 
    s.charAt(i) == "i" || 
    s.charAt(i) == "o" || 
    s.charAt(i) == "u") {
  strVowels += s.charAt(i);
} else if (s.charAt(i) != "a" || 
           s.charAt(i) != "e" || 
           s.charAt(i) != "i" || 
           s.charAt(i) != "o" || 
           s.charAt(i) != "u") {
  strConsonants += s.charAt(i);
}

Currently the if-else statement tries to express: If you are a, e, i, o, u do something, else if you are not from a, e, i, o, u do something.

This is semanticly the same as: If you are a, e, i, o, u do something, else do something.

Additional we can wrap the condition of the vowels into its own method to make the code more readable.

function isVowel(letter) {
    return letter === "a" || 
           letter === "e" || 
           letter === "i" || 
           letter === "o" || 
           letter === "u"
}

The if-statement could now look like

var letter = s.charAt(i)
if (isVowel(letter)) {
  strVowels += letter;
} else {
  strConsonants += letter;
}

String Concatenation..

strVowels += letter

Every time Strings get merged by + a new String gets created, because Strings are immutable, that means that for each concatenation new memory space gets allocated.

Better would be to use an array instead of a string an push into it.

vowels.push(letter)

Example Refactoring

function isVowel(letter) {
    return letter === "a" ||
           letter === "e" ||
           letter === "i" ||
           letter === "o" ||
           letter === "u"
}

function vowelsAndConsonants(s) {
    var consonants = [];
    var vowels = [];

    for (var letter of s) {
        if (isVowel(letter)) {
            vowels.push(letter)
        } else {
            consonants.push(letter)
        }
    }

    for (var vowel of vowels) {
        console.log(vowel);
    }

    for (var constant of consonants) {
        console.log(constant);
    }
}

and from here you can still use some methods like forEach or using the ternary operator to shorten an if-else

function isVowel(letter) {
    return letter === "a" ||
           letter === "e" ||
           letter === "i" ||
           letter === "o" ||
           letter === "u"
}

function print(x) {
    console.log(x)
}

function vowelsAndConsonants(s) {
    var consonants = [];
    var vowels = [];

    for (var letter of s) {
        isVowel(letter)
            ? vowels.push(letter)
            : consonants.push(letter)
    }

    vowels.forEach(print)
    consonants.forEach(print)
}
\$\endgroup\$
4
\$\begingroup\$

The shorter method can be achieved using Regex and it is also the fastest according to JSBEN.CH:

var str = "The quick brown fox jumps over a lazy dog";
var vowels = str.match(/[aeiou]/gi);
var consonants = str.match(/[^aeiou$]/gi);
    vowels.concat([''],consonants).forEach(function(k){
    	console.log(k);
    });

According to comments from others, the code can be further improved as follow:

const str = "The quick brown fox jumps over a lazy dog"; 
const vowels = str.match(/[aeiou]/gi); 
const consonants = str.match(/[^aeiou]/gi);   
vowels.concat([''],consonants).forEach(k => { console.log(k); } );
\$\endgroup\$
  • 1
    \$\begingroup\$ There's no reason to put the $ into the character class of the regular expression. The indentation of the last 3 lines is a bit off, but apart from this: nice solution. \$\endgroup\$ – Roland Illig Mar 25 '19 at 20:38
  • \$\begingroup\$ You could also use the const keyword for the variable declarations. And I think the callback function in the forEach method would be even a bit nicer like this forEach( consonant => { console.log(consonant)}) \$\endgroup\$ – Dennis Fink Mar 25 '19 at 21:02

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.