Adding order actions like "Print" or "RePrint" The eventual desire to implement code that's clean and extendable. I always fear that I'm overusing If's and or not seeing the appropriate pattern to reduce conditionals.

Now I love architecture, micro service's etc I even enjoy IoC, ISP(SOLID) But when it comes down to small encapsulated methods, the nuances of reducing If's , I struggle to grasp.

I know I've instantiated concrete types and coupled the responsibility(May have just answered my own question). The reality is, I want to minimize the use of If's.

Could I kindly request a Code Review on the below snippet?

public async Task AddOrderActionAsync(Identity identity, 
    string value, 
    string order_number)
{
    if (!string.IsNullOrWhiteSpace(value))
    {
        var client = await _context.tbl_Client.FirstOrDefaultAsync(c =&gt;
                            c.ClientKey == identity.ClientKey);
        try
        {
            // See if order action exists, if it does it could contain multiple
            // so delete all and re-add one
            var orderAction = _context.tbl_OrderActions.Where(o =&gt; 
            o.tbl_Client.ClientKey == identity.ClientKey &amp;&amp;
            o.OrderNumber == order_number).ToList();

            if (orderAction != null &amp;&amp; 
                orderAction.Count() &gt; 0)
            {
                _context.tbl_OrderActions.RemoveRange(orderAction);
               await AddOrderActionAsync(_context, identity,
                                       value, order_number);
            }
            else
            {
                await AddOrderActionAsync(_context, identity, value, order_number);
            }

            await _context.SaveChangesAsync();
        }
        catch (AddOrderActionException ex)
        {
            new Email().SendEmail(identity.ClientName,
                    "Add Order Action Error - Data API",
                    "Add Order Action Error(" + value + " - " + order_number + "): " + ex.Message,
                    "error@retain.me");

            //System needs to run regardless if this fails
            return;
        }
    }
}


The above code is adding an "OrderAction" which could be "Print" or "RePrint", there is a possibility that multiple could be added else where in the pipeline or another app. The ideas is to remove the multiple and re-add one an/or just add.
There aren't too many ifs in this code. However, what you can reduce is indentation and it will help readability.

This code:

public async Task AddOrderActionAsync(...)
{
    if (!string.IsNullOrWhiteSpace(value))
    {
        // Everything here
    }
}


Can be rewritten as:

public async Task AddOrderActionAsync(...)
{
    if (string.IsNullOrWhiteSpace(value))
        return;

    // Everything here
}


With the nice side-effect to also remove negation.



Looking into the code you check orderAction != null, it seems you try to be defensive here but it's misleading because it can't be null. Ever. Also orderAction.Count() &gt; 0 is equivalent to orderCount.Any() which is shorter, faster and communicates better the intent. Content of the else clause is repeated then one of them should be dropped:

if (orderAction.Any())
    _context.tbl_OrderActions.RemoveRange(orderAction);

await AddOrderActionAsync(_context, identity, value, order_number);


In your query you're using .ToList() but it's unnecessary and without it EF (or ORM you're using) might be able to optimize database calls (if not then you at least gain in clarity).



It's not a rule but an accepted convention in C# to do not use underscore to separate words, a variable named order_number should be orderNumber.



You're catching a specific exception, nice! However I think it can be thrown only by AddOrderActionAsync() then it should be the only part inside try/catch:

try
{
    await AddOrderActionAsync(_context, identity, value, order_number);
}
catch (AddOrderActionException ex)
{
    // ...
}




Really don't ignore t3chb0t's suggestion about dependency injection, sending e-mails like that it's definitely not something you want to happen inside your unit testing...