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 created the following HtmlHelper extension method for my Asp.Net MVC Razor views because the ternary syntax sucks when you need to use it intermixed with markup.

Is there a better way to write this and are there any potential type safety issues with it?

public static string GetValueTernary(this HtmlHelper htmlHelper, object a, object b, object valueIfEqual, object valueIfNotEqual)
{
    return a.Equals(b) ? valueIfEqual.ToString() : valueIfNotEqual.ToString();
}

and its usage in a Razor view is like this:

<div class="@Html.GetValueTernary(Model.UserVoteTypeId, VoteType.UpVote, "us", "u")">...</div>
share|improve this question
4  
Main issue I can see is that all arguments will be evaluated no matter what. Also your code could be factorised out : return (a.Equals(b) ? valueIfEqual : valueIfNotEqual).ToString() –  Josay Feb 18 at 16:51
2  
Can't you instead write <div class="@(Model.UserVoteTypeId == VoteType.UpVote ? "us" : "u")">...</div>? –  svick Feb 18 at 17:31
add comment

3 Answers 3

up vote 10 down vote accepted

I would change your method to use generics (so that the return value is meaningful) and not call .ToString() internally. This will enable you to use it in more scenarios.

public static T GetValueTernary<T>(this HtmlHelper html, object a, object b, T valueIfEqual, T valueIfNotEqual)
{
    return object.Equals(a, b) ? valueIfEqual : valueIfNotEqual;
}

Now you can use it like this as well and also it will enable you to use HtmlString-s as values.

<div class="@Html.GetValueTernary(Model.UserVoteTypeId, VoteType.UpVote, "us", "u").ToUpper()">...</div>

You could also benefit (depends on your actual usage) from not using object for the compared values - by using a generic the compiler will complain if the types of the two are not compatible (for example, you are comparing string to int):

public static TReturn GetValueTernary<TValue, TReturn>(this HtmlHelper html, TValue a, TValue b, TReturn valueIfEqual, TReturn valueIfNotEqual)
{
    return object.Equals(a, b) ? valueIfEqual : valueIfNotEqual;
}
share|improve this answer
    
p.s. object.Equals bit was borrowed from the answer by @svick –  Knaģis Feb 18 at 18:13
add comment

Your code will throw an exception when a is null. A safe way to compare values is the static version of object.Equals():

object.Equals(a, b)
share|improve this answer
add comment

I would do this a little differently...

/// <summary>
/// Uses <see cref="EqualityComparer{TValue}"/> to determine if <paramref name="left"/> is
/// equal to <paramref name="right"/>. If it is, <paramref name="returnIfEqual"/> is returned,
/// otherwise, <paramref name="returnIfNotEqual" /> is returned.
/// </summary>
public static TResult IfEqualReturn<TValue, TResult>(this HtmlHelper html, TValue left, TValue right, TResult returnIfEqual, TResult returnIfNotEqual)
{
    return EqualityComparer<TValue>.Default.Equals(left, right) ? returnIfEqual : returnIfNotEqual;
}

Notes:

  • Use generics instead of object to avoid boxing.
  • Use EqualityComparer<TValue> to check for equality. This class is very efficient, avoids boxing, and does a good job of handling the primitive types correctly, uses IEquatable<T> if TValue implements it, and correctly evaluates reference equality versus value equality depending on TValue. It also will not throw an exception if left or right is null.
  • When you create any function (but extensions methods in particular), choosing a descriptive but succinct name is critical. Ideally, the name should be enough information for another developer to know what the function does without looking at its code.

    • I chose IfEqualReturn because, to me, the most important thing to understand about the function is that it evaluates the equality of left and right to determine what to return-- remember, any boolean value can be used for a ternary expression, so GetValueTernary leaves out the critical detail that this function specifically evaluates equality.
    • Using XML comments is particularly important for extension methods. When you write your <summary>, imagine that another developer is reading your code, and explain what he would need to know to understand what the call will do. Keep it brief-- it should fit easily in a tooltip. If you feel like you need to explain more, add a <remarks> element.

Here are some other suggestions.

The .NET Framework interface IEqualityComparer<T> allows you to create classes that evaluate equality of a given type T. The following function allows someone to pass in an instance, and falls back on EqualityComparer<T>.Default if one is not specified.

public static TResult IfEqualReturn<TValue, TResult>(this HtmlHelper html, TValue left, TValue right, TResult resultIfEqual, TResult resultIfNotEqual, IEqualityComparer<TValue> equalityComparer = null)
{
    if (equalityComparer == null) 
        equalityComparer = EqualityComparer<TValue>.Default;

    return equalityComparer.Equals(left, right) ? resultIfEqual : resultIfNotEqual;
}

One place where this comes in handy is when comparing strings (see StringComparer). Usage:

var r1 = Html.IfEqualReturn("str1", "str2", true, false, StringComparer.Invariant); // r1 = false
var r2 = Html.IfEqualReturn("str1", "str1", true, false, StringComparer.Invariant); // r2 = true
var r3 = Html.IfEqualReturn("str1", "STR1", true, false, StringComparer.Invariant); // r3 = false
var r4 = Html.IfEqualReturn("str1", "STR1", true, false, StringComparer.InvariantIgnoreCase); // r4 = true

... or you could create one that uses a lambda expression:

public static TResult IfTrueThen<TValue, TResult>(this HtmlHelper html, TValue left, TValue right, TResult returnIfTrue, TResult returnIfFalse, Func<TValue, TValue, bool> evaluateFunc)
{
    return evaluateFunc(left, right) ? returnIfTrue : returnIfFalse;
}

// Usage:
var r1 = Html.IfTrueThen(0, 1, true, false, (l, r) => l == r); // r1 = false
var r2 = Html.IfTrueThen("string", "STRING", true, false, (l, r) => String.Equals(l, r, StringComparison.InvariantIgnoreCase)); // r2 = true

Another way:

public static TResult IfTrueThen<TResult>(this HtmlHelper html, Func<HtmlHelper, bool> evaluateFunc, TResult resultIfTrue, TResult resultIfFalse)
{
    return evaluateFunc(html) ? resultIfTrue : resultIfFalse;
}

// Usage:
int v1 = 0, v2 = 0;
var r1 = Html.IfTrueThen(h => v1 == v2, true, false); // r1 = true

v1 = 4;
var r2 = Html.IfTrueThen(h => v1 == v2, true, false); // r2 = false
var r3 = Html.IfTrueThen(h => (v1 - 4) == v2, true, false); // r3 = true

or...

<div class="@Html.IfTrueThen(h => Model.UserVoteTypeId == VoteType.UpVote, "us", "u")">...</div>
share|improve this answer
add comment

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.