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 code better.

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

Can anyone suggest more optimization in this code block?

Is the class EventStatus under your control or is it a framework class? Also what is a type of eventDetails? — Vojta, Feb 15 at 12:52

Uri Agassi · Accepted Answer · 2014-02-15 12:20:02Z

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

Oh, you're right. One line was duplicated there, I gotta update my answer :) — Simon André Forsberg, Feb 15 at 12:32

Simon André Forsberg · Answer 2 · 2014-02-15 17:33:45Z

up vote 11 down vote

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.

edited Feb 15 at 17:33

answered Feb 15 at 12:19

Simon André Forsberg
13.5k33198

1

Just to add a comment. I would say the two ternary operations at the end are very hard (not quick) to work out what will be returned. Particularly since both are checking the property onWaitingList but one checks directly and other one uses a comparison with FALSE. – Vojta Feb 15 at 12:40

@Vojta You have a point there, as I wasn't sure whether it was a pure boolean or a nullable-boolean I didn't replace it, but it does look like it could be written eventStatus.onWaitingList ? "waitinglist" : "accepted". Is that the reason for my down-vote? – Simon André Forsberg Feb 15 at 13:00

I am sorry, I am not a C# programmer so did not know about nullable booleans. I guess it is a nice feature but I am pretty sure that it would cause a headache to C# programmer as well to work out the proper return type (it was the reason for down-vote). However a suggestion. After you proposed the Enum you could have a method getStatus() in the enum class and return the proper string instead of having the ternary operations. That should work for C# right? – Vojta Feb 15 at 13:23

@Vojta Unlike Java, C# enums can't have methods in them. So that's not possible. I am not very good in C# but I do know some of it, so I am not entirely sure if nullable booleans would cause any problems here. I will edit my answer. – Simon André Forsberg Feb 15 at 13:26

I did not know that about the enums and method. Then what about the map with the key being the enum and the value being the status in String? Then you could use return map.get(eventStatus); . – Vojta Feb 15 at 13:28

show 5 more comments

ChrisW · Answer 3 · 2014-02-15 15:20:01Z

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.

asked	3 months ago
viewed	804 times
active	3 months ago

current community

your communities

more stack exchange communities

Reduce code complexity by applying ternary operator

3 Answers

Your Answer

Not the answer you're looking for? Browse other questions tagged c# ternary-operator or ask your own question.

Linked

Hot Network Questions

current community

your communities

more stack exchange communities

Reduce code complexity by applying ternary operator

3 Answers

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged c# ternary-operator or ask your own question.

Linked

Related

Hot Network Questions