Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a small validation code that needs to make sure that exactly one of two parameter is set

I consider two options, the "Naive" that has some repetitions and might look "clumsy" but is very clear, and a second that is a bit more "DRY" but I'm not sure it's as readable / convincing in it's correctness.

Option one

if(param1==null && param2==null){
  throw new RuntimeException("Either param1 or param2 must be specified");
}

if(param1!=null && param2!=null){
  throw new RuntimeException("Only one of param1 or param2 must be specified");
}

if(param1!=null){
  //do something with param1
}
if(param2!=null){
  //do something with param2
}

Option two

if(param1!=null){
  if(param2!=null){
    //only runs if both are not null
    throw new RuntimeException("Only one of param1 or param2 must be specified");
  }
  //do something with param1
} else if(param2!=null){
  //do something with param2
} else{
  //only runs if both are null
  throw new RuntimeException("Either param1 or param2 must be specified");
}

Which one is more readable? which one would you keep? do you see any issue with Option 2?

Edit:

Based on feedback, I think there is a bit nicer option

Option three

boolean isParam1Valid = param1!=null;
boolean isParam2Valid = param2!=null;

if(isParam1Valid == isParam2Valid ){ //e.g. either both are null or both are not null
  throw new RuntimeException("param1 or param2 must be specified exactly once");
}

if(isParam1Valid){
  //do something with param1
}

if(isParam2Valid){
  //do something with param2
}

Option four shorter, but a bit less readable IMHO than option three (one needs to go up and read the code to understand why author is so sure that the else block means that param2 is valid

boolean isParam1Valid = param1!=null;
boolean isParam2Valid = param2!=null;

if(isParam1Valid == isParam2Valid ){ //e.g. either both are null or both are not null
  throw new RuntimeException("param1 or param2 must be specified exactly once");
}

if(isParam1Valid){
  //do something with param1
}else{ //must be true, as isParam1Valid is false, and they are not equal
  //do something with param2
}
share|improve this question
add comment

3 Answers

up vote 3 down vote accepted

I would put all validations first, and then extract to a method. Better, trying using a parsing framework like JCommander that might be able to handle that for you, so that your code is just business logic

share|improve this answer
add comment

Your option one is ok.
I would recommend to not use option two, this is complex and everyone has to think what is happening here and if everything is correct.

Two possible ways:

1) If used multiple times

boolean isParameter1Valid = ...;
boolean isParameter2Valid = ...;
checkIfExactlyOneIsTrue(isParameter1Valid, isParameter2Valid, "Exactly one parameter must be specified");
if (isParameter1Valid) {
    // do something with param1
}
if (isParameter2Valid) {
    // do something with param2
}

/**
 * throws an exception with given message if either both are true or both are false
 */
private static void checkIfExactlyOneIsTrue(boolean b1, boolean b2, String message) {
    if ((b1 == true && b2 == true) || (b1 == false && b2 == false))
        throw new IllegalArgumentException(message);
}

2) If used only once:

boolean isParameter1Valid = ...;
boolean isParameter2Valid = ...;
if (!(isParameter1Valid ^ isParameter2Valid)) { // ^ is xor, which is true if both arguments are different, otherwise false
    throw new IllegalArgumentException("Exactly one parameter must be specified");
}
if (isParameter1Valid) {
    // do something with param1
}
if (isParameter2Valid) {
    // do something with param2
}

You should not forget to add a comment for the xor, because it is probably not known that much by the typical java programmer.

Edit: as Eran Medan suggested, the typical programmer without xor knowledge could live better with a != or == approach, see his starting post.

share|improve this answer
    
Thanks, isn't it better to use != instead of XOR? see stackoverflow.com/questions/160697/…. e.g. the first condition of the 2nd option is logically equivalent to if(isParameter1Valid == isParameter2Valid), or am I missing something? (if both are invalid or both are valid, throw the exception, else, e.g. if exactly one is valid, continue) –  Eran Medan Dec 6 '12 at 0:44
    
Good point. For me, xor would be the cleaner way, because xor does exactly what i want in the way i want. For the == I had to think if it is correct or wrong. But I am perhaps not the typical one. So It could be a better approach to assume no knowledge about xor, then == or != is the better way, I agree. I have edited my post, too –  tb- Dec 6 '12 at 15:40
add comment

as you have only two tested parameters :

if (isParameter1Valid) {
    // do something with param1
} else {
    // do something with param2
}
share|improve this answer
    
Thanks, yes, obviously, you are right, updated option 3. However some might say that having two explicit ifs is more readable, as the code above relies on the fact that isParam1Valid!=isParam2Valid –  Eran Medan Dec 6 '12 at 14:54
    
@EranMedan You do not have to take care of others reading in the coding phasis, the code have to do obvious, clean, light for an expert (I cannot make better). But, if you think that some people are more easy with two if, you have to add comments for them, not complicate (or penalize) your code. –  cl-r Dec 7 '12 at 7:31
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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