6
\$\begingroup\$

Here is my code for hacker rank question. Can somebody suggest improvement?

Question:

We consider a word, to be beautiful if the following two conditions are satisfied:

  • No two consecutive characters are the same.
  • No two consecutive characters are in the following vowel set: a, e, i, o, u, y. Note that we consider y to be a vowel in this challenge.

Solution:

import java.io.*;
import java.util.*;
import java.text.*;
import java.math.*;
import java.util.regex.*;

public class Solution {

   public static void main(String[] args) {
        Scanner in = new Scanner(System.in);
        String w = in.next();

        int result = isBeautiful(w);

        if (result == 1) {
            System.out.print("Yes");
        } else
            System.out.print("No");

    }

    static int isBeautiful(String w) {
        for (int i = 0; i < w.length() - 1; ++i) {
            if (w.charAt(i) == w.charAt(i + 1)) {
                return 0;
            }
            if ( ( isVowel(w.charAt(i)) == isVowel(w.charAt(i + 1)) && (isVowel(w.charAt(i)) == 1))) {
                return 0;
            }
        }
        return 1;
    }

    static int isVowel(char c) {
        if ( (c == 'a') || (c == 'e') || (c == 'i') || (c == 'o') || (c == 'u') || (c == 'y') ) {
            return 1;
        }
        return 0;
    }

}
\$\endgroup\$
1
  • \$\begingroup\$ this might solve the problem but it isn't very OOP, and looks difficult to interpret and not very amenable to change. Looks like an interesting problem though. i might have a go shortly \$\endgroup\$ – BKSpurgeon Apr 14 '17 at 5:04
11
\$\begingroup\$

First of all, you did well splitting it up into those methods.

The major issue I have with your code is that the natural translation from the English answers yes/no to Java is to use a boolean. Where true means yes and false means no.

So your isVowel method should be written like this:

static boolean isVowel(char c) {
    if ( (c == 'a') || (c == 'e') || (c == 'i') 
        || (c == 'o') || (c == 'u') || (c == 'y') ) {
        return true;
    }
    return false;
}

Now the next thing you need to know is that a check like c == 'a' is already a boolean, and that the || operator combines 2 booleans into a new boolean. Since we want to return a boolean, instead of writing

if (<boolean check>) {
    return true; 
} else {
    return false;
}

we should just write the following:

return <boolean check>;

which in your case would be:

static boolean isVowel(char c) {
    return (c == 'a') || (c == 'e') || (c == 'i') 
           || (c == 'o') || (c == 'u') || (c == 'y');
}

A final issue with this method is that it's actually package-visible. There is no reason to use this method outside of this class, so it should be private.

private static boolean isVowel(char c) {

The same reasoning applies to the isBeautiful method. So let us also make this private and return a boolean.

private static boolean isBeautiful(String w) {
    for (int i = 0; i < w.length() - 1; ++i) {
        if (w.charAt(i) == w.charAt(i + 1)) {
            return false;
        }
        if (isVowel(w.charAt(i)) && isVowel(w.charAt(i + 1))) {
            return false;
        }
    }
    return true;
}

Notice how I also simplified the check for 2 consecutive vowels so that it now actually reads: if this character is a vowel and the next character is a vowel.


Now the last thing left to do is use this new isBeautiful method. This means we will change this code:

int result = isBeautiful(w);

if (result == 1) {
    System.out.print("Yes");
} else
    System.out.print("No");

to this:

if (isBeautiful(w) {
    System.out.print("Yes");
} else {
    System.out.print("No");
}

Notice I also added the {} after the else. It's best practice to add curly braces after each if/else/for/while.


One last tip is that you can also rewrite the isVowel method like so:

private static final String VOWELS = "aeiouy";

private static boolean isVowel(char c){
    return VOWELS.contains(c);
}

But this doesn't mean that it's bad to check each character separately - it's just an alternative way.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ nice answer - I'll go ahead and cancel mine ;-) \$\endgroup\$ – rolfl Apr 11 '17 at 17:48
2
\$\begingroup\$

@Imus already posted a very nice answer, but was kind enough to leave me one thing.

You should only import the packages you really need (in this case java.util.*).

\$\endgroup\$
0
\$\begingroup\$

Do you need the second if statement at all?

if (w.charAt(i) == w.charAt(i + 1)) { return 0; } 
if ( ( isVowel(w.charAt(i)) == isVowel(w.charAt(i + 1)) && (isVowel(w.charAt(i)) == 1))) { return 0; }

The first if statement catches all instances of two consecutive characters including vowels

\$\endgroup\$
2
  • 1
    \$\begingroup\$ But it does not catch every two consecutive vowels – for example it catches ee or oo but not ae or yi. \$\endgroup\$ – CiaPan Apr 14 '17 at 11:21
  • \$\begingroup\$ You are right, i miss read the question 😀 \$\endgroup\$ – abcdef Apr 14 '17 at 16:17
0
\$\begingroup\$

You do not need to call isVowel() on w.charAt(i) if you already tested the character with isVowel(w.charAt(i+1)) in the previous iteration. We may store the 'previous character was a vowel' in a separate variable, so we can use it in the next iteration without calling a function:

static int isBeautiful(String w) {
    boolean prevVowel = isVowel(w.charAt(0));
    boolean thisVowel = false;
    for (int i = 0; i < w.length() - 1; ++i) {
        if (w.charAt(i) == w.charAt(i + 1)) {
            return 0;
        }
        thisVowel = isVowel(w.charAt(i + 1));
        if (prevWovel && thisVowel) {
            return 0;
        }
        prevWovel = thisVowel;
    }

    return 1;
}

static boolean isVowel(char c) {
    return (c == 'a') || (c == 'e') || (c == 'i') 
           || (c == 'o') || (c == 'u') || (c == 'y');
}
\$\endgroup\$

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.