Reputation: 16196

if/else, good design

Is it acceptable/good-style to simplify this function:

bool TryDo(Class1 obj, SomeEnum type)
{
    if (obj.CanDo(type))
    {
        return Do(obj);
    }
    else
    {
        return false;
    }
}

as:

bool TryDo(Class1 obj, SomeEnum type)
{
    return obj.CanDo(type) && Do(obj);
}

The second version is shorter but arguably less intuitive.

Upvotes: 19

Answers (20)

CodesInChaos

Reputation: 108800

IMO it's only OK if the second function has no side-effects. But since Do() has side-effects I'd go with the if.

My guideline is that an expression should not have side-effekts. When calling functions with a side-effect use a statement.
This guideline has a problem if a function returns a failurecode. In that case I accept assignment of that errorcode to a variable or directly returning it. But I don't use the return value in a complex expression. So perhaps I should say that only the outermost function call in an expression should have a side-effect.

See Eric Lippert's Blog for a longer explanation.

Upvotes: 4

Andrey

Reputation: 1248

I do not like this design, and maybe not for the obvious reason. What bothers me is. return Do(obj); To me it makes no sense for the Do function to have a bool return type. Is this a substitute for property pushing errors up? Most likely this function should be void or returning a complex object. The scenario should simply not come up. Also if a bool somehow makes sense now, it can easily stop making sense in the future. With your code change it would require more re factoring to fix

Upvotes: 6

Alex Martini

Reputation: 760

There are several good answers already, but I thought I would show one more example of (what I consider) good, readable code.

bool TryDo(Class1 obj, SomeEnum type)
{
    bool result = false;

    if (obj.CanDo(type))
    {
        result = Do(obj);
    }

    return result;
}

Keep or remove the curly brackets back around the body of the if statement according to taste.

I like this approach because it illustrates that the result is false unless something else happens, and I think it more clearly shows that Do() is doing something and returning a boolean, which TryDo() uses as its return value.

Upvotes: 1

smirkingman

Reputation: 6358

Neither, because when TryDo returns False, you can't determine whether it was because 'Not CanDo' or 'Do returned False'.

I fully understand that you can ignore the result, but the way it's expressed implies that the result has meaning.

If the result is meaningless, the intent would be clearer with

void TryDo(Class1 obj, SomeEnum type)
{
    if (obj.CanDo(type))
        Do(obj);
    return;
}

If the result does have a meaning then there should be a way to differentiate between the two 'false' returns. I.E. What does 'If (!TryDo(x))' mean?

Edit: To put it another way, the OP's code is saying that 'I can't swim' is the same as 'I tried to swim and drowned'

Upvotes: 5

KevinDTimm

Reputation: 14376

What I would code is :

return obj.CanDo(type) ? Do(obj) : false;

Upvotes: 65

Svisstack

Reputation: 16616

Version with brackets:

bool TryDo(Class1 obj, SomeEnum type)
{
    if (obj.CanDo(type))
    {
        return Do(obj);
    }

    return false;
}

~~Or version without brackets (in answer comments is high debate about it):~~

bool TryDo(Class1 obj, SomeEnum type) { /* * If you want use this syntax of * "if", this doing this on self * responsibility, and i don't want * get down votes for this syntax, * because if I remove this from my * answer, i get down votes because many * peoples think brackets i wrong. * See comments for more information. */ if (obj.CanDo(type)) return Do(obj); return false; }

Your first code example is better, but I think my version is even better.

Your second version is not good readable and makes code harder to maintain, this is bad.

Upvotes: 25

Jamie Ide

Reputation: 49261

No. The second version

return (obj.CanDo(type) && Do(obj))

relies on the short-circuiting behavior of the && operator which is an optimization, not a method to control program flow. In my mind this is only slightly different than using exceptions for program flow (and almost as bad).

I hate clever code, it's a bitch to understand and debug. The goal of the function is "if we can do this, then do it and return the result else return false." The original code makes that meaning very clear.

Upvotes: 1

Pedro

Reputation: 382

The side-effect problem that some people are mentioning is bogus. No one should be surprised that a method named "Do" has a side-effect.

