Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

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

I know a lot of memory is being used, but how do I make this more efficient in space(and time) .

   public class AnagramsTogether {


    public static String getAnagramsTogether(String sentence){

        if(sentence == null || sentence.trim().equals("")) return null;

        String[] words = sentence.split(" ");
        HashMap<String,List<String>> map =new HashMap<String,List<String>>();


        for(String word:words)
        {
            char[] wordArray = word.toCharArray();
            Arrays.sort(wordArray);

            List<String> tempList = map.get(new String(wordArray));
            if(tempList == null) tempList = new ArrayList<String>(); 
            tempList.add(word);
                            map.put(new String(wordArray),tempList);
        }

        StringBuilder result = new StringBuilder();
        for(Map.Entry<String, List<String>> entry : map.entrySet())
        {

            for(String item:entry.getValue()){
                result.append(item+ " ");
            }
        }


        return result.toString();
    }



    public static void main(String[] args) {
         System.out.println(getAnagramsTogether("cat dog act tac god"));
    }

}
share|improve this question
up vote 5 down vote accepted

Whitespace discipline

While your indentation is good, the placement of whitespace isn't very good yet. In Java, we tend to put whitespace after keywords and before braces. A beautifier with standard settings would make your code more readable already.

Use the most common type that still does the job.

For example, your variable map should be declared as Map, not HashMap.

Smarter Use of Map API

Instead of getting the list, checking whether it's null and if it isn't null, creating it, and then always putting it, you could use Map.computeIfAbsent(), like this:

map
    .computeIfAbsent(normalizedAnagram, s -> ArrayList::new)
    .add(word);

Split your logic

Your function getAnagramsTogether is doing many things. You could split it into multiple methods. For example, you could have a separate method normalizeAnagram() which takes a String and returns a new String with the characters sorted.

Bug in your call to Arrays.sort(char[] wordArray)

It should be noted that char is NOT a character. It is a UTF-16 fragment. For the first Unicode plane, i.e. code points with values that can be represented by 16 bits, char is a character. But for code points with higher values, a char contains only part of the character, and the way how you construct the normalized anagram is tearing characters apart.

It may not matter to you, as probably all relevant words in your use cases would have code points in the 16 bit plane.

Don't use + in StringBuilder.append()

You should be consistent, either use + or append(). But using + inside StringBuilder.append() doesn't make sense. We use StringBuilder in order to have String-concatenation which is faster and uses less memory than using +. Using + in an argument to StringBuilder.append() is completely contrary to the intention of using StringBuilder in the first place.

Don't construct the same object twice.

You're calling new String(wordArray) twice for no good reason. You could just create a single String normalizedAnagram and use that.

Consider a smarter way of String concatenation.

The following code concatenates your final Map into a String in a smarter way:

map
    .entrySet()
    .stream()
    .flatMap(entry -> entry.getValue().stream())
    .collect(Collectors.joining(" "))
share|improve this answer
    
Thanks a lot Christian! :) – DontForgetTheSemiColon Feb 14 at 4:00

Use String.isEmpty instead of .equals("")

if(sentence == null || sentence.trim().equals("")) return null;

Comparing a string with a empty string is slower than using isEmpty() because the first step in comparing a string getting the size of BOTH strings.

if(sentence == null || sentence.trim().isEmpty()) return null;
share|improve this answer

Returning null

You should throw an exception instead of returning null. Returning null is almost never a good idea.

Using java 8:

If you have java 8 available you can use streams:

   public static String getAnagramsTogether2(String sentence)
   {
       if(sentence == null ||  sentence.trim().isEmpty()) 
           throw new IllegalArgumentException();

       Map<String, List<String>> map =Arrays.stream(sentence.split(" "))
                                            .collect(Collectors.groupingBy(
                                                   AnagramsTogether::sortString));

       return map.values()
                 .stream()
                 .flatMap(List::stream)
                 .collect(Collectors.joining(" "));
   }
   public static String sortString(String string) {

       return  string.chars()
                     .sorted()
                     .collect(StringBuilder::new, 
                              StringBuilder::appendCodePoint, 
                              StringBuilder::append)
                     .toString();          
   }
share|improve this answer
    
i'm kinda new to java 8 but this reduces the size of code like crazy !:) – DontForgetTheSemiColon Feb 14 at 4:39
1  
@MAG Do you mind explaining the "magic" of sortString? – AlexR Feb 14 at 10:48

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.