Reduction if statements when calling other functions

Question

I have a function that calls many other functions from different objects. Each function must return true before calling the next. As you can see, I use too many if statements. How can I improve the code and make it more accurate? Thanks

bool ISOKToDoSomthing() { boo retVal = false; retVal = ObjA.CheckVersion(oldVersion); if(retVal) { retVal = objB.CheckUserRight(); } if(retVal) { retVal = ObjC.ISDBExist(); } if(retVal) { retVal = OjbD.ISServerUp(ServerName); } //tons of similar code as above ......... return retVal; } 

Answer 1

 return ObjA.CheckVersion(oldVersion) && objB.CheckUserRight() && ObjC.ISDBExist() && OjbD.ISServerUp(ServerName); 

Answer 2

My advice: do nothing for this code without a clear business code to make changes.

Your code is clear, obvious, probably correct, easy to maintain, and easy to debug. Why don't you change it in any way ? Spend your time adding value by fixing bugs and adding features, rather than changing your working code unnecessarily. When your boss asks you, "So what did you do today?" the answer should not be "I increased the risk of the schedule by making unnecessary cosmetic changes to fix, work, already debugged code."

Now, if the problem really exists, the problem is probably not that the code is hard to read, but rather that the code is hard-coded for what should be a user-configurable business process. In this case, create an object called "Workflow" that encodes the business process, and an engine that evaluates an arbitrary workflow. Then create an instance of this object that represents the desired workflow based on input from the user.

This actually adds value to the user; the user cares a bit about whether you use nested if statements or not.

Answer 3

 if (!ObjA.CheckVersion(oldVersion)) return false; if (!ObjB.CheckUserRight()) return false; if (!ObjC.IsDBExist()) return false; if (!ObjD.IsServerUp(serverName)) return false; ... your other checks ... return true; 

The && short circuit is useful for several conditions, but if you have tons of them, the IMO is too much to try to stick to one statement.

A combination of the two may be useful. It is even more useful to condense some of these checks together into larger chunks (but less IsOKToDoSomething ). For example, check if you have access to the database (regardless of whether it exists, whether you can enter it, etc.) at the same time

In truth, the fact that you have so many objects to check for hints of a design problem, namely, you are trying to do too much at once or you have a “god object” somewhere that has few tentacles in all aspects system. You might want to look at this.

Answer 4

 return ObjA.CheckVersion(oldVersion) && objB.CheckUserRight() && ObjC.ISDBExist() && OjbD.ISServerUp(ServerName) 

Answer 5

May be,?

  retVal = objB.CheckUserRight() && ObjC.ISDBExist() && OjbD.ISServerUp(ServerName); 

and etc.

Note that you can test, for example, if objB is null before calling the method on it in one expression (the code will violate execution as soon as the condition is not fulfilled, i.e. will not call the next condition), so you do not need to many operators of type (objB! = null). For instance:.

 retVal = (objB != null && objB.CheckUserRight()) && (ObjC != null && ObjC.ISDBExist()) && (OjbD != null && OjbD.ISServerUp(ServerName)); 

Answer 6

& & the operator will close, so you can chain them as follows:

 bool ISOKToDoSomthing() { return ObjA.CheckVersion(string oldVersion) && objB.CheckUserRight() && ObjC.ISDBExist() && OjbD.ISServerUp(ServerName) && //tons of similar code as above ......... } 

Answer 7

You can use the fact that C # performs a short circuit assessment:

 return ObjA.CheckVersion(oldVersion) && objB.CheckUserRight() && ObjC.ISDBExist() && OjbD.ISServerUp(ServerName); 

Editing to correct CheckVersion parameter syntax

Answer 8

 bool ISOKToDoSomthing() { return ObjA.CheckVersion(string oldVersion) && ObjB.CheckUserRight() && ObjC.ISDBExist() && OjbD.ISServerUp(ServerName); } 

Answer 9

How about using and :

 retVal = ObjA.CheckVersion(oldVersion) && objB.CheckUserRight() && ObjC.ISDBExist() && OjbD.ISServerUp(ServerName); return retval; 

Answer 10

To make the code less verbose, you can try a while loop. Given that your method here is to never change the value of your original value, if it / ever / becomes false, then it will be while (retval) {} and iterate over the list of actions. Personally, I think this is ugly. Consider using a switch or even (yuck on this, but it will work) a bitwise enumeration.

From my point of view, when I see that I am writing code this way, I made a serious architectural mistake somewhere, and I really have to rethink the reason for this call. Perhaps you should take a look at your logic, not your code. Sit down and draw some drawers and do some work at the design stage, and you can build things differently by yourself.

edit: or yes, like everyone else, you can make your iteration a single if statement. Again, this is a bigger problem than a long list of gates.

Answer 11

It depends on how much you want to change. Perhaps instead of returning a bool from your sub-methods, you can throw an exception.

 bool retVal = true; try { ObjA.CheckVersion(oldVersion); objB.CheckUserRight(); ObjC.ISDBExist(); OjbD.ISServerUp(ServerName); } catch (SomeException ex) { // Log ex here. retVal = false; } return retVal; 

If you do something like this, IsDBExist is probably not the best name (since Is usually translates to "returns bool"), but you get an image.