Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question
1  
Is the class EventStatus under your control or is it a framework class? Also what is a type of eventDetails? –  Vojta Feb 15 '14 at 12:52

5 Answers 5

up vote 16 down vote accepted

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";
    }
}
share|improve this answer
1  
Oh, you're right. One line was duplicated there, I gotta update my answer :) –  Simon André Forsberg Feb 15 '14 at 12:32

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.

share|improve this answer
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 '14 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 '14 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 '14 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 '14 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 '14 at 13:28

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.

share|improve this answer

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;
    }
}
share|improve this answer

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!

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.