Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Java is complaining about dead code in my translator:

public static void pigLatinify(String fname) throws IOException 
{
    File file = new File("projectdata.txt");

    try 
    {
        Scanner scan1 = new Scanner(file);
        while (scan1.hasNextLine()) 
        {
            Scanner scan2 = new Scanner(scan1.nextLine());
            while (scan2.hasNext())
            {
                String s = scan2.next();
                boolean b = Syllable.countSyllables(s);
                char firstLetter = s.charAt(0);
                if (b = false && firstLetter=='a' || firstLetter=='i' || firstLetter=='o' || firstLetter=='e' || 
                        firstLetter=='u' || firstLetter=='A' || firstLetter=='I' || firstLetter=='O' || 
                        firstLetter=='E' || firstLetter=='U')
                {
                    String output = s + "hay" + " ";
                    System.out.print(output);
                }
                else if (b = true && (firstLetter=='a' || firstLetter=='i' || firstLetter=='o' || firstLetter=='e' || 
                        firstLetter=='u' || firstLetter=='A' || firstLetter=='I' || firstLetter=='O' || 
                        firstLetter=='E' || firstLetter=='U')) 
                {
                    String output = s + "way" + " ";
                    System.out.print(output);
                }
                else 
                {
                    String restOfWord = s.substring(1);
                    String output = restOfWord + firstLetter + "ay" + " ";
                    System.out.print(output);
                }

            }
            System.out.println("");
            scan2.close();
        }
        scan1.close();
    } 

    catch (FileNotFoundException e) 
    {
        e.printStackTrace();
    }
}

The method called (Syllable.countSyllables) is:

class Syllable
{
    public static boolean countSyllables(String word)
    {
        int vowelCount = 0;
        word.toLowerCase();
        for (int a = 0; a < word.length(); a++)
        {
            char test = word.charAt(a);
            if (test == 'a' || test == 'e' || test == 'i' || test == 'o' || test == 'u')
            vowelCount++;
        }
        if (vowelCount > 1)
            return true;
        return false;
    }
}

Java says the result of local variable boolean b isn't being used. It also flags up the first "if" statement and says that the "firstLetter=='a' is dead code. It doesn't flag anything else up though.

Have I done something very silly I'm missing??

Thanks!

share|improve this question
add comment

closed as off-topic by Jamal Nov 27 '13 at 22:05

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Your question must contain working code for us to review it here. For questions regarding specific problems encountered while coding, try Stack Overflow. After getting your code to work, you may edit this question seeking a review of your working code." – Jamal
If this question can be reworded to fit the rules in the help center, please edit the question.

5 Answers

up vote 7 down vote accepted

I don't know Java much, but I see a possible syntax error.

if (b = false && firstLetter=='a' || firstLetter=='i' || firstLetter=='o' || firstLetter=='e' || 
                    firstLetter=='u' || firstLetter=='A' || firstLetter=='I' || firstLetter=='O' || 
                    firstLetter=='E' || firstLetter=='U')

Change to: b=false to b==false

Looks like you're setting the boolean value, not checking what it equals. It could be that the countSyllables is not actually being used (thus dead) since you're only turning around to set b to something else. Technically only true, so it's also not being used.

Disclaimer: Not a Java programmer, just C#.

share|improve this answer
    
I changed it and I think that's it, no more code flagging! Funny thing is that all the code worked the same way anyway. –  Andrew Martin Nov 15 '12 at 15:04
3  
+1. Note that there's also a b = true later on that needs to be b == true (or just b; likewise, b == false could just be ! b). –  ruakh Nov 15 '12 at 19:14
add comment

I'm going to go beyond the scope of the original question to do a more thorough critique. Please don't be overwhelmed — you can take as much or as little of this advice as you wish.

The main faults I see are:

  • Separation of concerns. Your has to do multiple things, and it tries to do them all together:

    • File input
    • Tokenizing
    • Pig Latin transformation
    • Output

    If you separate these problems into layers, your code will be easier to read, maintain, and reuse.

  • Complicated conditions. Your if-conditions are complex, combining many boolean expressions with && and ||. Furthermore, similar conditions appear several times. Both of these problems are Bad Code Smells, and are contributing factors to the error that led you to ask this question in the first place.

    What you wanted to do was detect vowels. There are more expressive ways to handle strings than to work character by character.

  • Excessive instantiations of Scanners. You should be able to handle the whole file using just one Scanner.

  • FileNotFoundException got swallowed. Since the pigLatinify() method throws IOException anyway, you're probably better off letting FileNotFoundException propagate to the caller.

  • pigLatinify(String) interface is prone to misuse. If the method expects a filename, then it should probably take a File argument rather than a String. Otherwise, someone might get the impression that you should pass the textual data to be transformed rather than the name of the file containing the data.

Putting everything together...

/**
 * Regular expression to match and capture a word and capture any
 * inter-word characters that follow the word.  A word consists of
 * Unicode letters and may contain one apostrophe.
 */
private static Pattern wordAndJunkPat = Pattern.compile("((?:\\p{L}+(?:'\\p{L}+)?)?)([^\\p{L}]*)");

public static void pigLatinify(File f) throws IOException {
    Reader r = new InputStreamReader(new FileInputStream(f));
    Writer w = new OutputStreamWriter(System.out);
    pigLatinify(r, w);
}

public static void pigLatinify(Reader r, Writer w) throws IOException {
    Scanner scan = new Scanner(r);
    try {
        while (true) {
            while (null != scan.findInLine(wordAndJunkPat)) {
                MatchResult result = scan.match();
                w.write(pigLatinifyWord(result.group(1)));
                w.write(result.group(2));
            }
            w.write("\n");
            w.flush();
            scan.nextLine();
        }
    } catch (NoSuchElementException eof) {
        scan.close();
    }
}

