patterncsharpModerate
Reduce code complexity by applying ternary operator
Viewed 0 times
operatorternaryreducecodeapplyingcomplexity
Problem
I have a condition in my code where I need to put a lot of nested
Can anyone suggest more optimization in this code block?
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;
}
}Solution
Before deciding to use more ternary operators in your code, let's understand what if/else is about:
When the expression in the
An
In your code you keep checking one condition, and then the opposite one, although this is totally redundant. if
Also you should try to make all your conditions positive conditions - refrain from asking
To sum the above, a better code would look like this:
Now it is easier to see that the same line is indicated twice - the
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";
}
}Code Snippets
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;
}
}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";
}
}Context
StackExchange Code Review Q#41727, answer score: 18
Revisions (0)
No revisions yet.