Reduce code complexity by applying ternary operator

Question

I have a condition in my code where I need to put a lot of nested else if statements. However, I have managed to reduce some if cases by applying ternary operator, so that code looks nice and is readable. But still I am not satisfied with the result. I would like to remove rest of the if cases too and make the code better.

Can anyone suggest more optimization in this code block?

if (!string.IsNullOrEmpty(SessionValues.AccessToken))
    {
        eventstatuscode = "logged";
        EventStatus eventStatus = rb.GetEventStatus();
        if (eventStatus != null)
        {
            if (!string.IsNullOrEmpty(eventStatus.cancelDate))
            {
                eventstatuscode = DateTime.Parse(eventDetails.signUpDeadline) < DateTime.Now ? "expired": eventstatuscode;

            }

            else if (string.IsNullOrEmpty(eventStatus.cancelDate))
            {
                eventstatuscode = eventStatus.onWaitingList ? "waitinglist" : eventstatuscode;
                eventstatuscode = eventStatus.onWaitingList == false ? "accepted" : eventstatuscode;

            }
        }
        else
        {
            eventstatuscode = DateTime.Parse(eventDetails.signUpDeadline) < DateTime.Now ? "expired" : eventstatuscode;
        }
    }

Answer 1

Before deciding to use more ternary operators in your code, let's understand what if/else is about:

When the expression in the if clause is resolved to true, the block right after it is executed. If that expression is resolved to false, and there is an else block indicated, that block is executed.

An else if clause is needed if when the first condition is false there is still more than one execution option, based on a second(different) condition.

In your code you keep checking one condition, and then the opposite one, although this is totally redundant. if (x==false) is true then (x==true) is always false, and vice versa. Asking the same question and then the opposite is not only redundant, but also a maintenance risk, since you need to change the condition, you need to remember also changing the opposite one.

Also you should try to make all your conditions positive conditions - refrain from asking if(!string.IsNullOrEmpty(eventStatus.cancelDate)) - it is far better to switch the if and else blocks and ask if(string.IsNullOrEmpty(eventStatus.cancelDate)) - the ! operator ("not") is easy to miss, and may cause confusion. Of course, if only one block is implemented - no else block is better than no if block - leave the condition as it is.

To sum the above, a better code would look like this:

if (!string.IsNullOrEmpty(SessionValues.AccessToken))
{
    eventstatuscode = "logged";
    EventStatus eventStatus = rb.GetEventStatus();
    if (eventStatus != null)
    {
        if (string.IsNullOrEmpty(eventStatus.cancelDate))
        {
            eventstatuscode = eventStatus.onWaitingList ? "waitinglist" : "accepted";
        } else {
            eventstatuscode = DateTime.Parse(eventDetails.signUpDeadline) < DateTime.Now ? "expired": eventstatuscode;
        }
    }
    else
    {
        eventstatuscode = DateTime.Parse(eventDetails.signUpDeadline) < DateTime.Now ? "expired" : eventstatuscode;
    }
}

Now it is easier to see that the same line is indicated twice - the "expired" line. By combining the two if clauses - we can reduce that to only once (this is called being DRY):

if (!string.IsNullOrEmpty(SessionValues.AccessToken))
{
    eventstatuscode = null;
    EventStatus eventStatus = rb.GetEventStatus();
    if (eventStatus != null && string.IsNullOrEmpty(eventStatus.cancelDate))
    {
        eventstatuscode = eventStatus.onWaitingList ? "waitinglist" : "accepted";
    }
    else
    {
        eventstatuscode = DateTime.Parse(eventDetails.signUpDeadline) < DateTime.Now ? "expired" : "logged";
    }
}

Answer 2

Let's take a look at the input and output you have in that code segment:

Input:

• rb, or technically only the EventStatus of rb, which becomes eventStatus.
• eventDetails

Output:

• eventstatuscode

Perfect, we have a clear input and a clear output, let's make it a method!

Using a method will allow us to use multiple return statements to simplify your code.

private String GetStatusCode(EventStatus eventStatus, EventDetails eventDetails)
{
    if (eventStatus == null || !string.IsNullOrEmpty(eventStatus.cancelDate))
    {
        return DateTime.Parse(eventDetails.signUpDeadline) < DateTime.Now ? "expired" : "logged";
    }
    // no need to check else as we return directly in the above check.

    return eventStatus.onWaitingList ? "waitinglist" : "accepted";
}

I reduced these two lines to one line, as you check for true first and then you check for false. I believe this should not make things different in your current code.

Another very important suggestion though:

Make your eventstatuscode an enum

public enum EventStatusCode 
{
    Logged, Expired, WaitingList, Accepted;
}

As other questions on Code Review has proven, hard-coding strings can easily lead to mistakes. If you only misspell "waitinglist" somewhere, it will lead to bugs in your program. Using enums will allow such bugs to be caught by the compiler instead.

Answer 3

Following on from Uri's answer, which was ...

