7
\$\begingroup\$

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 ); 
    }

}
\$\endgroup\$
8
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ Hi, janos. Thank you for your kind input. In your Other smaller issues comments, you said that the variable sentence is enough, but when I replaced revisedSentence with sentence in the method toPigLatin, I ran the program and got incorrect input. So does that mean I should keep the revisedSentence in the method? \$\endgroup\$ – Duck Apr 21 '15 at 22:14
  • \$\begingroup\$ +1 for the obvious comments. Better use descriptive names for program elements. About the string concatenation. I would leave that to the compiler to decide, as according to the specs (docs.oracle.com/javase/specs/jls/se8/html/…) it may optimize the + operator. It used to be better to do that in code, but that's no longer necessary. \$\endgroup\$ – Kwebble Apr 21 '15 at 22:46
  • \$\begingroup\$ Thanks @Kwebble, I didn't know that feature of Java 8 \$\endgroup\$ – Stop ongoing harm to Monica Apr 22 '15 at 6:25
5
\$\begingroup\$

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!

\$\endgroup\$
3
\$\begingroup\$

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);
\$\endgroup\$
  • \$\begingroup\$ Do I place this in my if statement in the while loop? If I do that, I will have to declare the char ch in the variables comment section, right? \$\endgroup\$ – Duck Apr 23 '15 at 3:30
  • \$\begingroup\$ Yes, in its current spot in the if statement, you are making the string longer by 1 each time you add a space. This will put all the character indexs after and including the currently being read one up by one. You add one to i to account for that, effectively leaving your i referring to the same letter it was before adding the space the normal i++ still happens moving onto the next character \$\endgroup\$ – Gilsham Apr 23 '15 at 4:11
  • \$\begingroup\$ I implement it and the code won't complete. It will prompt me to enter a sentence and then stop. \$\endgroup\$ – Duck Apr 23 '15 at 4:59
  • \$\begingroup\$ I'm not sure how it could do that, all it is is adding i++ after the str.insert and removing the + 1 on the is below it \$\endgroup\$ – Gilsham Apr 27 '15 at 20:27
2
\$\begingroup\$

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"
\$\endgroup\$
  • \$\begingroup\$ i think your code is too complicated and hard too understand for the OP. Personally, even I find it a little hard to follow. \$\endgroup\$ – Quintec Apr 30 '15 at 0:10
  • \$\begingroup\$ @thecoder16 is it the stream part, the regex, or the String formatting in the last method (or 'step')? \$\endgroup\$ – h.j.k. Apr 30 '15 at 0:25
  • \$\begingroup\$ The regex and the peek method—I have never seen it before. Also, why a double colon? \$\endgroup\$ – Quintec Apr 30 '15 at 0:27
  • \$\begingroup\$ peek() is a method that allows an intermediate operation to be performed on each stream element, and in my case I am printing it out. As explained in my answer, :: is a method reference. Just some new features of Java 8. :) \$\endgroup\$ – h.j.k. Apr 30 '15 at 0:31
  • \$\begingroup\$ Hmm…interesting! What about the regex? \$\endgroup\$ – Quintec Apr 30 '15 at 0:33

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.