0

Which is better of the following?

this.isLoggedIn = (bool)HttpContext.Current.Session["li"] == true;

or 

this.isLoggedIn = (bool)HttpContext.Current.Session["li"];

It needs be to true ONLY when the session is true. If the session is set to false will this evaluate to true in #2 as it exists? Or is it evaluating its value?

Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
Tom Gullen
  • 61,249
  • 84
  • 283
  • 456

8 Answers8

3

The second one:

this.isLoggedIn = (bool)HttpContext.Current.Session["li"];

(bool)HttpContext.Current.Session["li"] is already a boolean (so will be either true or false), so no need for the extra comparison and return value of the boolean expression.

Either way, you need to check that the li session variable exists before trying to cast it, or your code will throw (I think a NullReferenceException).

Oded
  • 489,969
  • 99
  • 883
  • 1,009
2

The latter is clearer, IMO. They're functionally equivalent though - in both cases, it will fetch the value of "li" from the session and attempt to cast it to bool, throwing an exception if the value isn't present.

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • Thanks, so they are both just evaluating the values of the session, and not evaluating if the session variable exists or not? (Like Javascript) – Tom Gullen Jan 26 '11 at 10:40
  • @Tom: Yes - the expression `(bool)HttpContext.Current.Session["li"];` is going to be evaluated either way. – Jon Skeet Jan 26 '11 at 10:41
  • @JonSkeet: Is it so, i thought a boolean would become false if it is `null`? – Tim Schmelter Jan 26 '11 at 10:42
  • @Tim: Nope, you're trying to perform an unboxing conversion, and that's going to fail if you don't unbox a genuine `bool`. – Jon Skeet Jan 26 '11 at 10:49
  • @Jon: But why does `Dim b As Boolean = Nothing` evaluates to `false`(option strict) in VB.Net and does not throw an exception? – Tim Schmelter Jan 26 '11 at 10:58
  • @Tim: Because Nothing in VB isn't quite the same as null in C#. In some contexts it's equivalent to default(T) instead. – Jon Skeet Jan 26 '11 at 11:21
  • @Jon: thanks for claifying. Sometimes VB.Net frightens me. It makes no sense at first sight why this throws an exception: `DirectCast(obj, Boolean)`(if obj is an object that is nothing) and this is `false`: `DirectCast(nothing , Boolean)`. – Tim Schmelter Jan 26 '11 at 11:32
  • @Tim, when dealing with this, it's better to use nullable variable, DirectCast(obj, Boolean?) or DirectCast(obj, nullable(of Boolean)) – Fredou Jan 26 '11 at 13:48
1

Create a property for the desired value:

public bool IsLoggedIn {
  get { return (bool)HttpContext.Current.Session["li"]; }
}

You could even go one extra level, if the session is used a lot in the class:

public bool IsLoggedIn {
  get { return (bool)Session["li"]; }
}

private HttpSessionState Session {
  get { return HttpContext.Current.Session; }
}

Also, if you ever want to look at the session by itself, use a better key, like "IsLoggedIn", instead of "li".

It might be good to create a special class for these application-wide values:

public static class MyAppSession {

  const string IsLoggedInKey = "IsLoggedIn";

  public static bool IsLoggedIn {
    get { 
      return Session[IsLoggedInKey] != null && (bool)Session[IsLoggedInKey]; 
    }
    internal set { Session[IsLoggedInKey] = value; }
  }

  // ...

  private static HttpSessionState Session {
    get { return HttpContext.Current.Session; }
  }

}
Jordão
  • 55,340
  • 13
  • 112
  • 144
0

The first and the second approach is equivalent, but the first one is to verbose for my taste. I like the second one much better.

Just as I like this

bool accepted = true;
if( accepted)
{
  ..
}

Better than

bool accepted = true;
if( accepted == true)
{
  ..
}

I feel it clearer that way if the variables are properly named.

Øyvind Bråthen
  • 59,338
  • 27
  • 124
  • 151
0

Just put the expected value in the place of the expression, and it will become pretty clear:

First example:
Before: this.isLoggedIn = (bool)HttpContext.Current.Session["li"] == true;
After:  this.isLoggedIn = true == true;

Second example:
Before: this.isLoggedIn = (bool)HttpContext.Current.Session["li"];
After:  this.isLoggedIn = true;

Now, try the same for the false case:

First example:
Before: this.isLoggedIn = (bool)HttpContext.Current.Session["li"] == true;
After:  this.isLoggedIn = false == true;

Second example:
Before: this.isLoggedIn = (bool)HttpContext.Current.Session["li"];
After:  this.isLoggedIn = false;

As you can see, there will be no difference in the result between the two approaches. It all comes down to questions about coding style and readability, where I would guess that you would find a bias towards the shorter version.

Fredrik Mörk
  • 155,851
  • 29
  • 291
  • 343
0

You never need to write code that says:

bool x = (y == true);

Instead just use

bool x = y;

In your specific case you should use:

this.isLoggedIn = HttpContext.Current.Session["li"] != null 
    && (bool)HttpContext.Current.Session["li"];

This way you will not get an exception if Session["li"] has not been assigned yet. However you will get an exception if Session["li"] is not castable to bool.

JK.
  • 21,477
  • 35
  • 135
  • 214
0

I would use the second option with a variant:
this.isLoggedIn = (bool) (HttpContext.Current.Session["li"] ?? "false");

The ?? is null-coalescing operator - it gives a value of "false" to the expression on its lefthand side, in case it happens to be null.

Arun
  • 2,493
  • 15
  • 12
-1

Both pieces of code are equal, so the better is the second (it's shorter).

Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
x2.
  • 9,554
  • 6
  • 41
  • 62