3
\$\begingroup\$

I obtain strings that contain chat messages, and they sometimes have formatting codes imbedded in them. A formatting code is a § followed by a character, and this character then defines how the code will affect the following text. Colour codes can occur at the very end of the String, a common case of a message would be:

"§r§aThis is a message.§r§bAnd this is blue...§r"

Currently I'm using this method, which simply iterates over the String and skips another character when it encounters a §. Is this a good solution, and how can I further improve it?

private String cleanFormattingCodes(String message) {
    StringBuilder newMessage = new StringBuilder();
    for (int i = 0; i < message.length(); i++) {
      char c = message.charAt(i);
      if (c == '§') {
        i++;
        continue;
      }

      newMessage.append(c);
    }
    return newMessage.toString();
  }
\$\endgroup\$

3 Answers 3

3
\$\begingroup\$

Quite straightforward code, no bells or whistles. Your implementation removes a lone § if it is at the end of the string (e.g. "this gets removed §"). Is it supposed to do that? I would rename newMessage to cleanMessage as the message is not new, just a cleaned one. And change the unmoddified variables to final (final StringBuilder cleanMessage and final char c).

That ends my code review. Now for the improvements.

The whole method can be replaced with a short regular expression.

return message.replaceAll("§.", "");

It is also possible to make a regex that matches "§ + letter" instead of "§ + any char" as both our implementations now do.

\$\endgroup\$
2
  • \$\begingroup\$ I think you have an error: the regular expression should be §.. \$\endgroup\$
    – RoToRa
    Sep 6, 2021 at 20:52
  • \$\begingroup\$ @RoToRa Thanks, my regex game is getting rusty... \$\endgroup\$ Sep 7, 2021 at 4:15
3
\$\begingroup\$

I have only three small points:

  1. The formatting/indentation isn't quite ok, but that may be just a copy and past error.

  2. Since you know the (maximum) length of the final string, you could initialize the StringBuilder with a capacity. That has the advantage, that the StringBuilder won't need to allocate new memory inbetween:

StringBuilder newMessage = new StringBuilder(message.length());
  1. I feel that the use of continue makes the loop a bit awkward to read. Since the rest of the loop body is just one line, I think a simple else would be better: (And a small comment wouldn't out of place.)
if (c == '§') {
  // Skip the following character
  i++;
} else {
  newMessage.append(c);
}
\$\endgroup\$
1
\$\begingroup\$

For me, this code breaks the rule of "least surprise" :

for (int i = 0; i < message.length(); i++) {
  char c = message.charAt(i);
  if (c == '§') {
    i++;
    continue;
  }

  newMessage.append(c);
}

The loop variable i is incremented in the loop control and in the body of the loop. That goes against my expectations of the behaviour of a for loop.

If your code is going to work this way, my preference would be for the loop to be a while loop, with the index explicitly managed in the different code paths - like this (untested code).

int i = 0;
while (i < message.length()) {
  char c = message.charAt(i);
  if (c == '§') {
    i += 2; // skip the 2-character formatting code
  } else {
    newMessage.append(c);
    i += 1 // move on to the next character.
  }
}

I've used "i += n" in both cases for symmetry.

As Torben has pointed out, you could just use String.replaceAll(regex)...

Torben's regex solution, like your manual copying, assumes that there is never any reason for your flag character to appear in the string other than as part of a formatting code. Can you confidently assert that that is the case? If, for example, the flag was allowed to appear as text, preceded perhaps by a quote character such as '\' your code breaks:

"I've learned how to remove \§ from strings"

would become

"I've learned how to remove \from strings"

If this sort of thing is possible, your code needs to be more subtle. I'd probably use a State Machine, but that's just a little foible of mine...

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.