Separate combined words in a sentence and convert them into Pig Latin

Question

My program is complete and working, but I would like second opinions on it.

Here are the instructions for the assignment:

1. The program will accept a string as input in which all of the words are run together, but the first character of each word is uppercase. Convert the string to a string in which the words are separated by spaces and only the first word starts with an uppercase letter. For example, the string "StopAndSmellTheRoses" would be converted to "Stop and smell the roses".

2. Then the program will convert each word in the result string of task 1 into "Pig Latin". In one version of Pig Latin, you convert a word by removing the first letter, placing that letter at the end of the word, and then appending "ay" to the word.

For example, for the result string "Stop and smell the roses" in task 1, the Pig Latin string should be "topSay ndaay mellsay hetay osesray".

Requirements:

• Your program should have 3 methods: a main method, a method for task 1, and a method for task2.
• The method for task 1 should return the result string of task1.
• The method for task 2 should display the Pig Latin string.

import java.util.Scanner; 
public class Assignment9
{
    public static void main(String[] args) 
    {   
    // Variables
    String sentence, revisedSentence, latin = " "; 

    // Create a Scanner object for keyboard input
    Scanner keyboard = new Scanner(System.in); 

    // Get the input string
    System.out.print("Enter sentence: ");
    sentence = keyboard.nextLine(); 

    //Close keyboard
    keyboard.close();

    // Task 1 
    revisedSentence = WordSeparator(sentence); 

    System.out.print("Revised Sentence: " +revisedSentence); 

    // Task 2
    PigLatin(revisedSentence, latin); 

    System.exit(0); 
}

private static String WordSeparator(String sentence) 
{
    // Variables 
    StringBuilder str = new StringBuilder(sentence); 
    int i = 1; 

    // While loop repeats until the end of the sentence
    while (i < str.length())
    {
        // Checks for upper case characters
        if(Character.isUpperCase(str.charAt(i)))
        {
            //Inserting space
            str.insert(i, ' '); 
            char ch = Character.toLowerCase(str.charAt(i+1)); 
            str.setCharAt((i+1), ch); 
        }
        i++; 
    }
    return str.toString();
}

private static void PigLatin(String revisedSentence, String latin) 
{   
    // Split sentence by spaces
    String[] tokens = revisedSentence.split(" ");

    // Store all the words into an array
    String[] arr = new String[tokens.length];       

    // Convert English sentence into Pig Latin 
    for (int i = 0; i < tokens.length; i++)
    {
        // Get string from array
        String str = tokens[i]; 

        // Get first letter from string
        String str1 = str.substring(0, 1); 

        // Get substring from string
        String str2 = str.substring(1, str.length()); 

        // Concatenate the two strings in a required format
        str2 = str2.concat(str1); 

        // Concatenate the result and "AY" 
        str2 = str2.concat("ay"); 

        // Store all of the strings into an array
        arr[i] = str2; 

        // Make a sentence with all the words 
        latin = latin.concat(str2 + " "); 
    }

    // Display pig latin verison
    System.out.println("\nPig Latin Version:" +latin ); 
    }

}

Answer 1

This is in addition to what @thecoder16 already said.

WordSeparator

Methods should be named using verbs instead of nouns. They usually take some action on the input, for example this method separates the words that were stuck together. So separateWords would be better.

Also, this part is not great:

    // Checks for upper case characters
    if(Character.isUpperCase(str.charAt(i)))
    {
        //Inserting space
        str.insert(i, ' '); 
        char ch = Character.toLowerCase(str.charAt(i+1)); 
        str.setCharAt((i+1), ch); 
    }

Objections:

1. The str.charAt(i) and str.charAt(i+1) refer to the same character. This is repetitive, and can be prone to errors, as you have to remember to use i+1 the second time because of the inserted space

2. The comments only state the obvious. Unnecessary comments are like noise, and a waste of time to read. Let the code speak for itself.

It would be better this way:

    char ch = str.charAt(i);
    if (Character.isUpperCase(ch)) {
        str.insert(i, ' ');
        str.setCharAt(i + 1, Character.toLowerCase(ch));
    }

PigLatin

The biggest issue here is the latin = latin.concat(...) call for two reasons:

