8

So I'm going through old code (2.0) and I came across this:

object isReviewingValue = ViewState["IsReviewing"];

if (isReviewingValue is bool)
{
  return (bool)isReviewingValue;
}

My first thought was to us the "as" keyword to avoid the unneeded 

(bool)isReviewingValue;

But "as" only works with non value types. No problem, I just went ahead and did this:

bool? isReviewingValue= ViewState["IsReviewing"] as bool?;
if (isReviewingValue.HasValue)
{
  return isReviewingValue.Value;
}

Question is: Besides looking a bit more readable, is this in fact better?

EDIT:

public Stopwatch AsRun()
{
  Stopwatch watch = new Stopwatch();

  watch.Start();
  for (Int32 loopCounter = 0; loopCounter < 10000; loopCounter++)
  {
    Object value = true;
    Boolean? test = value as Boolean?;
    if (test.HasValue)
    {
      Boolean something = test.Value;
    }
  }
  watch.Stop();

  return watch;
}

public Stopwatch ObjectIsRun()
{
  Stopwatch watch = new Stopwatch();

  watch.Start();
  for (Int32 loopCounter = 0; loopCounter < 10000; loopCounter++)
  {
    Object test = true;
    if (test is Boolean)
    {
      Boolean something = (Boolean)test;
    }
  }
  watch.Stop();

  return watch;
}

Answer: Turns out that with the above methods run in a test fashion, the original code is about 10 times faster.

Programmin Tool
  • 6,507
  • 11
  • 50
  • 68
  • 7
    I don't understand why you are even doing this. There isn't any improvement. They contain the same number of lines of code. The second is NOT any clearer that the 1st; in fact, there's more 'noise' in the second one. I say leave the first one alone. – John Kraft Apr 20 '10 at 14:14
  • Mostly because it seems really silly to type as object, use is, then type back to boolean when bool? will take care of it for me and gives me an easy way to check if the value was boolean in the first place (HasValue). Knowing that as will convert no bool values to null seems like a more sensible choice then the whole round about string to object to bool. – Programmin Tool Apr 20 '10 at 14:48
  • However, with that being said, I was curious also from a performance standpoint. – Programmin Tool Apr 20 '10 at 14:51
  • 1
    if you are using the "is-as" combination on tight loop, i suggest you read this: http://stackoverflow.com/questions/1583050/performance-surprise-with-as-and-nullable-types – Michael Buen Apr 20 '10 at 14:53
  • Interesting, now I'm going to have to test this. – Programmin Tool Apr 20 '10 at 14:58
  • Well I tested and mine is faster which I didn't expect. (Distinct possibility I screwed up the test) I edited my original question to reflect this. – Programmin Tool Apr 20 '10 at 15:20
  • Probably, I think I should have made this a wiki thing in the long run since it's more philosophical than possibly practical. – Programmin Tool Apr 20 '10 at 15:36
  • 1
    I don't believe the AsRun is actually doing any casting, I believe the compiler is optimizing out the cast, thus making it much faster. The reason is that bool? can be directly assigned a bool without cast. Try instead object test = bool; bool? test2 = test as bool?; – Erik Funkenbusch Apr 20 '10 at 15:42
  • 1
    Also, it's not true that "as" doesn't work on value types, as bool? *is* a value type, it only works on nullable types, and bool? is a special kind of value type that is nullable. – Erik Funkenbusch Apr 20 '10 at 15:49
  • Awesome, that did it. Yeah the first is by far faster. – Programmin Tool Apr 20 '10 at 15:57

6 Answers6

18

The coalesce operator will remove some code for you. To answer your question, as Jimmy made quite clear, the technical differences between the two are minuscule so use whichever you feel is better. Personally, I am inclined to use this method. I might be considered biased though...

private bool GetIsReviewing()
{
    return (ViewState["IsReviewing"] as bool?) ?? false;
}
ChaosPandion
  • 77,506
  • 18
  • 119
  • 157
5

I think the first one is more readable , and it is faster as well [about 10 nanoseconds versus 100 nanoseconds, according to a test I just ran ;) (i.e. not going to slow your program down either way) ]

Jimmy
  • 89,068
  • 17
  • 119
  • 137
0

It's actually a little worse. If the value in "IsReviewing" is not a bool or null, your code will throw an exception. The original version will just ignore it.

Tom Cabanski
  • 7,828
  • 2
  • 22
  • 25
  • 1
    He just needs to add a null check in his if statement and he's good to go – juharr Apr 20 '10 at 13:49
  • A null reference exception or an invalid cast exception. This isn't Java, you don't specify all potential exceptions... – cjk Apr 20 '10 at 14:20
  • @ck: the point is that isReviewingValue.HasValue doesn't throw an exception (it returns false) when isReviewingValue == null. – Jimmy Apr 20 '10 at 14:21
  • @Jimmy - I know, I was replying to sr pt – cjk Apr 20 '10 at 14:27
0

Second is better, because if ViewState["IsReviewing"] is not a bool, as keyword automaticaly set this to null. In first option you implement it self, you can dont do that. And result have in good container.

Svisstack
  • 16,203
  • 6
  • 66
  • 100
0

If in the first example you returned null otherwise, why not to use this:

bool? MyMethod()
{
  return ViewState["IsReviewing"] as bool?;
}
n535
  • 4,983
  • 4
  • 23
  • 28
0

You could use a generic extension method approach to add syntactic sugar:

public static class TypecastExtensions {
   public static T CastOrDefault<T>(this object o) where T : struct  {
      return o as Nullable<T> ?? default(T);
   }

   public static T CastOrDefault<T>(this object o, T defaultValue) where T : struct {
      return o as Nullable<T> ?? defaultValue;
   }
}

Usage:

bool isReviewingValue = ViewState["IsReviewing"].CastOrDefault<bool>();
spoulson
  • 21,335
  • 15
  • 77
  • 102