Length of the longest sequence of repeating characters

I have been going through some CodingBat exercises and I came across this problem. "Given a string, return the length of the largest "block" in the string. A block is a run of adjacent chars that are the same."

Required output:

maxBlock("hoopla") → 2
maxBlock("abbCCCddBBBxx") → 3
maxBlock("") → 0

My code seems to pass all the tests except for the last "other test". Can someone please review my code and tell me where exactly I could have gone wrong?

public int maxBlock(String str) {
      int charBlock = 0;
      int holder = 1;
      if(str.length() == 0){ //If string is empty return 0
         charBlock = 0;
      } else if(str.length() == 1){ //If string contains only a single char return 1
         charBlock = 1;
      } else {
          for(int i=0; i  charBlock){
                     charBlock = holder; //update the value of charBlock if a holder is larger current value
                     }
             } else holder = 1; 
          }
      }
      return charBlock;
    }

Example test cases:

Expected Run
maxBlock("hoopla") → 2 2 OK
maxBlock("abbCCCddBBBxx") → 3 3 OK
maxBlock("") → 0 0 OK
maxBlock("xyz") → 1 1 OK
maxBlock("xxyz") → 2 2 OK
maxBlock("xyzz") → 2 2 OK
maxBlock("abbbcbbbxbbbx") → 3 3 OK
maxBlock("XXBBBbbxx") → 3 3 OK
maxBlock("XXBBBBbbxx") → 4 4 OK
maxBlock("XXBBBbbxxXXXX") → 4 4 OK
maxBlock("XX2222BBBbbXX2222") → 4 4 OK
other tests X


The missed edge case

The test case you're missing is longer text with non-repeating letters, for example: abcdefghijkl. And it's easy to fix that: just add these lines before the return statement:

if (holder > charBlock) {
        charBlock = holder;
    }

The biggest clue for figuring out this problem was around these lines:

if((str.length() == 2) && (str.charAt(i) != str.charAt(i+1))){
    charBlock =1; //return 1 if the length of the string is 2 and non of the two chars match
}
else if((str.length() == 3) && (str.charAt(i) != str.charAt(i+1))){
    charBlock = 1; //return 1 if the length of the string is 3 and non of the three chars match
}
else if (str.charAt(i) == str.charAt(i+1)){

That is, the special treatment for length 2 and 3. There doesn't seem to be a logical reason to treat these cases any special, and you could have quessed that this might break for longer strings. And as it turns out, these conditions are completely unnecessary: you can safely remove them, and the code will still pass the tests.

Code review

About this piece:

int charBlock = 0;
  int holder = 1;
  if(str.length() == 0){ //If string is empty return 0
     charBlock = 0;
  } else if(str.length() == 1){ //If string contains only a single char return 1
     charBlock = 1;
  } else {
     // ...
  }
  return charBlock;

Two things:

• It's good to use early returns to reduce the indent level and make the code more readable

• It's good to limit variables to the smallest scope possible

These two ideas go hand in hand in this example. Consider this alternative:

if (str.length() == 0) {
        return 0;
    }
    if (str.length() == 1) {
        return 1;
    }
    int charBlock = 0;
    int holder = 1;
    // ...
    return charBlock;

The guard statements don't need the charBlock or holder variable,
they can simply return,
and now those variables are declared where they are really needed.

When checking if a string is empty, use str.isEmpty() instead of checking on the length.

And in this program, you don't actually need the str.length() == 1 check,
if you omit it the program will still work and pass the tests.

But the single biggest problem with this code, in my opinion,
is the naming of the variables charBlock and holder.
I would name them longest and length, respectively.

Putting the above suggestions together, the implementation becomes this:

if (str.isEmpty()) {
        return 0;
    }
    int longest = 0;
    int length = 1;
    for (int i = 0; i  longest) {
                longest = length;
            }
        } else {
            length = 1;
        }
    }
    if (length > longest) {
        longest = length;
    }
    return longest;

Almost. There is one more important improvement to do: why update the longest length while still counting the same letters? It would be more efficient to move that piece of code to the else block, like this:

if (str.charAt(i) == str.charAt(i + 1)) {
            ++length;
        } else {
            if (length > longest) {
                longest = length;
            }
            length = 1;
        }

Unit testing

When refactoring non-trivial code like this,
it's good to have unit tests to make it easy to re-validate everything, for example:

@Test
public void test_hoopla() {
    assertEquals(2, maxBlock("hoopla"));
}

@Test
public void test_abbCCCddBBBxx() {
    assertEquals(3, maxBlock("abbCCCddBBBxx"));
}

@Test
public void test_empty() {
    assertEquals(0, maxBlock(""));
}

@Test
public void test_xyz() {
    assertEquals(1, maxBlock("xyz"));
}

@Test
public void test_xxyz() {
    assertEquals(2, maxBlock("xxyz"));
}

@Test
public void test_longer_nonrepeating_sequence() {
    assertEquals(1, maxBlock("abcdefghijkl"));
}

Especially when the contest provides the cases you should pass.
Heck, it's good to write unit tests always.

StackExchange Code Review Q#72173, answer score: 9