In If else branches, what is the preferred approach?

Question

I am writing a validation function, which checks a multitude of conditions and returns Success if none of the checks fail. I want to know, what would be the preferred way among two choices and why ?

private ResponseObj validationFunc1(ObjA objA, ObjB objB, ObjC objC){
ResponseObj responseObj = new ResponseObj();
if(condA){
   responseObj.setStatus("FAILURE");
   responseObj.setMessage("condA message");
   return responseObj;
} else if(condB){
   responseObj.setStatus("FAILURE");
   responseObj.setMessage("condB message");
   return responseObj;
} ....
...
}else if(condZ){
   responseObj.setStatus("FAILURE");
   responseObj.setMessage("condZ message");
   return responseObj;
}
responseObj.setStatus("SUCCESS");
responseObj.setMessage("Valid");
return responseObj;
}


private ResponseObj validationFunc2(ObjA objA, ObjB objB, ObjC objC){
if(condA){
   return new ResponseObj("FAILURE","condA message");
} else if(condB){
   return new ResponseObj("FAILURE","condB message");
} ....
...
}else if(condZ){
   return new ResponseObj("FAILURE","condZ message");
}
return new ResponseObj("SUCCESS","Valid");
}

Which of the above 2 functions would be preferred in production code ? If so, does one method has a performance gain over another ? Or, Am I just mistaken and the compiled code for both the functions will be same ?

Thanks in advance for your answers. If I have asked the same question again, I am very sorry. I did try my best to search for this.

Answer 1

Which of the above 2 functions would be preferred in production code

Neither one.

Good code follows certain rules, especially for Java, nicely formulated in the "Clean Code" book by Robert Martin. Things that clean code strongly advocates against:

• high number of parameters (and 2 is consider high already ;-)
• doing multiple "things" within one method/class

So, at least for some people, (well written) production code would look much different.

What you could do for example: define an interface Validator, and then put each different validation into its own class implementing that interface. And then "multiple" checks turns into instantiating each of those classes, and putting the objects in some List<Validator> ... when you then iterate, and apply one after the other.

And note: performance doesn't matter here, too. The only thing that matters is how easy to read, understand, and maintain your code is. At least for me, the above ... isn't very good at that, for the reasons stated above.

Answer 2

Performance should be the same. You're making the same amount of comparisons and object creations in both cases.

Approach 1 is generally easier to trace later, especially in complicated functions. At the same time, something like

if (condition) {
  // lots of code
  // that scrolls and scrolls
  // with its own fors and ifs and whiles
} else {
  // by the time you're here, you've no idea what the corresponding if() was
}

can often be replaced with

if (!condition) { 
  return "error";
}

// lots and lots of code

and become more readable in the process. To summarize, there is not really a right answer, use your judgement and pick the variant you feel is easier to understand

Answer 3

The second example probably performs better because it has less method calls. However, it is so negligible that you really need to stop caring about it.

More importantly, the second example is more readable and less prone to bugs. For the first example, you have to worry about where responseObj came from and how corrupted its state may already be before you even get to it.

Answer 4

To review your sample.

1. Always prefer immutability, so your second approach would be better.
2. Returning early the code easy to follow so you don't have to scroll all the way down.
3. Returning a new ResponseObj("SUCCESS","Valid") is not a good structure cause it would open the gate to be (incorrectly) create a new ResponseObj("SUCCESS","Failure"). I'd suggest replacing it by an enum with both fields, which would also make the check for success easier downstream.
4. Keep it easy with the args as already pointed out

Answer 5

2nd approach is better, because in 1st approach code is tightly coupled and have repeated lines.

1) For tightly coupled: If the class provider ResponseObj is third person if they renamed the data members and their respective setters and getters then you also have to change where ever you have implemented.

2) Repeated lines of code: responseObj.setStatus("FAILURE"); there are so much same lines of code which we can see in every condition, and we dont know how many goes with number of conditions.

Solution: I feel 2nd Apparoach is better, however every thread should create a new object and constructors can be create any numbers as per the required(Overloading) without impacting to existing implemented developers.