Scrubbing user input

Question

I'm taking user input from a file in form (1, 2, 3) to create a color, and I just wanted to know if I was taking most cases into account, and if there is any way I could improve this method in terms of being robust and readable.

private Color getColorFromString(String rgb) throws MalformedColorException{
    rgb = rgb.replaceAll("[()\\s]", "");
    String[] colors = rgb.split(",");
    if(colors.length > 3){
        throw new MalformedColorException("Too many arguments");
    }else if (colors.length < 3){
        throw new MalformedColorException("Too little arguments");
    }
    try{
        int red = Integer.parseInt(colors[0]);
        int green = Integer.parseInt(colors[1]);
        int blue = Integer.parseInt(colors[2]);

        return new Color(red, green, blue);
    }catch(NumberFormatException e){
        throw new MalformedColorException("Malformed number", e);
    }
}

Answer 1

I only have a few minor suggestions, but none are hard-line.

• You have a space between the else if and open parenthesis. I don't like this style of removing spaces around keywords and from between ) and { as it becomes hard to read without syntax highlighting (even with it).

• Instead of too few/many arguments which isn't very specific, have a single exception that says how many were provided and how many are required.

  if (colors.length != 3) {
      throw new MalformedColorException("Color requires 3 values, got " + colors.length);
  }
  

• Moving the individual value parsing to a separate method would allow for more checks (range, sign, etc) and allow specifying which color was incorrect in the exception.

  return new Color(
      parseComponent("red", colors[0]),
      parseComponent("green", colors[1]),
      parseComponent("blue", colors[2])
  );
  
  private int parseComponent(String name, String value) {
      try {
          int parsed = Integer.parseInt(value);
          if (parsed < 0 || parsed > 255) {
              throw new MalformedColorException(
                      name + " component \"" + value + "\" out of range");
          }
          return parsed;
      } catch (NumberFormatException e) {
          throw new MalformedColorException(
                  name + " component \"" + value + "\" must be an integer");
      }
  }
  

Answer 2

How strict do you want the checking to be? The initial algorithm accepts (1),(2, 3) as valid input, as well as 1,2,3)))))) etc.

You might want to consider changing your regex to make it more strict, as there are a lot more than just your example input (1, 2, 3) that gets accepted.

Answer 3

I see a few things that you could improve:

1. The argument rgb should be checked for possibility of being null, with preferably Objects.requireNonNull(rgb), which will throw a NullPointerException if the argument is null.
2. I would strongly recommend to leave one white space before and after every { and }.
3. I would only use one statement to check the length, colors.length != 3 and report back the given input, thus the rgb string.
4. The same holds for the last place where the exception is being thrown, I would echo back what rgb was.

Then one more additional thing about the try-catch onparsing the integers. I suppose you can take several views on this. You should only do it this way if it is an exceptional situation, which seems to be correct here. Yet, for performance reasons, you might want to simply check whether colors[0], etc. is a number and only then try to parse it.