Forming words that start with consonants and vowels

Question

I am just starting to learn programming. I had already coded some functions and stuff in other languages, and whilst trying to complete this Python challenge Kevin and Stuart want to play the 'The Minion Game'.

Game Rules

Both players are given the same string. Both players have to make substrings using the letters of the string.

• Stuart has to make words starting with consonants.
• Kevin has to make words starting with vowels.
• The game ends when both players have made all possible substrings.

Scoring A player gets +1 point for each occurrence of the substring in the string.

For Example:

• String = BANANA
• Kevin's vowel beginning word = ANA here, ANA occurs twice in BANANA. Hence, Kevin will get 2 Points.

For better understanding, see this image.

I have not yet implemented the feature that multiple occurrences of substrings yield more points.

I switched over to Java.

I initially wanted to start learning Python, but the company I just started an apprenticeship in almost exclusively uses Java, so I figured I might as well learn that language.

I tried completing it by mostly using trial and error and some google, I did not watch a lot of guides so far, so figuring out what I need to code in what way took me quite a few hours. Now my code basically does what it is supposed to do, but I feel like it is written unnecessarily complicated and could be much shorter.

Since I have not really understood scope and accessibility of methods yet, I was not able to reuse code as I would have liked to and needed to rewrite a lot.

public static void main(String[] args)
{
    System.out.println("Please enter a word");
    Scanner consoleInput = new Scanner(System.in);
    String userInput = consoleInput.nextLine(); 



    System.out.println(GetVowels(userInput).size() + " words starting with wovels: " + GetVowels(userInput));
    System.out.println(GetConsonants(userInput).size() + " words starting with consonants: " + GetConsonants(userInput));
}

 static ArrayList<String> GetVowels(String userInput){ 
    char[] vowels = "aeiou".toCharArray(); 
    ArrayList<String> possibleWords = new ArrayList<String>(); 

    for(int i = 0; i < vowels.length; i++){ 
        int offset = 0; 
        while(userInput.indexOf(vowels[i], offset) >= 0){ 
            int wordStartingIndex = userInput.indexOf(vowels[i], offset); 
            for (int u = wordStartingIndex; u <= userInput.length(); u++)  
            {
                String maybeAWord = userInput.substring(wordStartingIndex, u); 
                if(maybeAWord.length() > 0 && possibleWords.indexOf(maybeAWord) < 0) 
                {
                    possibleWords.add(maybeAWord); 
                }
            }
            offset = userInput.indexOf(vowels[i], offset) +1; 
        }
    }
  return possibleWords;

}


static ArrayList<String> GetConsonants(String userInput){
    char[] vowels = "aeiou".toCharArray();
    ArrayList<String> possibleWords = new ArrayList<String>();
    ArrayList<Integer> vowelIndexes = new ArrayList<Integer>();
    ArrayList<Integer> consonantIndexes = new ArrayList<Integer>();


    for(int i = 0; i < vowels.length; i++) { 
        int offset = 0;
        while (userInput.indexOf(vowels[i], offset) >= 0)
        {
            vowelIndexes.add(userInput.indexOf(vowels[i], offset));
            offset = userInput.indexOf(vowels[i], offset) + 1;
        }

    }

    for(int i = 0; i < userInput.length(); i++)
    {
        if(vowelIndexes.indexOf(i) < 0)
        {
            int wordStartingIndex = i;
            int offset = wordStartingIndex;

            while(offset <= userInput.length())
            {
                String maybeAWord = userInput.substring(wordStartingIndex, offset);
                if(maybeAWord.length() > 0 && possibleWords.indexOf(maybeAWord) < 0)
                {
                   possibleWords.add(maybeAWord);
                }
                offset = offset + 1;
            }

        }

    }
    return possibleWords;
}

I absolutely do not expect anyone to rewrite part of my code in a more compact or efficient way, but I would be very grateful for some tips or hints on what I could write differently, how I might be able to reuse some of the code that I had to write multiple times, or just things that I should avoid in the future.

Answer 1

Just a couple of words before the code review starts: I only found small issues that are not super crucial but make your code more readable or even a bit faster. My review is kinda long not because your code is bad but because I tried to elaborately explain why I would make some changes.

General Code Style

