ansaurus

Question

c# is this design "Correct"?

Answer 1

+1 A:

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

Unless it is real inefficient.

astander 2010-10-15 13:24:34

+1 This joins the "keep it simple" expression! =)

Will Marcouiller 2010-10-15 13:57:21

Answer 2

+3 A:

Is the first code a set of OR statements?

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

Mark Dickinson 2010-10-15 13:24:37

If you do it as OR statements, you get the efficiency of returning as soon as any of the conditions are true, rather than waiting to see if they all are.

Mark Dickinson 2010-10-15 13:27:21

Ani 2010-10-15 13:30:45

+1 for performance and for correcting the OP's second solution.

BrunoLM 2010-10-15 13:37:01

How is it short circuiting if it has to evaluate all the conditions (in and and). Obviously this depends if you are executing or exiting early. I once worked with a team who said that there is no logic in OR and made all their logic with AND, and NOT. Is this preference, way of working, perhaps a regional thing, because these guys were off shore and were quite dismissive of OR logic?

Mark Dickinson 2010-10-15 13:38:36

@BrunoLM: This is the same as the OP's second solution. The only difference is that the contents of the if and else blocks are reversed (think De Morgan's law).

Brian 2010-10-15 13:38:44

@Mark Dickinson: It is shortcircuiting because when c# sees `!foo() || !bar()`, it omits executing `bar()` if `foo()` is false, since that makes `!foo()` true and this is sufficient information to determine that the entire condition is true (since true || X is true regardless of the value of X).

Brian 2010-10-15 13:40:25

BrunoLM 2010-10-15 13:42:54

SwDevMan81 2010-10-15 13:47:07

@Brian, yes I understand this but the shortcircuiting works backwards if the whole conditional block is used to exit a loop or a method, rather than run some code. I don't think it is the same, if any of the conditions in mine are true, it shorts. If any of the OPs conditions are false it shorts. Is that really the same thing?

Mark Dickinson 2010-10-15 13:49:40

Oh, I didn't notice.

BrunoLM 2010-10-15 13:49:51

SwDevMan81 2010-10-15 13:57:19

Brian 2010-10-15 14:02:58

@SwDevMan81, yeah I think we've covered the bases. Just wanted to make people aware that the choice of where you short circuit depends on the wider codebase. :)

Mark Dickinson 2010-10-15 14:06:37

it's just common sense, really

Nico 2010-10-15 15:36:42

Answer 3

+2 A:

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....

Preet Sangha 2010-10-15 13:25:33

This is a nice approach, but still not very debug friendly. Especially when you are filling the list with lambda's all over the place. Normal methods should be ok though.

leppie 2010-10-15 13:34:34

agreed on the debug comment. I'm very data driven in my code but it makes it more unit test friendly. Also I tend to use logging more that debugging for analysis these days

Preet Sangha 2010-10-15 13:37:02

This sort of stuff debugs much better in vs2010 :)

Mark Dickinson 2010-10-15 14:08:20

Answer 4

+3 A:

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.

Ani 2010-10-15 13:46:16

what do you mean by "side-effects"?

Nico 2010-10-15 13:48:43

http://en.wikipedia.org/wiki/Side_effect_(computer_science). Effectively, what I mean in this case is that the choice of short-circuiting vs. not short-circuiting introduces an observable, functional difference in the state of the program.

Ani 2010-10-15 13:49:53

+1 For providing a good rule for deciding between which statement will be more readable. Eric Lippert has a post on SO somewhere discussing this rule in the context of the ternary operator, but I can't seem to find it.

Brian 2010-10-15 13:59:32

Eric Lippert's discussion, which is actually only peripherally relevant: http://stackoverflow.com/questions/3909849/what-is-the-point-of-using-ternary-operators-instead-of-if-then/3909875#3909875

Brian 2010-10-15 15:40:26

Answer 5

A:

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...
     }
  }
}

Mantisen 2010-10-15 15:00:01

huh, I'm clearly already doing that?

Nico 2010-10-15 15:35:22

Mantisen 2010-10-17 14:44:03

ansaurus

tags:

views:

answers:

c# is this design "Correct"?

related questions