0

Below is the code i wrote for placing all anagrams next to each other, in a collection of strings. The output is not sorted as expected.In fact, the output is the same as the input. Where am I going wrong ?

package set2;

import java.util.Arrays;
import java.util.Comparator;

public class printAllAnagrams {
    public static void main(String[] args) {
        String[] s = { "Harsha", "ant", "sha", "tna", "ash" };
        sortAnagrams(s);
        for (String e : s) {
            System.out.println(e);
        }
    }

    private static void sortAnagrams(String[] s) {
        Arrays.sort(s, new Comparator<String>() {

            @Override
            public int compare(String s1, String s2) {
                s1.toLowerCase();
                s2.toLowerCase();

                if (s1.length() != s2.length()) {
                    return -1;
                } else {
                    char[] s1_char = s1.toCharArray();
                    char[] s2_char = s2.toCharArray();
                    Arrays.sort(s1_char);
                    Arrays.sort(s2_char);

                    for (int i = 0; i < s1_char.length; i++) {
                        if (s1_char[i] != s2_char[i]) {
                            return -1;
                        }
                    }
                }
                return 0;
            }

        });

    }
}
1
  • Debug your code to validate your expectations.
    – Kai
    Mar 20, 2013 at 15:39

6 Answers 6

0

This can't be right for sure:

if (s1.length() != s2.length()) {
    return -1;
}

It would mean that if s1.length() != s2.length() s1 < s2 and s2 < s1.

What I think you mean to do is:

public int compare(String s1, String s2) {
    if (s1.length() == s2.length()) {                   
        char[] s1_char = s1.toLowerCase().toCharArray();
        char[] s2_char = s2.toLowerCase().toCharArray();
        Arrays.sort(s1_char);
        Arrays.sort(s2_char);

        for (int i = 0; i < s1_char.length; i++) {
            if (s1_char[i] != s2_char[i]) {
                return (int)(s1_char[i] - s2_char[i]);
            }
        }
        return 0;
    } else {
        return s1.length() - s2.length();
    }
}
1
  • @downvoter If I run this code, it gives sha ash ant tna Harsha... that seems exactly what is asked... Mar 20, 2013 at 15:55
0

Without looking too closely, this

 s1.toLowerCase();

doesn't alter s1 but rather returns a new string which is a lower case variant of s1. In Java Strings are immutable. So you need to collect and work with the value returned from the above.

0

Why don't you just do the following:

        @Override
        public int compare(String s1, String s2) {
            return s1.toLowerCase().compareTo(s2.toLowerCase());
        }
0
0

Strings are immutable so just calling the method on a String object doesn't change the String itself. You should use the following:

s1 = s1.toLowerCase();
s2 = s2.toLowerCase();
0

Your comparator is not stable at all.

First of all you return -1 if the lengths are different. This means that depending on the operand order you might find that "asbd" > "ash" or the opposite.

Also you do the same with the char comparison.

if (s1_char[i] != s2_char[i]) {
   return -1;
}

Substitute this with:

if (s1_char[i] != s2_char[i]) {
  return s1_char[i] > s2_char[i] ? 1 : -1;
}

Use the same pattern for the length comparison.

EDIT Returning -1 from a compare methods, means that you find the first operand smaller than the second as per the documentation.

0

From the javadoc:

The implementor must ensure that sgn(compare(x,y)) == -sgn(compare(y,x)) for all x and y. (This implies that compare(x,y) must throw an exception if and only if compare(y,x) throws an exception.).

This basically means if you call compare(s1,s2) it has to yield -1 * compare(s2,s1) and neither of your return -1; statements follow this. Instead of that, you can compare the ints and characters to each other and return that value, for instance this code works (instead of the return -1;):

return Integer.compare(s1.length(),s2.length()); //for the ints

return Character.compare(s1_char[i],s2_char[i]); //for the chars

Also take a look at Vishal's answer, that's another bug.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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