1. String concatenation is inefficient. You should use a StringBuilder like you did in your other method.
2. Reusing input parameters is not a good practice, as it can be confusing and error prone. It would be better to name local variables differently.

Issues similar to WordSeparator:

• The method is poorly named. Perhaps toPigLatin would be better.
• The comments are unnecessary noise

Other smaller issues:

• Instead of printing output, it would be better to return the result
• The String[] arr is unused, so it would be better to remove it
• The counting for loop can be converted to an enhanced loop, for example: for (String str : tokens) { ... }
• The input parameter revisedSentence carries information that's unnecessary for the functioning of this method. Just sentence would be enough.

Answer 2

I just have two suggestions:

Function names in java should be camelCase, and should say what the function does, like toPigLatin and separateWord instead of WordSeparator and PigLatin.

Also, your pig latin function should return the string in Pig Latin and have main() print it out. I feel like that is better organization.

Otherwise, you have great, working, efficient code!

Answer 3

You are checking the capital letters twice in your loop, you should increment i by 1 when you add in a space. This will also mean you don't need to use i + 1 to de-capitalise the current letter

if(Character.isUpperCase(str.charAt(i)))
"andDad"
    ^== (i == 3)

str.insert(i, ' ');
"and Dad"
    ^=== (i == 3)

char ch = Character.toLowerCase(str.charAt(i+1)); 
str.setCharAt((i+1), ch);
"and dad"
     ^=== (i == 3) + 1
i++;
"and dad"
     ^=== (i == 4)

new while loop iteration
"and dad"
     ^=== (i == 4)

change the code in the if this

str.insert(i, ' '); 
i++;
char ch = Character.toLowerCase(str.charAt(i)); 
str.setCharAt(i, ch);

Answer 4

Just two pointers for your code:

1. You don't need to do an explicit System.exit(0) at the end of your main() method.
2. A school of thought says that "comments should explain 'why', not 'how'". This mindset encourages developers to write self-documenting code (such as using proper field names, and coding in a more direct way) instead of relying on comments, which may become outdated, and then ultimately misleading. Your choice of comments is unfortunately needless, as practically all the lines are pretty much self-explanatory. Hence, my suggestion here is to remove them and the extra newlines between each line of code. :)

Now, I'm not sure if you can use Java 7 (so that you can rely on try-with-resources for your Scanner object) or even Java 8 (so that you can think of your operations as more of a pipe-lined process). Assuming both are available at your disposal and you are also allowed to sprinkle a little of regex magic, then you may want to consider the following simplification.

public class PigLatin {

    public static void main(String[] args) {
        try (final Scanner keyboard = new Scanner(System.in)) {
            System.out.println("Enter sentence: ");
            Stream.of(keyboard.nextLine())
                    .map(PigLatin::sentenceParser)
                    .peek(System.out::println)
                    .forEach(PigLatin::toPigLatin);
        }
    }

    private static String parseSentence(final String input) {
        final String result = input.replaceAll("(\\p{Ll})(\\p{Lu})", "$1 $2");
        return result.substring(0, 1) + result.substring(1).toLowerCase();
    }

    private static void toPigLatin(final String input) {
        Stream.of(input.split(" ")).forEach(i -> 
            System.out.printf("%s%say ", 
                    i.substring(1), i.substring(0, 1)));
    }
}

1. try-with-resources handles the eventual closing of the Scanner object implicitly
2. From the user input, I 'map' it using the required transformation of your task 1
   • The regex is used here to insert the extra space before every uppercase character
   • Then, I keep the first character of the sentence and convert the rest to lower case
3. Before I start task 2, I 'peek' at the result of task 1 by printing it (System.out::println is a method reference, i.e. equivalent to writing something like System.out.println(<output of task 1>))
4. Finally, I start processing with task 2 'forEach' of the output of task 1 (which in this case is a single sentence)
   • I split it on the " " character, and instead of a slightly verbose String concatenation, I rely on formatting the input String given the requirements:
     • 'removing the first letter': i.substring(1)
     • 'placing that letter at the end of the word': i.substring(0, 1)
     • 'and then appending "ay" to the word': "%s%say"