2
\$\begingroup\$

This question follows on from :Previous Question

Here is what my reworked code looks like. I neatened it up as much as I could. Changed some of the semantics of it. I also tried to add some faster exit points and checks to prevent errors.

Any further critiques?

 /**
 * @author              :KyleMHB
 * Project Number       :0002
 * Project Name         :Anagramatic
 * IDE                  :NETBEANS
 * Goal of Project      - 
 * Capture user input, compare to a dictionary file for anagrams,
 * output number of matches and the matches.
 */

package anagramatic;

import java.io.File;
import java.io.FileNotFoundException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Iterator;
import java.util.List;
import java.util.Scanner;
import javax.swing.JOptionPane; 

-

public class Anagramatic {
public static void main(String[] args) throws FileNotFoundException{
    String anagram=getInput("Enter the word you would like to process");
    List<String> words=readAnagramsFromFile(anagram, new File("words.txt"));
    String output= formatOutput(anagram,words);
    displayOutput(output);
}//pvsm

-

private static String getInput(String prompt) {
    String input = JOptionPane.showInputDialog(null,prompt);
    return input;
}//getInput

-

  private static List readAnagramsFromFile(String word, File f) 
        throws FileNotFoundException{
    ArrayList<String> anagrams = new ArrayList<>(); 
    try(Scanner s = new Scanner(f)){
        while(s.hasNext()){
            String candidate=s.next();
            if ( (candidate.length()==word.length()) &&
                    (checkMatch(word,candidate)==true)){
                anagrams.add(candidate);
            }
        }
}
    return anagrams;
}//readFile

-

private static boolean checkMatch(String word, String candidate) {
    char[] wordArray = word.toCharArray();
    char[] candidateArray = candidate.toCharArray();
    if (Arrays.equals(wordArray, candidateArray)){
        return false;
    }
    Arrays.sort(wordArray);
    Arrays.sort(candidateArray);
    if(Arrays.equals(wordArray, candidateArray)){
        return true;
    }
    /**I did not use an else function for the the below return
    *because if (Arrays.equals(wordArray, candidateArray))==true
    *it will break on the return*/
    return false;

}//match

-

private static String formatOutput(String original, List<String> words) {
    StringBuilder output=new StringBuilder("[ ");
    int counter=0;
    Iterator<String> wordIt =words.iterator();
    while(wordIt.hasNext()){
       output.append(wordIt.next());
       if(wordIt.hasNext()){
           output.append((++counter % 8 == 0)? ",\n" : ", ");
       }
    }
    output.append(" ]");
    return ("The Anagram "+original+" has "+words.size()+" matches.\n\nThey are:\n"+output.toString());
}//formatOutput

-

private static void displayOutput(String output){
    JOptionPane.showMessageDialog(null,output);
}//displayOutput
}
\$\endgroup\$
2
\$\begingroup\$

Four issues with private static boolean checkMatch(String word, String candidate):

  • checkMatch is not as descriptive as a name could be. I think private static boolean isAnagram(String a, String b) would be better. I would also rename the parameters to acknowledge that they are symmetrical: it doesn't matter which is the original word, and which is the candidate.
  • You don't need to end with an if-else. You can just say return Arrays.equals(wordArray, candidateArray);
  • You would be re-sorting the characters of the original word each time you test a candidate. That's OK, I suppose, if you prefer clean code over efficiency.
  • Why not start with

    private static boolean isAnagram(String a, String b) {
        if (a.length() != b.length()) {
            return false;
        }
    
        // Continue with a thorough comparison...
        ...
    }
    

    Then the code in readAnagramsFromFile() can read very smoothly:

    if (isAnagram(word, candidate)) {
        anagrams.add(candidate);
    }
    

Note that the return type of readAnagramsFromFile() should be List<String>, not just a generic List.

\$\endgroup\$
  • \$\begingroup\$ So is having if ( (candidate.length()==word.length()) && (checkMatch(word,candidate)==true)) in my readAnagramsFromFile() less efficient than the solution you suggested? I was trying to create an earlier exit point. I changed the String names as you suggested as well as the method name to checkAnagram(). Finally what is the difference between using the return type of List<String> for readAnagramsFromFile()? \$\endgroup\$ – Kylo Oct 4 '13 at 19:05
  • \$\begingroup\$ Moving the length check inside isAnagram() makes isAnagram() fulfill its duty the way it should, and is therefore better code. You might be able to save the overhead of a function call by checking the length before calling isAnagram(), but since isAnagram() is private static, there is a good chance that the JIT will inline the function call anyway, especially if you also declare isAnagram() to be final. However, considering that you are willing to re-sort the characters of the original word, it makes no sense to obsess over the overhead of a function call. \$\endgroup\$ – 200_success Oct 4 '13 at 19:28
  • \$\begingroup\$ readAnagramsFromFile() returns a list of strings, so your code should state that. Otherwise, it could be returning a list of who-know-what kind of objects. (The runtime behaviour would be identical either way. However, using Java Generics consistently lets the compiler do its job better by catching programming mistakes and reducing the need for casting.) \$\endgroup\$ – 200_success Oct 4 '13 at 19:35
  • \$\begingroup\$ And it makes the code a lot cleaner in the readAnagramsFromFile(). That makes sense, about the List<String>. Thanks for the explanations and your patience. Answer Accepted. \$\endgroup\$ – Kylo Oct 4 '13 at 19:35

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.