3
\$\begingroup\$

For starters, I'm a student taking AP Computer Science this year, loving it so far. Lots of fun, especially problems like this. I think I'm going to make it a regular practice to post some of my exercises and projects here for people to review, so I can get some feedback from programmers other than my teacher on different ways to do things, ways to clean up my code, and good practice.

I'm not looking for answers here. I've completely written the code and am just looking for some feedback on a few questions listed below.

The prompt is as follows:

Write an interactive program that reads lines of input from the user and converts each line into Pig Latin. Terminate the program when the user types a blank line.

  • Words beginning with consonants have the consonant moved to the end of the word and "ay" appended
  • Words beginning with vowels simply have "ay" appended

Specific questions:

  • Did I do this in an acceptably efficient way? What other, possibly better, ways could I have accomplished this?
  • How could this be done without an ArrayList? Learning how to use ArrayLists is something I've done on my own time, not that we've learned in class, and I want to become more independent from them on problems like this in case my teacher doesn't want me to keep using them.
  • What variables do I need to keep as class variables? When I was declaring them I wasn't sure exactly how I would structure the program, or if I was going to organize it into separate methods or not.

import java.util.*;
public class PigLatin {
    static ArrayList<String> al = new ArrayList<String>();
    static Scanner sc = new Scanner(System.in);
    static String userString;
    static String latinString;
    static String temp;

    public static void main(String[] args) {
        while (true) {
            latinString = "";
            System.out.print("Enter a string to be converted into Pig Latin. To stop, enter a blank input: ");
            String userString = sc.nextLine();
            if (userString.isEmpty()) {
                break;
            }

            ArrayList<String> al = new ArrayList<String>(Arrays.asList(userString.split("\\s")));

            for(int i = 0; i <= al.size() - 1; i++) {
                temp = al.get(i);
                String tester = temp.toUpperCase();
                if (tester.charAt(0) == 'A' || tester.charAt(0) == 'E' || tester.charAt(0) == 'I' || tester.charAt(0) == 'O' ||
                        tester.charAt(0) == 'U' ) {
                    latinString = latinString + temp + "ay ";
                } else {
                    latinString = latinString + temp.substring(1) + temp.charAt(0) + "ay ";
                }

            }
            System.out.println(latinString);
        }   
        sc.close();
    }
}
\$\endgroup\$
4
\$\begingroup\$

Scoping

All the static fields of your class can be made into local variables. This gives them a smaller scope. Always give variables the smallest possible scope. When you do this you'll notice userString and al are already defined as a local variable. al should be declared as a List<String>, in fact you'll see that the code works fine with the List returned by Arrays.asList().

Naming

Use meaningful names instead of abbreviations for names : al, sc, even temp don't convey anything.

Algorithm

Testing whether the first letter of a String is a vowel shouldn't require you to uppercase the entire String. You shouldn't even be making a second String (tester). It's easier to make a String of all vowels (upper and lower case) and use indexOf() on that string with the character you want to test.

The while(true) loop is bogus too. I had to look for the exit condition. Make it explicit.

You remember to close the Scanner, but do it in a finally block, so a possible Exception doesn't bypass the closing. For extra points use a try-with-resources structure (see my refactored example).

Wait, remember I said the code would work fine with the result of Arrays.asList()? It will, but you don't even need a list to loop over an array. Just use an advanced for loop.

for (String temp : userString.split("\\s")) {
    ...
}

latinString should be a StringBuilder, as it's mostly used to build the resulting String.

Use helper methods to explain what you're doing or to extract repeated code.

Here's my refactored version :

public static void main(String[] args) {
    try (Scanner scanner = new Scanner(System.in)) { // try with resource will ensure the Scanner gets closed.
        String userString = getUserInput(scanner);
        while (!userString.isEmpty()) {
            System.out.println(buildLatinString(userString));
            userString = getUserInput(scanner);
        }
    }
}

private static String buildLatinString(String userString) {
    StringBuilder latinString = new StringBuilder();
    for (String word : userString.split("\\s")) {
        latinizeWord(latinString, word);
    }
    return latinString.toString();
}

private static void latinizeWord(StringBuilder latinString, String word) {
    if (startsWithVowel(word)) {
        latinString.append(word).append("ay ");
    } else {
        latinString.append(word.substring(1)).append(word.charAt(0)).append("ay ");
    }
}

private static String getUserInput(Scanner scanner) {
    System.out.print("Enter a string to be converted into Pig Latin. To stop, enter a blank input: ");
    return scanner.nextLine();
}

private static boolean startsWithVowel(String word) {
    return "AEIOUaeiou".indexOf(word.charAt(0)) != -1;
}
\$\endgroup\$
  • \$\begingroup\$ Thank you for the response! It's already helped me improve my current code quite a bit. I do have a quick question, though. We haven't learned about the enhanced for loops yet. From what I can gather, this is essentially creating a String for us to use within that iteration of the loop from userString.split("\\s"). Correct? \$\endgroup\$ – Wolverine1621 Nov 9 '15 at 1:44
  • \$\begingroup\$ An enhanced for loop is basically syntactic sugar for a for loop over an Iterable using its Iterator or over an array. The syntax allows you to specify the minimum to get a for loop. for (Type a : b) will assign to a each element of b of type Type in turn, until all of b's elements have been looped, or the loop code breaks out of the loop. \$\endgroup\$ – bowmore Nov 9 '15 at 5:40
  • \$\begingroup\$ Why exactly should I close a Scanner on System.in? I don't see the reason. I don't need to close System.in, and the reference to the Scanner is lost after the method. What if I wanted to read from STDIN in multiple places? \$\endgroup\$ – Justin Nov 9 '15 at 9:02
  • \$\begingroup\$ @Justin In this instance, I thought it more important to encourage the good practice of properly closing resources, rather than nitpick over whether the System.in should really be closed (ever). But it's a valid remark. \$\endgroup\$ – bowmore Nov 9 '15 at 13:18

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.