14
votes
\$\begingroup\$

Lately I seem to run into a situation very frequently where I want to write something like the following:

var x =
    condition1 ? value1 :
    condition2 ? value2 :
    condition3 ? value3 :
    condition4 ? value4 :
    throw new InvalidOperationException();

Suppose the conditions are not simply constants so I can’t use switch, but they are not complex enough expressions to warrant saying that ?: is too unreadable.

Of course the above construct doesn’t compile because throw is a statement, not an expression. Therefore, I considered writing a method that does exactly that:

/// <summary>Throws the specified exception.</summary>
/// <typeparam name="TResult">The type to return.</typeparam>
/// <param name="exception">The exception to throw.</param>
/// <returns>This method never returns a value. It always throws.</returns>
public static TResult Throw<TResult>(Exception exception)
{
    throw exception;
}

[...]

var x =
    condition1 ? value1 :
    condition2 ? value2 :
    condition3 ? value3 :
    condition4 ? value4 :
    Ut.Throw<MyType>(new InvalidOperationException());

Are there any problems with this approach that I’m not seeing?

\$\endgroup\$
5
  • 1
    \$\begingroup\$ How odd. throw expressions are legal in ?: expressions in C++. \$\endgroup\$
    – Jon Purdy
    Apr 10, 2011 at 23:18
  • 1
    \$\begingroup\$ Originality? This kind of code appears all over the place and has for years. \$\endgroup\$
    – OJ.
    Apr 16, 2011 at 21:52
  • 1
    \$\begingroup\$ @OJ.: Including the generic return type? \$\endgroup\$
    – supercat
    Apr 18, 2011 at 18:32
  • \$\begingroup\$ @Supercat: yes. \$\endgroup\$
    – OJ.
    Apr 18, 2011 at 22:42
  • \$\begingroup\$ Trackback: this inspired me to codereview.stackexchange.com/questions/5115/… I don't think it answers your question though \$\endgroup\$ Oct 2, 2011 at 14:03

5 Answers 5

8
votes
\$\begingroup\$

If it makes you feel any better, mscorlib and System assemblies both have internal ThrowHelper classes full of nothing but methods whose only statement is throw, like this:

internal static void ThrowKeyNotFoundException()
{
    throw new KeyNotFoundException();
}

Now I'm not going to say it's a good idea, but surely MS has a good reason for this, right?

\$\endgroup\$
4
  • 2
    \$\begingroup\$ I believe Microsoft’s reason for this is to minimise code size. A single call to a static method is smaller than a constructor call followed by a throw, especially if the constructor call takes arguments. But this doesn’t really relate to my question: these methods are still void and thus cannot be used in expressions... \$\endgroup\$
    – Timwi
    Apr 11, 2011 at 16:37
  • 1
    \$\begingroup\$ @Timwi: I'm not suggesting that you would use these methods, so it doesn't matter that these are void. You can put any type on your "Throw" methods, obviously. My point, though, is that if it's OK for MS to do it, it should be OK for you to do it. \$\endgroup\$
    – Gabe
    Apr 11, 2011 at 17:02
  • \$\begingroup\$ @Timwi I doubt code size is the issue, especially when the compiler will optimise out the call and essentially inline the throw. I'd say it's more about consistency; making sure that exception messages/contents are consistent across uses. \$\endgroup\$
    – OJ.
    Apr 16, 2011 at 21:54
  • \$\begingroup\$ @OJ: I was refering to IL code size, not jitted code size. But I take your point, consistency may play a role in it too. \$\endgroup\$
    – Timwi
    Apr 17, 2011 at 14:08
18
votes
\$\begingroup\$

I would avoid using such statement. It doesn't seem to be a right place for this construction inside ?: operators and I do not see major benefits in using it. I would consider using following options:

1)

Type variable;
if (condition1) variable = value1;
else if (condition2) variable = value2;
...
else throw new InvalidOperationException();

This has a disadvantage that in each line you have variable = but anyway compiler will let you know if you haven't initialized variable value before using it so it won't bother me much.

2) Also if your value1, value2, etc cannot be null then I would consider using this:

var variable = 
    condition1 ? value1 :
    condition2 ? value2 :
    condition3 ? value3 :
    condition4 ? value4 :
    null;
if (variable == null) throw new InvalidOperationException();

P.S.: Of course I cannot be sure without knowing the context but if you have to write such statements frequently then it already seems to be wrong for me, maybe something may be changed on higher level?

\$\endgroup\$
8
votes
\$\begingroup\$

One problem with this approach is stack trace population.

The stack trace (generally) gives most immediate access to information relating to where whatever it was that went wrong actually went wrong. In this case it is populated with information spelling out extra method calls that are once removed from the actual location of the issue and may require (in absence of useful error messages, or their brain) developers (or, eeek! users) to think.

The fact that the method is generic aids in saturating the stack trace with irrelevant information.

\$\endgroup\$
2
  • 3
    \$\begingroup\$ A full stack trace would not contain any less information. One would only have to ignore the first line of the stack trace — which you often have to do anyway, e.g. when a method’s precondition failed (ArgumentOutOfRangeException etc.). \$\endgroup\$
    – Timwi
    Apr 10, 2011 at 16:58
  • \$\begingroup\$ I understand; and it's not unheard of, yet certainly isn't preferential or common (from my experience). I'm just bringing one view to the table - if you can dismiss this point easily then so be it. I wouldn't use such a construct indiscriminately. \$\endgroup\$ Apr 10, 2011 at 17:01
7
votes
\$\begingroup\$

Although you do solve a bit of the code clutter by using ternary operators instead of a bunch of if else statements, I believe you didn't solve the original problem.

Usually a long list of if else statements indicate the need to refactor. Usually this is possible by applying the Strategy pattern, where an 'algorithm' is selected at runtime. Whether or not this is a viable approach for your code is dependant on the specific situations where you use it.

If the conditions you are checking on can easily be attributed to different states or concrete implementations, I advise a redesign.

If there is no 'logical' way to separate the different assignments, I'd prefer Snowbear's solution with the null check. I do see the benefit of your solution however, and find it clear enough.

\$\endgroup\$
4
votes
\$\begingroup\$

I think the only thing that's really wrong with this is that it's a bit unusual. That's not very wrong IMO, but may be hard to get past conservative reviewers. It's easier to accept this construct in the context of large expressions that aren't trivially expandable into multiple statements.

The only reason I can think of against such a feature in the core language is that it may be considered as not making the cost/benefit bar, along with lots of other small syntactic sugar ideas which look nice one by one, but together could make a language too complicated if not carefully considered and balanced.

Here's an example of something that becomes less elegant without Throw:

mylist.Add(new Item
{
    Prop1 = int.Parse(input1),
    Prop2 = input2 == "stuff" ? true : input2 == "notstuff" ? false : Ut.Throw(),
});

Just imagine this with a few more properties with more meaningful names.

\$\endgroup\$

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