CODEWITHSUNDEEP
c#
c830
c830

Reputation: 514

Reducing if statements when calling other functions

I have a function that calls a lot of other functions from different objects. Each function has to return true before calling the next one. As you can see I am using too many if statements. How can I improve the code and make it neater? 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;
 }

Upvotes: 4

Views: 692

Answers (11)

Eric Lippert
Eric Lippert

Reputation: 660032

My advice: do nothing to this code without a clear business case for making the change.

Your code is clear, obvious, likely correct, easy to maintain and easy to debug. Why on earth would you want to change it in any way? Spend your time adding value by fixing bugs and adding features, not by changing working code unnecessarily. When your boss asks you "so what did you do today?" the answer should not be "I increased our schedule risk to by making unnecessary cosmetic changes to correct, working, already-debugged code".

Now, if there really is a problem here, the problem is likely not that the code is hard to read, but rather that the code rigidly encodes what ought to be a user-configurable business process. In that case, create an object called "Workflow" that encodes the business process, and an engine which evaluates an arbitrary workflow. Then construct an instance of that object that represents the desired workflow based on input from the user.

That actually adds value for the user; the user cares not a bit whether you use nested "if" statements or not.

Upvotes: 8

CD Jorgensen
CD Jorgensen

Reputation: 1391

You can leverage the fact that C# does short-circuit evaluation:

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

Editing to fix syntax on CheckVersion's parameters

Upvotes: 2

Jeb
Jeb

Reputation: 3799

Perhaps?

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

etc.

A side note, you can test for example, if objB is null before calling a method on it in one statement (the code will break execution as soon as a condition has not been met, i.e. won't call the next condition) so you don't need lots of if(objB != null) type statements. E.g.:

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

Upvotes: 2

Matt Grande
Matt Grande

Reputation: 12157

It depends on how much you want to change. Perhaps instead of returning a bool from your sub-methods, you could 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 probably isn't the best name (since Is generally translates to "returns a bool" in my mind), but you get the picture.

Upvotes: 0

cHao
cHao

Reputation: 86506

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-circuiting of && is useful for a few conditions, but if you have "tons" of them, IMO that's way too much to try and stick in one statement.

A combination of the two might be useful, though. More useful still would be to condense some of these checks together into bigger chunks (but smaller than IsOKToDoSomething). For instance, check whether you have access to the database (whether it exists, whether you can log in to it, etc) all at once

Truth be told, the fact that you have so many objects to check hints at a design issue -- namely, you're trying to do too much at once, or you have a "god object" somewhere that has its little tentacles in every aspect of the system. You might want to look at fixing that.

Upvotes: 6

Andrey
Andrey

Reputation: 60065

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

Upvotes: 9

deltree
deltree

Reputation: 3824

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

From my perspective, when I see myself writing code like this, I've made a grave architectural mistake somewhere and I should really rethink the reason behind making this call. Perhaps you should take another look at your logic, rather than just your code. Sit down and draw some boxes and work a bit more in the design phase and you might find yourself building things very differently.

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

Upvotes: 1

Dennis Traub
Dennis Traub

Reputation: 51634

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

Upvotes: 2

Pablo Santa Cruz
Pablo Santa Cruz

Reputation: 181280

How about using and:

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

Upvotes: 1

D Stanley
D Stanley

Reputation: 152521

The && operator will short-circuit, so you can chain them like so:

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

    //tons of similar code as above
    ......... 

 }

Upvotes: 2

Dave
Dave

Reputation: 266

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

Upvotes: 5

Related Questions