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.
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.

share|improve this question
    
please provide a bit of background. Are you trying to implement your own sorting algorithm? Otherwise, trust Arrays.sort from Java. Your sorting algorithm is using bubble sort which has O(n^2) complexity. There are faster algorithms. Also, do you have restriction in using built in methods in Stringclass? Have a look here –  zencv Apr 2 at 8:22
    
This was an interview question in which I was asked to implement any sorting technique and not use the Arrays.sort()...no restriction on built in methods of String class –  user3296744 Apr 2 at 10:43
    
then you can probably use toCharArray method of String. Sorting of course you have to perform with your own provided method then. If they had not insisted on sorting, another way would be to build 2 maps, each with key=a unique char and val=#occurences and then check for equality. That is faster than sorting solution. Btw., this was asked for an interview to me as well few weeks ago, I proposed both solutions I mentioned and was selected (though I did not know what an Anagram was until then) :-) –  zencv Apr 2 at 11:26
    
Can you elaborate on the 2nd solution? I didnt get it completely –  user3296744 Apr 2 at 11:39
    
check this post, especially the selected answer. –  zencv Apr 2 at 11:44

3 Answers 3

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(...);
        }
    }
    
share|improve this answer
    
System.out.println is an expansive operation most of the time, this is a really major performance problem in a many cases. +1 for removing it! –  Marc-Andre Apr 1 at 17:53
    
Thanks for your feedback! –  user3296744 Apr 2 at 10:44

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.

share|improve this answer
    
Thanks for your feedback....Could you also provide feedback on how i could optimize the code? –  user3296744 Apr 1 at 17:13
    
I don't have time at the moment to look deep in your algo, but if there is no other answer by the time I'm free, I will gladly do it. –  Marc-Andre Apr 1 at 17:14
    
Thank you very much!!!!....Wud be great if u could!!!! –  user3296744 Apr 1 at 17:15

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.

share|improve this answer
    
Thanks for your valuable feedback....I will incorporate it from the next program i write –  user3296744 Apr 1 at 17:57
    
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......Can you elaborate on this? –  user3296744 Apr 2 at 10:34
    
@user3296744 The suggestion is to either make sortCharArray be a void return method, or creating a copy of the array and return that. Currently, your char[] charArray gets modified inside the method, and is also returned. I would suggest making the method public static void sortCharArray(char[] charArray) –  Simon André Forsberg Apr 2 at 10:54

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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