Pig Latin Anslatortray

Question

I've created a program that translates any word into the form of igpay atinlay or pig Latin!

I got the project idea from here and you can read the rules here.

My program outputs the same translation as in the rules section in the link above, except for the word "always". It isn't really a big deal as the program translates almost any word correctly.

package pigLatin;

import java.util.Scanner;

public class PigLatin
{
 String str;
 Scanner scan = new Scanner(System.in);
 char firstVow;
 char secondVow;
 int i;

 public void init()

 {
  str = scan.next();
  i = 0; //beginning of a string

  if (str.length() <= 3 && (str.charAt(i) == 'a' || str.charAt(i) == 'e' || str.charAt(i) == 'i' || str.charAt(i) == 'o' || str.charAt(i) == 'u'))
  {
   System.out.println(str + "ay");
   scan.close();
  }


  else if (str.length() > 3 && (str.charAt(i) == 'a' || str.charAt(i) == 'e' || str.charAt(i) == 'i' || str.charAt(i) == 'o' || str.charAt(i) == 'u')) {
   firstVow = str.charAt(i);
   i = 1;

   while (str.charAt(i) != 'a' || str.charAt(i) != 'e' || str.charAt(i) != 'i' || str.charAt(i) != 'o' || str.charAt(i) != 'u')

   {
    i++;
    if (i == str.length() - 1 && (str.charAt(i) != 'a' || str.charAt(i) != 'e' || str.charAt(i) != 'i' || str.charAt(i) != 'o' || str.charAt(i) != 'u')) {
     System.out.println(str + "ay");
     break;

    } else if (str.charAt(i) == 'a' || str.charAt(i) == 'e' || str.charAt(i) == 'i' || str.charAt(i) == 'o' || str.charAt(i) == 'u')

    {
     secondVow = str.charAt(i);

     //issues arise if the first vowel is equal to the second one
     if (firstVow != secondVow) {

      System.out.print(str.substring(str.indexOf(secondVow), str.length()));
      System.out.print(str.substring(str.indexOf(firstVow), str.indexOf(secondVow)));
      System.out.println("ay");
      break;

     } else if (firstVow == secondVow) {
      System.out.print(str.substring(2));
      System.out.print(firstVow);
      System.out.print(str.charAt(1));
      System.out.print("ay");
      break;
     }
     scan.close();
    }
   }

   //checks for words that begin with consonants
  } else {

   i = 0;
   while (str.charAt(i) != 'a' || str.charAt(i) != 'e' || str.charAt(i) != 'i' || str.charAt(i) != 'o' || str.charAt(i) != 'u')

   {
    i++;
    if (str.charAt(i) == 'a' || str.charAt(i) == 'e' || str.charAt(i) == 'i' || str.charAt(i) == 'o' || str.charAt(i) == 'u')

    {
     firstVow = str.charAt(i);
     System.out.print(str.substring(str.indexOf(firstVow), str.length()) + str.substring(0, str.indexOf(firstVow)) + "ay");
     break;
    }
   }
  }
  scan.close();

 }

 public static void main(String[] args) {
  new PigLatin().init();
 }

}

Answer 1

I also highly recommend Eric Stein's answer below for some more tips on clean Java style.

Vowels ad infinitum...

str.length() <= 3 && (str.charAt(i) == 'a' || str.charAt(i) == 'e' || str.charAt(i) == 'i' || str.charAt(i) == 'o' || str.charAt(i) == 'u')

could be simpler and clearer:

public static final String VOWELS = "aeiou";

// ...

str.length() <= 3 && VOWELS.indexOf(str.charAt(i)) != -1

Then you have a repeated check for vowels again in the else if clause of the conditional that originally check for vowels. To avoid repeated checks, just wrap the entire block in an if (VOWELS.indexOf(str.charAt(i)) != -1)

This is generalizeable: any time there are two parts of the same conditional that do the same check, it's probably better to wrap the entire thing in that one check.

And every other time you encounter (str.charAt(i) == 'a' || str.charAt(i) == 'e' || str.charAt(i) == 'i' || str.charAt(i) == 'o' || str.charAt(i) == 'u'), it should be replaced by VOWELS.indexOf(str.charAt(i)) != -1 But make sure it's exactly identical. (Note that there are more efficient ways to do this, but it gets complicated quickly, and my familiarity with Java is limited.)

Loopy code smells

// ...

