Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

Please review my code:

public class Anagram {

    public static void main(String args[]) {
        String[] arr = { "abc", "cbc", "bcc", "dog", "god", "mary", "army",
                "rty" };
        HashMap<Integer, List<String>> mapList = new HashMap<Integer, List<String>>();

        for (int i = 0; i < arr.length; i++) {
            int hashcode = gethashcode(arr[i]);

            if (mapList != null && mapList.get(hashcode) == null) {
                List<String> list = new ArrayList<String>();
                mapList.put(hashcode, list);
                list.add(arr[i]);
            } else {
                List<String> list = mapList.get(hashcode);
                list.add(arr[i]);
            }
        }

        printMap(mapList);

        System.out.println("Count=" + mapList.size());
    }

    private static void printMap(HashMap<Integer, List<String>> mapList) {

        if (mapList != null && mapList.size() > 0) {
            for (Integer key : mapList.keySet()) {
                List<String> list = mapList.get(key);

                System.out.print("[");
                if (list != null && list.size() > 0) {
                    for (int k = 0; k < list.size(); k++) {
                        System.out.print(list.get(k) + " ");
                    }
                }
                System.out.print("]");
            }
        }

    }

    private static int gethashcode(String str) {
        int hashcode = 0;
        char ch[] = str.toCharArray();

        for (int i = 0; i < ch.length; i++) {
            if (hashcode != 0) {
                hashcode = hashcode + String.valueOf(ch[i]).hashCode();
            } else {
                hashcode = String.valueOf(ch[i]).hashCode();
            }
        }
        return hashcode;
    }
}

Output:

[mary army ][abc ][rty ][cbc bcc ][dog god ]    Count=5
share|improve this question
up vote 2 down vote accepted

Bug

I fed "aad" and "abc" to your program, and it considered the two strings anagrams of each other. The problem is that you are trying to use a hashcode to determine whether two strings are anagrams of each other. However:

  1. Your hashcode is not very good because it simply adds the characters of the string together. Therefore "aad" and "abc" were considered equivalent.

  2. No matter what, a 32 bit hashcode is going to have some problems because there are more than 2^32 different anagram possibilities if you use long enough strings. Even a 64 bit hashcode would not be enough.

I suggest that you think about creating a sorted string (i.e. alphabetized) as your hash key instead of a numerical hashcode.

Simplification

This code:

       if (mapList != null && mapList.get(hashcode) == null) {
            List<String> list = new ArrayList<String>();
            mapList.put(hashcode, list);
            list.add(arr[i]);
        } else {
            List<String> list = mapList.get(hashcode);
            list.add(arr[i]);
        }

could be simplified to:

        List<String> list = mapList.get(hashcode);

        if (list == null) {
            list = new ArrayList<String>();
            mapList.put(hashcode, list);
        }
        list.add(arr[i]);
share|improve this answer

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.