The fact is, you are calling two methods. Both of those methods have bool as a return value. Your second option is very clear and concise. (Though I would get rid of the outer parenthesis and you forgot an ending semi-colon.)

Upvotes: 3

TimC

Reputation: 1061

I think that the Class1 Type should determine if it can do, given a SomeEnum value.

I would leave the decision on whether or not it can handle the input for it to decide:

bool TryDo(Class1 obj, SomeEnum type)
{
    return obj.Do(type));    
}

Upvotes: 1

Travis

Reputation: 185

While it is certainly clear what the second code for a competent programmer, it would seem to me clearer and easier to read in a more general case to write the code like any other "if precondition satisfied, do action, else fail" style.

This could be achieved either by:

return obj.CanDo(type)? Do(obj) : false;

Or,

if(obj.CanDo(type)) return Do(obj);
return false;

I find this superior because this same style can be replicated no matter the return type. For example,

return (array.Length > 1)? array[0] : null;

Or,

return (curActivity != null)? curActivity.Operate() : 0;

The same style can also be expanded to situations that don't have a return value:

if(curSelection != null)
    curSelection.DoSomething();

My two cents.

Upvotes: 4

asdfjklqwer

Reputation: 3594

I don't like the second version as I'm not really a fan of taking advantage of the order of sub-expression evaluation in a conditional expression. It's placing an expected ordering on sub-expressions which in my mind, should have equal precedence (even though they don't).

At the same time, I find the first version a bit bloated, so I'd opt for the ternary solution which I consider highly readable.

Upvotes: 4

Jeff LaFay

Reputation: 13350

For me, I prefer the second method. Most of my methods that return bool are shortened in the same manner when simple conditional logic is involved.

Upvotes: 1

Radu

Reputation: 8699

I might get hate for this but what about:

if (obj.CanDo(type)) return Do(obj);
return false;

I don't like having braces for one liners.

Upvotes: 1

Konrad Rudolph

Reputation: 545588

Yes.

Especially with names similar to your chosen names, i.e. CanDoSomething and DoSomething it is absolutely clear to any competent programmer what the second code does: “if and only if the condition holds, do something and return the result”. “if and only if” is the core meaning of the short-circuited && operator.

The first code is convoluted and unnecessarily long without giving any more information than the second code.

But in general, the two conditions may not form such an intimate relationship (as in CanDo and Do) and it might be better to separate them logically because putting them in the same conditional might not make intuitive sense.

A lot of people here claim that the first version is “much clearer”. I’d really like to hear their arguments. I can’t think of any.

On the other hand, there’s this closely related (although not quite the same) code:

if (condition)
    return true;
else
    return false;

this should always be transformed to this:

return condition;

No exception. It’s concise and still more readable to someone who is competent in the language.

Upvotes: 13

Paulo Guedes

Reputation: 7259

The else is useless and the &&, however obvious, is not as readable as pure text.

I prefer the following:

bool TryDo(Class1 obj, SomeEnum type)
{
    if (obj.CanDo(type))
    {
        return Do(obj);
    }   
    return false;
}

Upvotes: 18

Kevin O'Donovan

Reputation: 1669

The shortened version hides the fact that Do does something. It looks like you're just doing a comparison and returning the result, but you're actually doing a comparison and performing an action, and it's not obvious that the code has this "side effect".

I think the core of the problem is that you're returning the result of an evaluation and the return code of an action. If you were returning the result of two evaluations in this way, I wouldn't have a problem with it

Upvotes: 12

Guffa

Reputation: 700322

Another alternative that may be a bit more readable is using the conditional operator:

bool TryDo(Class1 obj, SomeEnum type) {
  return obj.CanDo(type) ? Do(obj) : false;
}

Upvotes: 7

kaliatech

Reputation: 17867

The 1st version is much easier to read with less chance of being misunderstood, and I think that is important in real world code.

Upvotes: 6

Oded

Reputation: 499002

In this case, I would go with the first option - it is much more readable and the intention of the code is much clearer.

Upvotes: 5

Liviu Mandras

Reputation: 6617

Never do that. Keep it simple and intuitive.

Upvotes: 1

if/else, good design

Answers (20)

Related Questions