CODEWITHSUNDEEP
c#if-statementconditional-statementsrefactoring
xdtTransform
xdtTransform

Reputation: 2057

Refactorisation of an if Block

In the following code I have 2 if block.
A, The first return the error when the object is null.
B, the second will try to send the object and if it' fails then return and error. It should be executed only if not A, the object is not null.

private bool SendToERP(Foo foo, out string error)
{
    error = "";
    var ticketIn = TicketFromFoo(foo, out string err);

    if(ticketIn == null)
    {
        error = err;
        return false;
    }

    if ( !GenericERP_TicketSubmit(ticketIn, out err))
    {
        error = err;
        return false;
    }

    return true;
}

The action after those condition are the same and I want to refactor on into a unique if block.

As I can't warp my head 2 condition and an and I wrote a simple truth table to help me. But it didn't help me mutch.

Upvotes: 1

Views: 112

Answers (3)

D-Shih
D-Shih

Reputation: 46219

In your question you can combine two IF as @Thierry V answer

if(ticketIn == null || !GenericERP_TicketSubmit(ticketIn, out err))

but We can use another angle to read the question. This function expects to return bool so we can only write in a statement instead of IF.

return !(ticketIn == null || !GenericERP_TicketSubmit(ticketIn, out err))

We can use another skill (De Morgan's laws) let ! into the statement, that will reverse all logic, let the code more clear.

1.ticketIn == null will be ticketIn != null

2.|| will be &&

3.!GenericERP_TicketSubmit(ticketIn, out err) will be GenericERP_TicketSubmit(ticketIn, out err)

so we can get 

return ticketIn != null && GenericERP_TicketSubmit(ticketIn, out err)

the code can use like.

private bool SendToERP(Foo foo, out string error)
{
    error = "";

    var ticketIn = TicketFromFoo(foo, out error);

    return ticketIn != null && GenericERP_TicketSubmit(ticketIn, out error);
}

Upvotes: 4

Che
Che

Reputation: 169

I'd suggest to look up at c# 7 with new Value Tuples. We could also refactor all code on this way:

    private (bool result, string error) SendToERP(Foo foo)
    {
        var result = TryMakeTicketFromFoo(foo, out TicketIn ticketIn);
        return result.isSuccess ? GenericERP_TicketSubmit(ticketIn) : result;
    }

You need to refactor semantic of your other method also:

    private (bool isSuccess, string error) GenericERP_TicketSubmit(TicketIn ticketIn)
    {
        throw new NotImplementedException();
    }

    private (bool isSuccess, string error) TryMakeTicketFromFoo(Foo foo, out TicketIn ticketIn)
    {
        throw new NotImplementedException();
    }

Upvotes: 1

Antoine V
Antoine V

Reputation: 7204

The && and || operators short-circuit. It is considered from left to right. That means:

1) If && evaluates its first operand as false, it does not evaluate its second operand.

2) If || evaluates its first operand as true, it does not evaluate its second operand.

In your case, if ticketIn is null, you don't want the TicketSubmit executed. So, you can group 2 conditions by an OR. Like this.

    var ticketIn = TicketFromFoo(foo, out string err);        
    if(ticketIn == null || !GenericERP_TicketSubmit(ticketIn, out err))
    {
         error = err;
         return false;
    }
    return true;

Upvotes: 8

Related Questions