if/else, good design

Question

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.

Answer 1

What I would code is :

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

Answer 2

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.

Answer 3

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

Answer 4

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.

Answer 5

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

Answer 6

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

Answer 7

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

Answer 8

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

Answer 9

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

Answer 10

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'

Answer 11

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.