if (!string.IsNullOrEmpty(SessionValues.AccessToken))
{
    eventstatuscode = null;
    EventStatus eventStatus = rb.GetEventStatus();
    if (eventStatus != null && string.IsNullOrEmpty(eventStatus.cancelDate))
    {
        eventstatuscode = eventStatus.onWaitingList ? "waitinglist" : "accepted";
    }
    else
    {
        eventstatuscode = DateTime.Parse(eventDetails.signUpDeadline) < DateTime.Now ? "expired" : "logged";
    }
}

... you can condense that further using the ternary expressions you asked about, to remove the rest of the if statements (untested code ahead):

EventStatus eventStatus = rb.GetEventStatus();
eventstatuscode = (eventStatus != null && string.IsNullOrEmpty(eventStatus.cancelDate))
    ? (eventStatus.onWaitingList) ? "waitinglist" : "accepted"
    : (DateTime.Parse(eventDetails.signUpDeadline) < DateTime.Now) ? "expired" : "logged";

I don't recommend that though: because IMO Uri's version is almost equivalent and is easier to read.

Answer 4

Looking at the initial code, it seems like we are doing two things:

1. Check if the user is authenticated
2. Parse the status code

I have assumed the code is under your control. If not, most of the methods can be implemented as extension methods while keeping the syntax.

After refactoring, this is the top level code:

if (!Authenticated()) return;

var eventStatus = rb.GetEventStatus();
var deadline = DateTime.Parse(eventDetails.signUpDeadline);

eventstatuscode = eventStatus.GetStatusCode(deadline);

Checking for authentication is simply:

static bool Authenticated()
{
    return !string.IsNullOrEmpty(SessionValues.AccessToken);
}

The EventStatus class got a new method to parse the status code. There are four status codes of interest, and I like to be explicit on each one:

public string GetStatusCode(DateTime deadline)
{
    if (HasExpired(deadline)) return "expired";
    if (IsOnWaitingList()) return "waitinglist";
    if (IsAccepted()) return "accepted";
    return "logged";
}

The evaluation of each status code is extracted to individual methods. The whole class now looks like this:

internal class EventStatus
{
    readonly string cancelDate;
    readonly bool onWaitingList;

    public EventStatus(string cancelDate, bool onWaitingList)
    {
        this.cancelDate = cancelDate;
        this.onWaitingList = onWaitingList;
    }

    public string GetStatusCode(DateTime deadline)
    {
        if (HasExpired(deadline)) return "expired";
        if (IsOnWaitingList()) return "waitinglist";
        if (IsAccepted()) return "accepted";
        return "logged";
    }

    bool HasExpired(DateTime deadline)
    {
        return !Active() && Expired(deadline);
    }

    bool IsOnWaitingList()
    {
        return Active() && onWaitingList;
    }

    bool IsAccepted()
    {
        return Active() && !onWaitingList;
    }

    bool Active()
    {
        return String.IsNullOrEmpty(cancelDate);
    }

    static bool Expired(DateTime deadline)
    {
        return deadline < DateTime.Now;
    }
}

Answer 5

One technique that in my opinion makes the code way simpler is to exit as soon as possible (or "fail early", or "handle-the-error-first", if you'd like). This avoids having multiple nested blocks in your code, plus keeps your mind "clean" because you can be sure that, if the code followed under a given position, you can always be sure the right conditions have been met (and you don't have to worry about them any longer).

For instance, I would rewrite the code as

if (string.IsNullOrEmpty(SessionValues.AccessToken))
   return; // fail early

// now we don't have to worry about SessionValues.AccessToken

eventstatuscode = "logged";
EventStatus eventStatus = rb.GetEventStatus();
if (eventStatus == null)
{
    eventstatuscode = DateTime.Parse(eventDetails.signUpDeadline) < DateTime.Now ? "expired" : eventstatuscode;
    return;
}

// now we don't have to worry about eventStatus
if (string.IsNullOrEmpty(eventStatus.cancelDate))
{
    eventstatuscode = eventStatus.onWaitingList ? "waitinglist" : eventstatuscode;
    eventstatuscode = eventStatus.onWaitingList == false ? "accepted" : eventstatuscode;
    return;
}

eventstatuscode = DateTime.Parse(eventDetails.signUpDeadline) < DateTime.Now ? "expired": eventstatuscode;

Some people might not like this because someone has told them to avoid having multiple return points in a function. But for me, the above is much clearer. Every time I am reading a code that branches into an if-else, I have to keep both in my head, read the if, then go back, read the else as if the 'if' never happened, then start wondering what would be the output if the "if" conditions were met, then the else... back and forth.

This way I can read the if, and when I see it leads to a return, I know I will be able to forget that block to understand the rest of the function. One less thing to have in mind in order to understand the flow of the algorithm. I would apply the same to the ternary operation blocks, likely removing them if they don't lead to any reading improvements.

Plus, whenever I find myself in the middle of the function, I can always know that the bad conditions have already been handled (otherwise the execution cursor wouldn't be in that position in the first place). And if the execution shouldn't be there but is, I can quickly realize where the bug was (in the condition checks in the beginning of the function).

By the way, I hope I didn't introduce any mistakes in this refactoring as I didn't test it. I believe I might have, because usually I rely heavily on unit testing to get those refactoring artifacts!