Weighted Uniform Strings Problem

Adapted from this HackerRank problem.



Instead of printing YES or NO, I just want to return the Set of all possible weights for an input String.

I found this example to be illustrative



Implementation

Some of my thoughts are in the implementation as comments

import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;

public class WeightedUniformStrings {
  public static Set&lt;Long&gt; getWeights(String s) {
    // I thought about using streams here, but I'm not using java 8
    Set&lt;Long&gt; weights = new HashSet&lt;&gt;();
    Map&lt;Character, Long&gt; consecutiveCharacterCounts = WeightedUniformStrings.getConsecutiveCharactersCounts(s);
    for (Map.Entry&lt;Character, Long&gt; entry : consecutiveCharacterCounts.entrySet()) {
      if (entry.getValue() != null &amp;&amp; entry.getKey() != null) {
        long weight = WeightedUniformStrings.calculateWeight(entry.getKey());
        for (long i = 0; i &lt; entry.getValue() + 1; i++) {
          weights.add(weight * i);
        }
      }
    }
    return weights;
  }

  private static long calculateWeight(char c) {
    // Could add a check to see if character is alphabetical
    return Character.toLowerCase(c) - 'a' + 1;
  }

  private static Map&lt;Character, Long&gt; getConsecutiveCharactersCounts(String s) {
    if (s.isEmpty()) {
      return new HashMap&lt;&gt;();
    }

    Map&lt;Character, Long&gt; consecutiveCharacterCounts = new HashMap&lt;&gt;();
    char[] chars = s.toCharArray();

    // I'm not a big fan of this initialization + for loop - but I haven't thought of a better alternative implementation
    char startingCharacter = chars[0];
    long consecutiveCharacterCount = 1;
    consecutiveCharacterCounts.put(startingCharacter, consecutiveCharacterCount);

    for (int i = 1; i &lt; chars.length; i++) {
      char currentCharacter = chars[i];

      if (currentCharacter == startingCharacter) {
        consecutiveCharacterCount++;
      }

      if (currentCharacter != startingCharacter || i == chars.length - 1) {
        Long characterCount = consecutiveCharacterCounts.get(startingCharacter);
        if (characterCount == null || consecutiveCharacterCount &gt; characterCount) {
          consecutiveCharacterCounts.put(startingCharacter, consecutiveCharacterCount);
        }
      }

      // Doing this logical check twice feels weird
      if (currentCharacter != startingCharacter) {
        startingCharacter = currentCharacter;
        consecutiveCharacterCount = 1;
        Long characterCount = consecutiveCharacterCounts.get(startingCharacter);
        if (characterCount == null || consecutiveCharacterCount &gt; characterCount) {
          consecutiveCharacterCounts.put(startingCharacter, consecutiveCharacterCount);
        }
      }
    }

    return consecutiveCharacterCounts;
  }
}
Avoid unnecessary math


        for (long i = 0; i &lt; entry.getValue() + 1; i++) {



As you noted in your comment, this should start at 1.  

        for (long i = 1; i &lt; entry.getValue() + 1; i++) {


But we can also simplify the end.  

        for (int i = 1; i &lt;= entry.getValue(); i++) {


We want to process entry.getValue() and then stop, so do that.  No need to determine the next place where we don't want to be.  The other way works, but this is easier to read.  

We also don't need to use long here.  Because Java arrays can't be indexed past the range of an int, we can be sure that none of the counts will exceed the capacity of an int.  Of course, if we change to int here, we should change everywhere else as well.  

Use a helper


    consecutiveCharacterCounts.put(startingCharacter, consecutiveCharacterCount);

    for (int i = 1; i &lt; chars.length; i++) {
      char currentCharacter = chars[i];

      if (currentCharacter == startingCharacter) {
        consecutiveCharacterCount++;
      }

      if (currentCharacter != startingCharacter || i == chars.length - 1) {
        Long characterCount = consecutiveCharacterCounts.get(startingCharacter);
        if (characterCount == null || consecutiveCharacterCount &gt; characterCount) {
          consecutiveCharacterCounts.put(startingCharacter, consecutiveCharacterCount);
        }
      }

      // Doing this logical check twice feels weird
      if (currentCharacter != startingCharacter) {
        startingCharacter = currentCharacter;
        consecutiveCharacterCount = 1;
        Long characterCount = consecutiveCharacterCounts.get(startingCharacter);
        if (characterCount == null || consecutiveCharacterCount &gt; characterCount) {
          consecutiveCharacterCounts.put(startingCharacter, consecutiveCharacterCount);
        }
      }
    }



You can simplify this with a helper method:  

public static void update(Map&lt;Character, Long&gt; counts, char c, int count) {
    Long oldCount = counts.get(c);
    if (oldCount == null || count &gt; oldCount) {
        counts.put(c, count);
    }
}


Then we can change the bounds of the for loop.  

    for (int i = 1, n = chars.length - 1; i &lt; n; i++) {
      if (chars[i] == startingCharacter) {
        consecutiveCharacterCount++;
        continue;
      }

      update(consecutiveCharacterCounts, startingCharacter, consecutiveCharacterCount);
      startingCharacter = chars[i];
      consecutiveCharacterCount = 1;
    }

    if (chars[chars.length - 1] == startingCharacter) {
      consecutiveCharacterCount++;
    } else {
      update(consecutiveCharacterCounts, startingCharacter, consecutiveCharacterCount);
      consecutiveCharacterCount = 1;
    }

    update(consecutiveCharacterCounts, chars[chars.length - 1], consecutiveCharacterCount);


Now we do one fewer iteration and on exiting the loop we do different processing.  This saves us having to do the extra check on every iteration of the loop.  

Now we don't have to compare currentCharacter and startingCharacter three times.  Once is enough.  

Using the continue saves us a level of indent over using an else.  

We don't need to update the Map for the first one of each letter.  The logic works without that.