Refactoring nested if-else interface method in Java8

Question

I have the below default method in an interface and it seems to be pretty complex because of the many if-else conditions.

 default void validate() {
 Application application = application().get();
 if (StringUtils.isEmpty(application.name()) || application.data() == null) {
 throw new IllegalArgumentException());
 } else {
 for (Form form : application.data().forms) {
 if (StringUtils.isEmpty(form.getId())) {
 throw new IllegalArgumentException();
 }
 for (Question question : form.getQuestions()) {
 if (StringUtils.isEmpty(question.getQuestion())
 || StringUtils.isEmpty(question.getValue())) {
 throw new IllegalArgumentException();
 }
 }
 }
 }
 }

I want to refactor this into different methods to reduce the complexity. However since the project is configured to use Java8, private methods cannot be used in the interface. How else can I break this down and reduce the complexity? The reason for this being an interface is because I'm using Immutables library and Jackson Json to deserialize a Json string into a Java object. Therefore I'd like to keep this an interface. Any advice would be much appreciated.

• Alexander: It's hard to say without knowing all the types involved here, but it looks to me like ApplicationData (the type of the return value of application.data()) should itself have a validate() method, which in turn calls a validate() method on Question, and so on.
• gnat: Does this answer your question? Proper way to refactor multiple if based conditions
• Andy: You do that by keeping the interface without implementation and providing and abstract class, which can then contain private methods so splitting up larger code a piece of cake.
• mtj: If you remove the first unnecessary else (as the if throws), you have a mere 17 lines of code which a maximum nesting depth of 3 (for-for-if). How much simpler does code need to be to fulfil your complexity requirements?
• Kilian Foth: Why are you using else after a throw at all?

Answer 1

It seems that your validation is complex enough in that you should have a separate Validator class. This could be a dependency injected anywhere were validation on forms is required. Currently, you tie validation of forms to a certain application(). Do you really need forms to come from a certain application()? See Data validation: separated class or not? for more specific pointers.

Note that this Validator class could be specific to each "data level" of a form (each for loop you have). For example, you could have a QuestionValidator, injected into a FormValidator. Then this FormValidator could be injected into an ApplicationValidator. Making Validator generic over whatever data it validates would allow each of these level-specific validators to subclass a parametrizeed Validator with the type of data they take. This massively improves test-ability, while also keeping components that don't need to be binded together separate.

From a code review standpoint, in terms of reducing complexity through readability:

1. You shouldn't return IllegalArgumentException. This usually implies illegally given arguments directly to a method, instead of in the underlying implementation's state. An IllegalStateException would be more relevant, or preferably a custom checked validation error (as shown in the last link).
2. You can use String.isEmpty instead of StringUtils.isEmpty if you don't need to check for null.
3. You can remove the outer else since the if body (throwing an exception) exits the method early anyways. See Best practice on if/return, it's related because both a return and throw would exit the method.