8
\$\begingroup\$

I've done my second application in Java now. I want to know if:

  • This code is efficient
  • This code can be written in a shorter and faster way
  • Any flaws
  • Any misuse or better use of concepts

Main Class:

package Excercises;

public class Main {
    public static void main(String[] args) {
        PigLatin pigLatin = new PigLatin();

        pigLatin.initializeText();
        pigLatin.getWord();
        pigLatin.convertWordToStringArray();
        pigLatin.convertArrayToList();
        pigLatin.moveFirstLetterToLast();
        pigLatin.mergeList();

        System.out.println(pigLatin.returnNewWord());
    }
}

PigLatin class:

package Excercises;

import java.util.ArrayList;
import java.util.List;
import java.util.Scanner;

public class PigLatin {
    private String word;
    private String newWord;
    private String[] Array;
    private List<String> list = new ArrayList<String>();

    public void initializeText() {
        System.out.print("Welcome to the Pig Latin Game! Please enter a word: ");
    }

    public void getWord() {
        Scanner scan = new Scanner(System.in);
        this.word = scan.next();
    }

    public void convertWordToStringArray() {
        this.Array = word.split("");
    }

    public void convertArrayToList() {
        for(String s : this.Array) {
            this.list.add(s);
        }
    }

    public void moveFirstLetterToLast() {
        this.list.add("-");
        this.list.add(this.Array[0]);
        this.list.remove(0);
        this.list.add("ay");
    }

    public void mergeList() {
        for(int i = 0; i < list.size(); i++) {
            this.newWord = this.newWord + list.get(i);
        }
        this.newWord = this.newWord.replaceAll("null", "");
    }

    public String returnNewWord() {
        return this.newWord;
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ Not really related to code review, but the rules for Pig Latin are not that simple. You are supposed to move all of the beginning letters up to the first vowel. If the word begins with a vowel, you just add "way" to the end. \$\endgroup\$
    – gla3dr
    Oct 11, 2014 at 16:33
  • \$\begingroup\$ Where i found this idea it said what ai did \$\endgroup\$ Oct 11, 2014 at 16:35
  • \$\begingroup\$ *what I did just now. \$\endgroup\$ Oct 11, 2014 at 16:35

4 Answers 4

11
\$\begingroup\$

Input and general structure

Right now, the only possible way to use your class is via the command line. This makes testing really hard. I would change your getWord function to a simple setWord(String word) setter, and read the word in outside the class (somewhere in Main).

And in general, your PigLatin class should only transform words to piglatin, not also print and read. I would move initializeText to Main as well.

And your public interface for PigLatin is quite hard to manage. I would make all those methods private, and then either create a toPigLatin method, or just call them in returnNewWord.

I also think that you could merge some of the methods. At least convertWordToStringArray and convertArrayToList could well be in one method (wordToCharList or something).

Your naming is sometimes not very precise. For example, moveFirstLetterToLast also adds ay, which is unexpected.

This code is efficient

Probably not. You are iterating over the whole string twice, which is not necessary.

This code can be written in a shorter and faster way

Sure, there are lots of ways. For example:

   String s;
   char firstChar = s.charAt(0);
   String piglatin = s.substring(1) + "-" + firstChar + "ay";

It's probably not the fastest way, but it should be faster than your approach.

\$\endgroup\$
2
  • \$\begingroup\$ wow.... thats only a few lines. I'm such a noob. -.- \$\endgroup\$ Oct 11, 2014 at 14:40
  • 5
    \$\begingroup\$ +1 This covers a lot of important points. I think it's worth repeating for emphasis that a class should not just expose every stage of its process as a public method and have the caller call each of them in turn. Because now two classes need to know how the process works: the actual class, and its caller. \$\endgroup\$ Oct 11, 2014 at 15:01
10
\$\begingroup\$

It's much cleaner to iterate through a List using a foreach loop:

for (String s : list) {
    this.newWord = this.newWord + s;
}

And you can use the += operator to append onto the end of a String:

for (String s : list) {
    this.newWord += s;
}

But it's more efficient to use the StringBuilder class when performing multiple string concatenations.

StringBuilder sb = new StringBuilder();
for (String s : list) {
    sb.append(s);
}
this.newWord = sb.toString();
\$\endgroup\$
6
\$\begingroup\$

Any flaws?

Yep, you don't seem to generate a correct output:

STDIN
>>> Generic
STDOUT
>>> Welcome to the Pig Latin Game! Please enter a word:
... Generic-ay

I want to know if this code can be written in a shorter and faster way

I suppose you can do this instead, here you go:

package Excercises;

public class Main {
    public static void main(String[] args) {
        System.out.print("Welcome to the Pig Latin Game! Please enter a sentence: ");
        System.out.println(java.util.Scanner(System.in).nextLine().replaceAll("(\\w)(\\w+)", "$2-$1ay"));
    }
}

With regex replacement, you can handle the entire line without constructing a List, an array, and constantly creating new String objects. You can replace the entire line with the code too:

STDIN
>>> Generic sentences are so funny.
STDOUT
>>> Welcome to the Pig Latin Game! Please enter a word:
... eneric-Gay entences-say re-aay o-say unny-fay.
\$\endgroup\$
4
  • \$\begingroup\$ I think that you copied the code wrong. For me it works fine, eg code -> ode-cay \$\endgroup\$
    – tim
    Oct 11, 2014 at 13:53
  • \$\begingroup\$ @tim Is that so? I ran it on ideone: ideone.com/0XGblY \$\endgroup\$
    – Unihedron
    Oct 11, 2014 at 13:54
  • \$\begingroup\$ I ran it locally and it works fine. I'm using Java 8 (but I don't think that that would make a difference). \$\endgroup\$
    – tim
    Oct 11, 2014 at 13:55
  • \$\begingroup\$ It works fine for me. I am using Java 8. \$\endgroup\$ Oct 11, 2014 at 16:09
1
\$\begingroup\$

I don't think anyone here's code handles vowels at all, I believe a Pig Latin creator should actually move the first consonants to the end (which may be nothing) and add -ay

Pseudocode (operating on a single word):

list-of-consonants = []
index = 0
while(current-letter.is-consonant) do
    list-of-consonants.add(current-letter)
    index++
end
output-string =
    original-word.substring-starting-at(index) concat-with
    "-" concat-with
    list-of-consonants concat-with
    "ay"

On a more practical level, checking if something is a consonant will probably be done with a simple array check. I think Java's substring method has an overload that just takes an int, which will do a substring starting at an index and ending at the end of the string.

This means that "are" gets changed to "are-ay" instead of "re-aay" and "Philadelphia" gets changed to "iladelphia-Phay" (maybe you should handle capitals somehow but this probably doesn't matter too much)

\$\endgroup\$
1
  • 2
    \$\begingroup\$ But here it is code review. \$\endgroup\$ Oct 12, 2014 at 3:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.