patternjavaMinor
Which one is more readable for mutually exclusive but mandatory command line parameters validation?
Viewed 0 times
readablemutuallymandatorylineexclusivebutmorevalidationonefor
Problem
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
Option two
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
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"
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"
Solution
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
Context
StackExchange Code Review Q#19271, answer score: 3
Revisions (0)
No revisions yet.