i = 1;
while (str.charAt(i) != 'a' || str.charAt(i) != 'e' || str.charAt(i) != 'i' || str.charAt(i) != 'o' || str.charAt(i) != 'u') {
    i++;

    //...

Seeing this gives me pause: having an iteration variable in a while loop is bad style. There's a reason the for loop exists!

There's another, even more glaring problem though. This loop condition will never fail on its own.

One can figure that out simply from the first two conditions (str.charAt(i) != 'a' || str.charAt(i) != 'e'): even if the character is an 'a', it cannot also be an 'e'; if it's not an 'a', then it's true on the first condition. Replacing it with while (true) would do the same thing.

Unreachable statement

Examine this:

if (firstVow != secondVow) {
    //...
    break;
} else if (firstVow == secondVow) {
    //...
    break;
}
scan.close();

One may note that because firstVow either equals or does not equal SecondVow at any given point, this last statement will never be reached and the scanner will never be closed. Bummer. (It actually will later, and this statement can be safely ignored and deleted.) This is a good reason to separate the scanning from the translating method; it makes it easier to change both (e.g. add validation, etc.), and it prevent resource leaks.

More transparent code

Hopefully more transparent code can help you identify the source of the bugs.

package pigLatin;

import java.util.Scanner;

public class PigLatin
{
    public static final String VOWELS = "aeiou";

    public static String translate(String str)
    {
        String pigLatinPrefix = "";
        char firstVowel;
        if (VOWELS.indexOf(str.charAt(0)) != -1) {
            if (str.length() > 3) {
                firstVowel = str.charAt(0);
                for (int i = 1; i < str.length(); ++i) {
                    if (VOWELS.indexOf(str.charAt(i)) != -1) {
                        char secondVowel = str.charAt(i);
                        //issues arise if the first vowel is equal to the second one
                        if (firstVowel != secondVowel) {
                            pigLatinPrefix = str.substring(str.indexOf(secondVowel), str.length()) + str.substring(str.indexOf(firstVowel), str.indexOf(secondVowel));
                            break;
                        } else {
                            pigLatinPrefix = str.substring(2) + firstVowel +  str.charAt(1);
                            break;
                        }
                    }
                }
            }
        }
        //checks for words that begin with consonants
        else {
            for (int i = 1; i < str.length(); ++i) {
                if (VOWELS.indexOf(str.charAt(i)) != -1) {
                    firstVowel = str.charAt(i);
                    pigLatinPrefix = str.substring(str.indexOf(firstVowel), str.length()) + str.substring(0, str.indexOf(firstVowel));
                    break;
                }
            }
        }

        if (pigLatinPrefix.equals("")) {
            pigLatinPrefix = str;
        }
        return pigLatinPrefix + "ay";
    }

    public static void main(String[] args) {
        Scanner scan = new Scanner(System.in);
        String str = scan.next();
        scan.close();
        System.out.println(translate(str));
    }
}

---

Stylistic concerns

To begin with, please do not indent your code only 1 space! Studies have shown that code is most comprehensible when indented 2-4 spaces. My personal preference, along with many other programmers, is 4 spaces.¹

A consistent bracketing style wouldn't hurt either.

---

^{1: Naturally, the first thing I did when examining your code was apply the regex \r\n( +) -> \r\n\1\1\1\1}

Answer 2

• Your leading whitespace is much smaller than is typical. Prefer 4 spaces for a level of indent.

• In java we typically put the curly brace { on the same line. If you insist on doing it on a new line, do it consistently (main) and don't add a blank line of whitespace before the curly brace.

• Variable names need to be clear and descriptive. str is meaningless - perhaps word? Likewise, firstVow is confusing, since nobody's getting married. Surely you mean firstVowel. Extra characters come at no charge, so if they make the variable name more descriptive, use them.

• Closeable resources need to be closed always. Use either try-with-resources or a finally block to close open resources. Otherwise an exception in the middle of execution will prevent the resource from being closed.

• Variables should be defined in the most constraining scope possible. All of your variables can and should be method variables, defined at the time of first need.

• You repeat code to see if a character is a vowel. Using either a method so you're only writing the code once or a Set to hold all vowels would be better.

• for loops would be easier to read than while loops, since you need all three statement anyway.

• Your last block of work (word starts with a consonant) assumes there'll eventually be a vowel in the word.

• It's generally preferable that methods return a value rather than performing direct IO themselves. If you don't follow that, the pattern you used for the first two blocks (multiple System.out.print calls) is much easier to read than the one big print with everything mushed together. If you do return a value, using a StringBuilder might be a better idea than using string concatenation.

• In several places you store a character and use to that with an indexOf when the index is just i or 0. You should simplify that down.

• In a couple of places you do if (condition) { } else if (!condition) { }. It's OK to just do if (condition) { } else { }