Anagram Checking

Question

public class AnagramNumber {

    public static void checkAnagram(String[] str)
    {   
        String[] sortedArray=new String[str.length];
        for(int i=0;i<str.length;i++)
        {
        char[] strToCharArray=str[i].toCharArray();
        sortedArray[i]=new String(sortCharArray(strToCharArray));
        }

        boolean flag=true;
        for(int j=0;j<sortedArray.length-1;j++)
        {   
            if(!sortedArray[j].equals(sortedArray[j+1]))
            {
                flag=false;
                break;
            }
        }

        if(flag)
            System.out.println("The array has anagrams");
        else
            System.out.println("The array doesnt have only anagrams");  
    }

    public static char[] sortCharArray(char[] charArray)
    {
        char temp='\u0000';
        for(int i=0;i<charArray.length-1;i++)
        {
            for(int j=i+1;j<charArray.length;j++)
            {
                if(charArray[j]>charArray[i])
                {
                    temp=charArray[j];
                    charArray[j]=charArray[i];
                    charArray[i]=temp;
                }   

            }
            System.out.println(Arrays.toString(charArray));
        }

        return charArray;
    }
    public static void main(String[] args) {
        // TODO Auto-generated method stub

        String[] str={"hello","helol","elloh"};

        checkAnagram(str);

    }
}

Please review program and provide the best practices and feedback on code optimizing.

Answer 1

Besides what others have mentioned:

• System.out.println in your sorting method just confused me. Either inform what exactly you are logging or remove it entirely. Preferably remove it.

• Speaking of removing, your sorting method can be replaced by a call to Arrays.sort(strToCharArray);

  char[] strToCharArray = str[i].toCharArray();
  Arrays.sort(strToCharArray);
  sortedArray[i] = new String(strToCharArray);
  

• Make your checkAnagram method (which should be renamed to verifyIsOnlyAnagrams or something similar) more flexible by returning the value of flag instead of outputting the result.

  boolean flag = true;
  for (int j = 0; j < sortedArray.length - 1; j++) {
      ...
  }
  
  return flag;
  

  And then let the main method deal with the output:

  public static void main(String[] args) {
      String[] str={"hello","helol","elloh"};
      if (verifyIsOnlyAnagram(str)) {
          System.out.println(...);
      }
      else {
          System.out.println(...);
      }
  }
  

Answer 2

Indentation

Your indentation seems consistent in your code, except for this part. It could be a problem in the question here on Code Review, but always pay attention to the indentation of your code. It's a major point for good readability.

    for(int i=0;i<str.length;i++)
    {
    char[] strToCharArray=str[i].toCharArray();
    sortedArray[i]=new String(sortCharArray(strToCharArray));
    }

You should add spaces in all your conditions :

for(int i=0;i<str.length;i++)

Should be :

for(int i = 0; i < str.length; i++)

Brace { }

This could be argue whether or not this is best practice, but I always like to see brackets, even if there is only one line of code. This can save a lot of troubles and if you ever need to add something to the block you won't have to add braces.

    if(flag)
        System.out.println("The array has anagrams");
    else
        System.out.println("The array doesnt have only anagrams");

One thing for sure is that the first { should always be at the end of the line, not on a new line.

public static void checkAnagram(String[] str) {

}

Comments

You should remove // TODO Auto-generated method stub since this is now obsolete. This serve nothing in your code.

Naming

There is some variables that could have a better name. Take boolean flag, by the name I know that it's a flag, but a flag for what ? This is not clear at all by the name. You could have use a relevant name that would show what are you flagging.

Another example :

public static void checkAnagram(String[] str)

The name str is not relevant at all. I already know by the class that this is a String.

Answer 3

Naming
Your naming is quite verbose, which is good (not counting flag), but sometimes, although the name looks meaningful, it actually does not convey any useful meaning:

• AnagramNumber - what does this class actually do? Counts the number of anagrams? Maybe finds anagrams for a number? Someone reading your code will never guess...
• checkAnagram - what does it check for? Does it return anything?
• str - OK, this is just a generic name, but it is extra confusing, since str implies a single string, while you expect an array. Btw, an array of words? sentences? URLs?
• "The array has anagrams" vs "The array doesnt have only anagrams" - these sentences are not complementary, so one is obviously wrong. The first should say "The array has only anagrams". This is especially important, because you are not only talking with the reader of your code, here you are talking to your user. Confusing him means you have failed your task.
• char temp='\u0000'; - this value is never used, so making it so verbose is confusing. A better choice would be not initializing the temp at all (char temp), or, better yet, declare it only where needed (char temp = charArray[i])
• public static char[] sortCharArray(char[] charArray) - here the problem is a little more subtle, but all the more serious. The signature of the method (returning a char[]) implies that it creates a new sorted array. Actually, it modifies the input array itself.
  Either create a new array, and leave the one given as it is, or remove the return value. This will make it clear that the input array changes.

Exit Strategy
Your use of flag makes your code a little awkward. Initializing it with false, and changing it to true makes it even more obfuscated, as it is counter-intuitive to most coders. As its name does not convey it any meaning, your gentle reader is left to guess its meaning by reading your code over and over...
I would drop the flag altogether, breaking from the method itself upon finding your stop condition:

for(int j=0;j<sortedArray.length-1;j++)
{   
    if(!sortedArray[j].equals(sortedArray[j+1]))
    {
        System.out.println("The array doesn't have only anagrams");  
        return;
    }
}

System.out.println("The array has only anagrams");

Optimizations
Since you don't give any background, it is hard to give you feedback on your algorithm - are you trying to implement a specific sorting algorithm on your own? are you simply not familiar with java's own solution to sorting (Arrays.sort)? Optimal solutions for sorting an array are with complexity of \$O(n \cdot log(n))\$, your solution is \$O(n^2)\$, so there is room for improvement there... you can find a myriad of sorting algorithms here.

You could further optimize your code by making a few quick checks on your data before doing the heavy lifting. For example: if not all the strings have the same length, you can immediately determine that they are not all an anagram of one another, and save yourself the sorting.