Swap Elements in Array if first characters match but only once per first character

Question

This is my solution to CodingBat problem p150113. My Java knowledge is from 2011 and I'd like to know if this is still the optimal way to approach such a problem. and if my code is clean and understandable.

We'll say that 2 strings "match" if they are non-empty and their first chars are the same. Loop over and then return the given array of non-empty strings as follows: if a string matches an earlier string in the array, swap the 2 strings in the array. A particular first char can only cause 1 swap, so once a char has caused a swap, its later swaps are disabled. Using a map, this can be solved making just one pass over the array.

firstSwap(["ab", "ac"]) → ["ac", "ab"]
firstSwap(["ax", "bx", "cx", "cy", "by", "ay", "aaa", "azz"]) → ["ay", "by", "cy", "cx", "bx", "ax", "aaa", "azz"]
firstSwap(["ax", "bx", "ay", "by", "ai", "aj", "bx", "by"]) → ["ay", "by", "ax", "bx", "ai", "aj", "bx", "by"]

public String[] firstSwap(String[] strings) {
  HashMap<String,Integer> hashMap = new HashMap<>();
  int counter = 0;
  int index = 0;
  for (String s : strings){
    String firstChar = s.substring(0,1);
    if(hashMap.containsKey(firstChar)){
      // make the switch in the array
      if(hashMap.get(firstChar) > -1){
        index = hashMap.get(firstChar);
        String temp = strings[counter];
        strings[counter] = strings[index];
        strings[index] = temp;
        // make sure it doesn't get swapped again:
        hashMap.put(firstChar,-1);
      }
    } else {
      hashMap.put(firstChar,counter);
    }

    counter = counter + 1;
  }
  return strings;
}

Answer 1

Some thoughts:

Use Interfaces when the implementation type does not matter. In your code, the fact that your Map is a HashMap is irrelevant, so prefer the Map interface.

-1 is a magic number. Use a constant to document what it's doing.

Use whitespace consistently. Just like for statements, if statements should have a blank space between the if and the (. There should also be whitespace before a { and after a , for readability.

Your comments are not contributing enough to offset the visual distraction they provide.

Your map's key is a string of length 1. It should probably be a character. And you definitely shouldn't label it a character in your variable names unless it is one.

Use final on variables which will not be reassigned as a statement of intent.

A for loop would more tightly constrain your indexing variable, which you're confusingly referring to as a counter.

You can use a guard clause to handle the simple case and then continue, rather than having a large nested if statement.

You're looking up firstChar three times. I agree that containsKey is clearer to read than a null check, but there's no reason to duplicate the other call. And you can use another constant to make clear what case you're trying to handle there too.

Note that you're destructively modifying the incoming array. In toy problems that's not a big deal, but if you're writing real code this is a very bad practice. It would be preferable to make a copy of the array and modify that instead.

If you were to apply all these changes, your code might look more like:

private static final Integer ALREADY_SWAPPED = -1;
private static final Integer FIRST_OCCURRENCE = null;

public String[] firstSwap(final String[] strings) {
    final Map<Character, Integer> map = new HashMap<>();
    final String[] result = Arrays.copyOf(strings, strings.length);

    for (int i = 0; i < result.length; i++) {
        final char firstCharacter = result[i].charAt(0);
        final Integer priorIndex = map.get(firstCharacter);

        if (priorIndex == FIRST_OCCURRENCE) {
            map.put(firstCharacter, i);
            continue;
        }

        if (priorIndex == ALREADY_SWAPPED) {
            continue;
        }

        final String temp = result[i];
        result[i] = result[priorIndex];
        result[priorIndex] = temp;
        map.put(firstCharacter, ALREADY_SWAPPED);
    }

    return result;
}