CODEWITHSUNDEEP
c#if-statementreturn
bevacqua
bevacqua

Reputation: 48566

c# is this design "Correct"?

I currently have the following

        if (!RunCommand(LogonAsAServiceCommand))
            return;

        if (!ServicesRunningOrStart())
            return;

        if (!ServicesStoppedOrHalt())
            return;

        if (!BashCommand(CreateRuntimeBashCommand))
            return;

        if (!ServicesStoppedOrHalt())
            return;

        if (!BashCommand(BootstrapDataBashCommand))
            return;

        if (!ServicesRunningOrStart())
            return;

would it be cleaner to do this? is it safe?

        if (
           (RunCommand(LogonAsAServiceCommand))
        && (ServicesRunningOrStart())
        && (ServicesStoppedOrHalt())
        && (BashCommand(CreateRuntimeBashCommand))
        && (ServicesStoppedOrHalt())
        && (BashCommand(BootstrapDataBashCommand))
        && (ServicesRunningOrStart())
        )
        {
               // code after "return statements" here
        }

Upvotes: 6

Views: 275

Answers (5)

Ani
Ani

Reputation: 113472

When I look at the first approach, my first thought is that the reason the code has been written in that way is that the operations involved probably have side-effects. From the names of the methods, I assume they do? If so, I would definitely go with the first approach and not the second; I think readers of your code would find it very confusing to see a short-circuiting && expression being used to conditionally execute side-effects.

If there are no side-effects, either will do; they are both perfectly readable. If you find that there are several more conditions or the conditions themselves vary in different scenarios, you could try something like:

Func<bool>[] conditions = ...
if(conditions.Any(condn => condn()))
{
   ...
}

I wouldn't really go with such an approach in your case, however.

Upvotes: 4

Mantisen
Mantisen

Reputation: 365

If you're using this code multiple times I would suggest you put it in a separate method such as:

public static class HelperClass
{
  public static bool ServicesRunning()
  {
    // Your code here... 
  }
}

and calling it when needed

public class SomeClass
{
  // Constructors etc...
  public void SomeMethod()
  {
     if(HelperClass.ServicesRunning())
     {
        // Your code here...
     }
  }
}

Upvotes: -1

Adriaan Stander
Adriaan Stander

Reputation: 166576

You should stick to whatever is more readable, and understandable.

Unless it is real inefficient.

Upvotes: 1

Preet Sangha
Preet Sangha

Reputation: 65555

Either is fine. However from a style perspective I'd suggest that since this is a series of comands you could use a List<Action> to hold these and make the function data driven.

(new Action[] {  func1, func2, .... }).ToList().ForEach(a => a());

Of course since the functions have differing signatures you may have to do multiple ones...

again it's a style matter....

Upvotes: 2

Mark Dickinson
Mark Dickinson

Reputation: 6633

Is the first code a set of OR statements?

if ( 
       (!RunCommand(LogonAsAServiceCommand)) 
    || (!ServicesRunningOrStart()) 
    || (!ServicesStoppedOrHalt()) 
    || (!BashCommand(CreateRuntimeBashCommand)) 
    || (!ServicesStoppedOrHalt()) 
    || (!BashCommand(BootstrapDataBashCommand)) 
    || (!ServicesRunningOrStart())

Upvotes: 3

Related Questions