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\$
  • \$\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 '14 at 16:33
  • \$\begingroup\$ Where i found this idea it said what ai did \$\endgroup\$ – Abandoned Account Oct 11 '14 at 16:35
  • \$\begingroup\$ *what I did just now. \$\endgroup\$ – Abandoned Account Oct 11 '14 at 16:35
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\$
  • \$\begingroup\$ wow.... thats only a few lines. I'm such a noob. -.- \$\endgroup\$ – Abandoned Account Oct 11 '14 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\$ – Ben Aaronson Oct 11 '14 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\$
  • \$\begingroup\$ I think that you copied the code wrong. For me it works fine, eg code -> ode-cay \$\endgroup\$ – tim Oct 11 '14 at 13:53
  • \$\begingroup\$ @tim Is that so? I ran it on ideone: ideone.com/0XGblY \$\endgroup\$ – Unihedron Oct 11 '14 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 '14 at 13:55
  • \$\begingroup\$ It works fine for me. I am using Java 8. \$\endgroup\$ – Abandoned Account Oct 11 '14 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\$
  • 2
    \$\begingroup\$ But here it is code review. \$\endgroup\$ – Abandoned Account Oct 12 '14 at 3:02

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.