Can anyone help me with this simple code?

Question

function isVowel(char){

    if(typeof char == 'string' && char.length > 1){
        console.log('not a char');
        return 'not a char';
    } else {
        if (char.toLowerCase() === ('a'||'e'||'i'||'o'||'u')){
            console.log(char, true);
            return true;
        } else {
            console.log(char, false);
            return false;
        }
    }
}

document.writeln(isVowel('a'));
document.writeln(isVowel('e'));
document.writeln(isVowel('l'));

the result is: true, false, false;

it should be: true, true, false;

Can anyone help me why this is happening?

I'm just barely learning JavaScript...

Also, is there any way to refactor this code? I don't want to be repeating myself for every new condition..

I think the downvote is due to the question title... "Can anyone help me?" does not describe your problem... you see? — J. Bruni, Feb 22 '12 at 02:22
"Function to determine if char is a vowel is not working" may be not the best title, but I think it is already a little better. — J. Bruni, Feb 22 '12 at 02:26

score 4 · Answer 1 · answered Feb 22 '12 at 02:09

4

You need to separate ||s like this:

char.toLowerCase() === 'a'
|| char.toLowerCase() === 'e'
|| char.toLowerCase() === 'i'
|| char.toLowerCase() === 'o'
|| char.toLowerCase() === 'u'

instead of like this:

char.toLowerCase() === ('a'||'e'||'i'||'o'||'u')

Here's a jsFiddle of the above:

http://jsfiddle.net/sXbBh/

answered Feb 22 '12 at 02:09

icyrock.com

27,952
4
66
85

thanks.. just wondering.. what would be the best way of doing this instead of repeating myself over and over again? – Jan Carlo Viray Feb 22 '12 at 02:12
1

[Set](http://en.wikipedia.org/wiki/Set_(computer_science)) is a structure suited for this. JS doesn't have them, but you can simulate with objects (i.e. `{}`) easily, e.g.: http://stackoverflow.com/questions/7958292/mimicking-sets-in-javascript. For small number of these, looping over array should be just fine - i.e. make an array `var vowels = ['a', 'e', 'i', 'o', 'u']` and then use `for` to go through each of the values and check. Or don't reinvent and just use what's already been done: http://stackoverflow.com/questions/5488028/best-way-of-checking-for-vowels-in-javascript – icyrock.com Feb 22 '12 at 02:15

J. Bruni · Accepted Answer · 2012-02-22T02:20:40.830

('a'||'e'||'i'||'o'||'u') is equal to "a"

To confirm the above, just try it using the console, or:

console.log(('a'||'e'||'i'||'o'||'u'));

My suggestion:

' aeiou'.indexOf(char) > 0

Complete version:

function isVowel(char){

    if(typeof char == 'string' && char.length > 1){
        console.log('not a char');
        return 'not a char';
    } else {
        if (' aeiou'.indexOf(char) > 0){
            console.log(char, true);
            return true;
        } else {
            console.log(char, false);
            return false;
        }
    }
}

document.writeln(isVowel('a'));
document.writeln(isVowel('e'));
document.writeln(isVowel('l'));

Refactored version:

function isVowel(char)
{
    return ((typeof char == 'string') && (char.length == 1) && ('aeiou'.indexOf(char.toLowerCase()) != -1));
}

@paislee: I have not swiped your idea, if you mean the "indexOf" usage; only when I was about to add the "refactored version", I improved the code by using the `-1` info from your comment — J. Bruni, Feb 22 '12 at 05:35

score 3 · Answer 3 · answered Feb 22 '12 at 02:13

3

    if (/[aeiou]/.test(char.toLowerCase())){
      // ...
    } else {
      // ...
    }

answered Feb 22 '12 at 02:13

Lior Cohen

8,985
2
29
27

be warned of `regex` solutions is almost always slower than `indexOf` algorithm. This can be a issue inside critical loops. – Vitim.us Feb 22 '12 at 16:59

calebds · Answer 4 · 2012-02-22T05:49:13.763

2

The problem is here

if (char.toLowerCase() === ('a'||'e'||'i'||'o'||'u'))

Equality operators don't "distribute", you'll have to test the input against each possibility independently. A concise solution might be

if ("aeiou".indexOf(char.toLowerCase()) + 1) {
    console.log(char, true);
    return true;
}

Array.indexOf()^MDN

edited Feb 22 '12 at 05:49

answered Feb 22 '12 at 02:09

calebds

25,670
9
46
74

1

`indexOf` finds the location of the search string. if it can't be found it returns `-1`. It's doing all the work for you == simpler code. – calebds Feb 22 '12 at 02:15
@paislee: `"aeiou".indexOf("a")` will return `0` (zero) and evaluate to `false` - your code above will give wrong results – J. Bruni Feb 22 '12 at 05:36

Can anyone help me with this simple code?

4 Answers4