HiveBrain v1.2.0
Get Started
← Back to all entries
debugjavaModerate

Scrubbing user input

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
inputuserscrubbing

Problem

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);
    }
}

Solution

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  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");
    }
}

Code Snippets

if (colors.length != 3) {
    throw new MalformedColorException("Color requires 3 values, got " + colors.length);
}
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");
    }
}

Context

StackExchange Code Review Q#51642, answer score: 10

Revisions (0)

No revisions yet.