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 getWeights(String s) {
// I thought about using streams here, but I'm not using java 8
Set weights = new HashSet<>();
Map consecutiveCharacterCounts = WeightedUniformStrings.getConsecutiveCharactersCounts(s);
for (Map.Entry entry : consecutiveCharacterCounts.entrySet()) {
if (entry.getValue() != null && entry.getKey() != null) {
long weight = WeightedUniformStrings.calculateWeight(entry.getKey());
for (long i = 0; i getConsecutiveCharactersCounts(String s) {
if (s.isEmpty()) {
return new HashMap<>();
}

Map consecutiveCharacterCounts = new HashMap<>();
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 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 > characterCount) {
consecutiveCharacterCounts.put(startingCharacter, consecutiveCharacterCount);
}
}
}

return con

Avoid unnecessary math

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

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

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

But we can also simplify the end.

for (int i = 1; i <= 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  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 > characterCount) {
          consecutiveCharacterCounts.put(startingCharacter, consecutiveCharacterCount);
        }
      }
    }

You can simplify this with a helper method:

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

Then we can change the bounds of the for loop.

for (int i = 1, n = chars.length - 1; i < 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.

StackExchange Code Review Q#162783, answer score: 2