• Methods, in general, should always use lowerCamelCase for consistency and readability. So in your case, the two methods should be named getVowels and getConsonants.
• You should stay consistent with the placement of the braces. Most coding conventions recommend to have whitespace between keywords and braces and to keep the curly braces in the same line instead of having them in a new line, e.g.: for (int i = 0; i < 10; i++) {
• Using access modifiers is considered good practice and using the default access is not neccessary in your code which means you can use the modifier private.

Programming to Interface

• It's generally a good practice to program to an interface if possible, which in your case means that you should use the interface List wherever you can. The main reason for this design principle is that your code doesn't become dependent on a specific implementation. Here is a link to a post that explains this better than I ever could.
  • Concretely this means that both your methods should return List<String> instead of ArrayList<String> and when initializing a list you should use the following pattern:

List<T> identifier = new ArrayList<>();

main

• You should probably modify the input to be in all uppercase letters just in case the user doesn't use all caps. This makes the game more fool-proof.

String userInput = consoleInput.nextLine().toUpperCase();

• Safe the Lists getVowels(userInput) and getConsonants(userInput) as they are used multiple times and a call to one of these methods is pretty cost-intensive since they have three nested for-loops (we want those to be called as few times as possible).

The full method would look like this:

public static void main(String[] args) {
    System.out.println("Please enter a word");
    Scanner consoleInput = new Scanner(System.in);
    String userInput = consoleInput.nextLine().toUpperCase();

    List<String> vowels = getVowels(userInput);
    List<String> consonants = getConsonants(userInput);
    System.out.println(vowels.size() + " words starting with vowels: " + vowels);
    System.out.println(consonants.size() + " words starting with consonants: " + consonants);
}

getVowels

• You can use a for-each loop instead of the currently existing for-loop with minimal effort. Usually, you should always use for-each over a classic for-loop because it is shorter and a clarifies the intention of the loop.

for (char vowel : vowels) {

• Declare the variable wordStartingIndex outside of the loops and initialize it inside of the condition. This reduces the amount of memory usage a bit:

int wordStartingIndex;
// ...
while ((wordStartingIndex = userInput.indexOf(vowels[i], offset)) >= 0) {

• The variable wordStartingIndex can also be reused when assigning a new value to the variable offset:

offset = wordStartingIndex + 1;

• Use the method contains instead of indexOf when checking if a word is already in the list of possible words. This effectively does the same but makes it clearer to someone who is reading the code what your goal is with that line (or even you if you take a look at it a few months later):

!possibleWords.contains(maybeAWord)

Applying the above recommendations the method would look like this:

private static List<String> getVowels(String userInput) {
    char[] vowels = "aeiou".toCharArray();
    List<String> possibleWords = new ArrayList<>();

    int wordStartingIndex;
    for (char vowel : vowels) {
        int offset = 0;
        while ((wordStartingIndex = userInput.indexOf(vowel, offset)) >= 0) {
            for (int u = wordStartingIndex; u <= userInput.length(); u++) {
                String maybeAWord = userInput.substring(wordStartingIndex, u);
                if (maybeAWord.length() > 0 && !possibleWords.contains(maybeAWord)) {
                    possibleWords.add(maybeAWord);
                }
            }
            offset = wordStartingIndex + 1;
        }
    }
    return possibleWords;
}

getConsonants

• The list consonantIndexes is never used so it can safely be deleted at the current state of the project. If you plan on using it later leave it in the code but I removed it in the final version.
• Again you should make use of the for-each loop and use a variable to store the value of userInput.indexOf(vowel, offset) since it is used three times in a row without it changing.
• The variable wordStartingIndex is completely redundant and thus can safely be deleted. Instead you should use the iterator-variable i.
• Also again, you should use the contains method instead of checking if the index is less than 0.

Applying the above recommendation the method would look like this:

private static List<String> getConsonants(String userInput) {
    char[] vowels = "aeiou".toCharArray();
    List<String> possibleWords = new ArrayList<>();
    List<Integer> vowelIndexes = new ArrayList<>();

    int vowelIndex;
    for (char vowel : vowels) {
        int offset = 0;
        while ((vowelIndex = userInput.indexOf(vowel, offset)) >= 0) {
            vowelIndexes.add(vowelIndex);
            offset = vowelIndex + 1;
        }
    }

    for (int i = 0; i < userInput.length(); i++) {
        if (vowelIndexes.indexOf(i) < 0) {
            int offset = i;
            while (offset <= userInput.length()) {
                String maybeAWord = userInput.substring(i, offset);
                if (maybeAWord.length() > 0 && !possibleWords.contains(maybeAWord)) {
                    possibleWords.add(maybeAWord);
                }
                offset = offset + 1;
            }
        }
    }
    return possibleWords;
}