debugjavaModerate
Scrubbing user input
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
-
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.
-
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.
-
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.