public static String pigLatinifyWord(String word) {
    if ("".equals(word)) {
        return "";
    }
    char firstChar = word.charAt(0);
    if ("AaEeIiOoUu".indexOf(firstChar) >= 0) {
        // word starts with vowel
        return word + (Syllabifier.isMultiSyllabic(word) ? "way" : "hay");
    } else {
        // word starts with consonant
        return word.substring(1) + firstChar + "ay";
    }
}

As for the syllable counter:

  • countSyllables() is poorly named. The name suggests that it should return the number of syllables, not a boolean.

    public class Syllabifier {
    
        private static Pattern multiVowel = Pattern.compile("[aeiou].*[aeiou]", Pattern.CASE_INSENSITIVE);
    
        /**
         * Determines whether a word contains multiple syllables.
         * TODO: The current heuristic is overly simplistic: it just checks
         * whether the word contains multiple vowels.  For a better
         * heuristic, see http://stackoverflow.com/questions/405161
         */
        public static boolean isMultiSyllabic(String word) {
            return multiVowel.matcher(word).find();
        }
    }
    
share|improve this answer
    
+1 I may have answered the question, but you reviewed the code. :) –  StyxRiver Nov 16 '12 at 16:30
    
I know I'm a little late responding to this post, but I just wanted to say thank you for all your hard work. I'm going to go through it now and try and tidy up my code accordingly. Thanks! –  Andrew Martin Nov 18 '12 at 14:28
    
One note: What does "((?:\\p{L}+(?:'\\p{L}+)?)?)([^\\p{L}]*)" mean? –  Andrew Martin Nov 18 '12 at 14:34
    
And a second note! In your comment to Kinjal's answer below, you said I could use ("aeiouAEIOU".indexOf(firstLetter) >= 0). This works brilliantly but I was wondering why it had to be ">=0". I've tried changing it to == 0, but it gives incorrect output. Could you explain what this little snippet is doing? Thanks –  Andrew Martin Nov 18 '12 at 15:02
    
Actually, scrap just that last comment. The "aeiouAEIOU" is the string to be scanned and if firstLetter appears anywhere in that string (from position 0 to whatever the highest position is), the if statement will proceed, or else move on. –  Andrew Martin Nov 18 '12 at 15:11
show 1 more comment

few things about the code.

The if-else block seems to be clumsy. you could try something like-

boolean b = Syllable.countSyllables(s);
char firstLetter = s.charAt(0);
String output = "";
Collection<Character> letters = Arrays.asList('a', 'e', 'i', 'o', 'u',
                                              'A', 'E', 'I', 'O', 'U');
if (letters.contains(firstLetter)) {
  output = b ? (s + "hay" + " ") : (s + "way" + " "); 
} else {
  // do something
}
System.out.println(output);
// rest of the code...
share|improve this answer
    
Looks pretty good - I haven't used collections/array lists etc yet in my course, so wasn't sure how they worked. –  Andrew Martin Nov 15 '12 at 16:09
    
It would be more compact and efficient to do if ("aeiouAEIOU".indexOf(firstLetter) >= 0) { ... } Whether you consider it to be more readable would be a matter of taste. The intention of .indexOf() is slightly less clear than .contains(), but brevity has its advantages too. –  200_success Nov 15 '12 at 22:05
    
@200_success: So would your snippet of code replace all the above effectively? –  Andrew Martin Nov 15 '12 at 22:15
    
@Andrew Martin, using indexOf will not replace all the above code. It is another way of checking the vowels for the if condition if you want to avoid using a collection. –  Kinjal Nov 16 '12 at 2:51
add comment

Just going to add some pointers (some style, some functional):

  • It's unlikely in this small program, but if your scanners fail while translating, they will not be closed properly. I would take a look at how you can better handle them in the case of failure (finally block or Java 7's try-with-resources). Proper resource handling is a good habit to learn early!
  • A FileNotFoundException can be avoided here
  • The result of word.toLowerCase() isn't stored (see below), so it isn't going to work as intended.

  • Your countSyllables(String) method probably isn't named right for its purpose. Something like hasMultipleSyllables(String) might be better. Since you are only looking for a vowelCount > 1, you can return earlier. Perhaps something like:

    public static boolean hasMultipleSyllables(String word)
    {
        String w = word.toLowerCase(); // Don't forget to store this method result!
        int vowelCount = 0;
        for (int i = 0; i < w.length(); i++)
        {
            if (isVowel(w.charAt(i))
            {
                vowelCount++;
                if (vowelCount > 1) 
                    return true;
            }
        }
        return false;
    }
    
    private static boolean isVowel(char c)
    {
        return (c == 'a' || c == 'e' || c == 'i' || c == 'o' || c == 'u')
    }
    

This is out of scope for this project/program, but it wouldn't hurt to look over some of the widely used libraries out there for some help/ideas/inspiration in the future!

Apache Commons Lang

Guava Strings

share|improve this answer
add comment

A side point, and probably out of scope for this exercise, but your method for counting syllables is going to fail in many cases. Many one-syllable words have multiple vowels (e.g. beef, here, each, or my favorite: queue). You could optimize it by looking for vowels separated by a consonant, which would be more accurate, but still not perfect. (e.g. lion has the same format as beef, but would generally be considered to have 2 syllables.) You could get more specific, ignoring the letter 'e' at the end of a word, or the 'u' after a 'q', and counting certain vowel-pairs as a single vowel. Unfortunately, there are so many irregularities in the English language that it's far from trivial to program a computer to recognize how many syllables there are. Though these rules should catch most of them.

share|improve this answer
add comment

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