3

It's hard to find a consensus, although many people say you should not use exceptions to handle bad user input. Still, I'm not convinced that it's the bad thing to do in my specific case. Could you try to explain why I'm wrong?

My code (please focus just on the exception handling aspect) follows. My rational for using exceptions here is that if I didn't, assuming I would want to keep the validation logic close to the keyword parsing (because parsing and validation are tightly coupled), I would have to change three methods (submitOnAdd, submitOnUpdate, getKeywords) to make them handle this exceptional situation. Do you think I definitely wrong in this case to use exceptions, or is it a matter of personal style?

public SubmitResponse internalSubmit(Map<String, String[]> submitParameters) {
  try {
      if (!submitParameters.containsKey("foo")) {
        return submitOnAdd(submitParameters);
      } else {
        return submitOnModify(submitParameters);
      }
  } catch (SubmitErrorException e) {
      return SubmitResponse.fieldError(Arrays.asList(e.getSubmitError()));
  }
}

SubmitResponse submitOnAdd(Map<String, String[]> submitParamters) {
  // do some stuff
  // ...
  if (addKeywordList(createKeywordList(submitParameters.get("concatenated_keywords"))
    return SubmitResponse.OK();
  return SubmitResponse.bad("Failed to add");
}

SubmitResponse submitOnUpdate(Map<String, String[]> submitParamters) {
  // do some other stuff
  // ...
  if (updateKeywordList(createKeywordList(submitParameters.get("concatenated_keywords"))
    return SubmitResponse.OK();
  return SubmitResponse.bad("Failed to update");
}

List<Keyword> getKeywords(String concatenatedKeywords) {
  List<String> rawKeywords = splitKeywords(concatenatedKeywords);
  return Collections.transform(new Function<String, Keyword>() {
      @Override
      public KeywordListProto.Keyword apply(String expression) {
        return buildKeyword(expression);
      }
    });
}

private Keyword buildKeyword(String rawKeyword) {
  // parse the raw keyword
  if (/*parsing failed */)
    throw new SubmitResponseException("Failed to parse keyword " + rawKeyword);

  return parsedKeyword;
}
Community
  • 1
  • 1
ripper234
  • 222,824
  • 274
  • 634
  • 905

1 Answers1

1

I can't say I would never advise to use Exceptions somewhere in input validation. But in this case, I would say it adds to much confusion. I would either:

  • Add a separate method to handle the validation. (Might have to call this method in several places which is a negative, but it might make the code easier to understand).
  • In the more ideal case, I would validate closer to the user input and not allow a submit of invalid data. (A possible negative is separation of validation and parsing logic, but if you could somehow use the same class to do both this could be avoided).
jzd
  • 23,473
  • 9
  • 54
  • 76
  • Actually we have a separate method for user validation, but the reason I opted not to put my validation code there is because it's much more convenient validating every expression by itself, and the splitting to expressions is only done in the parsing code. I like validation close to parsing because it's effectively the same thing in this case. I could use the same class, but it's still more complicated than doing it inline. I actually would want to take the try...catch logic to a base class, and thus keep the main internalSubmit() method pristine. – ripper234 May 25 '11 at 15:09
  • @ripper, it is a tough decision that I have been faced with before. My answer indicates my preference, but I don't think I would refactor existing code, therefore I would not have much of an issue with another developer making a difference preference. – jzd May 25 '11 at 15:17