CODEWITHSUNDEEP
c#
MetaGuru
MetaGuru

Reputation: 43813

Can anyone think of an elegant way of reducing this nested if-else statement?

if (Request.QueryString["UseGroups"] != null)
{
  if (Request.QueryString["UseGroups"] == "True")
  {
    report.IncludeGroupFiltering = true;
  }
  else
  {
    report.IncludeGroupFiltering = false;
  }
}
else
{
  report.IncludeGroupFiltering = false;
}

Upvotes: 13

Views: 1189

Answers (13)

arielsan
arielsan

Reputation: 428

report.IncludeGroupFiltering = ShouldIncludeGroupFiltering(Request.QueryString["UseGroups"])

private boolean ShouldIncludeGroupFiltering(String queryString) {
    return ("True" == queryString)
}

Upvotes: 0

verdy_p
verdy_p

Reputation: 1833

String constants are immutable and atomized, they are also reference objects. However the result of Request.QueryString["UserGroups"] is a string reference (or a reference to an object that may be implicitly converted to a string...) that may not be atomized, so you cant just compare the references which may be distinct, even if the strings are comparing equal.

The == operator on pairs of Strings is not comparing references but string contents. This means that the Request.QueryString["UserGroups"] will be dereferenced, and this may cause null pointer dereference exceptions. That's why there's a prior test for null (because tests with "reference == null" is NOT dereferencing the reference but actually checking if it's null or not).

There's a possibility however to avoid the null check: you may use the === operator to just compare the reference, if the result of Request.QueryString["Usergroups"] has been atomized (but it may require allocation and hashing within the static list of atoms, not a good idea if the QueryString is very large.

So yes, the best to do is to first cache the Querystring in a local string variable, and perform the two tests:

final string queryString; // cache the string value 
if ((queryString = Request.QueryString["UserGroups"]) != null &&
  queryString == "True") {
   ...
} else {
   ...
}

But given that the bodies of your if/else statement is just to store the result of the if()'s condition, just write this:

final string queryString; // temporary register caching the non-atomized string reference 
report.IncludeGroupFiltering =
  (queryString = Request.QueryString["UserGroups"]) != null &&
  queryString == "True"; // compares the two strings contents

BUT ONLY IF the Request.QueryString[] contents are already atomized strings, or if their implicit conversion to string return atomized strings, save the string compares and use === instead:

final string queryString; // temporary register caching the atomized string reference 
report.IncludeGroupFiltering =
  (queryString = Request.QueryString["UserGroups"]) != null &&
  queryString === "True"; // compares the atomized references

I will not suggest this dangerous assumption here (it's more probable that query results from a remote source will not be atomized, for security/memory reasons, unless the returned value has already been checked. Given your code, I suspect that this is performing validation on returned values from your query, so the result is most probably not atomized: the main reason of your code is to atomize the content of the Query String into a shared boolean value, which will later compare much more easily.

Note: I absolutely don't know what is the type of the value or reference returned by Request.QueryString["UserGroups"]. It may happen that this is an object that implements the "bool operator==(string)" method, or that even returns another type than bool. Storing the returned object in a string variable however will perform its conversion to a string if the object's type is not null (and if the object is compatible, otherwise you'll get an exception).

You may want to avoid this conversion of the unknown object, if the object itself can compare to a string like "True", with code like this:

report.IncludeGroupFiltering =
  Request.QueryString["UserGroups"] != null &&
  // uses object's operator==(string) to compare its contents OR reference.
  Request.QueryString["UserGroups"] == "True";

All this depends on how you declared the QueryString[] array property of your Request object, and if the array contents can be polymorphic (varying type). If you know how it's declared, then use exactly the same type for declaring the temporary final register above, to avoid double member access to QueryString from Request, and double indexing of the QueryString array.

Here, it's impossible to know which code will be the best for you as we don't have all the declarations (C# inherits the same type complexity/ambiguity as C++, with too many implicit conversions and complex inheritance schemes).

Upvotes: -1

Paul Nathan
Paul Nathan

Reputation: 40299

This is how I do this sort of code:

report.IncludeGroupFiltering = false;

if (Request.QueryString["UseGroups"] != null && 
    Request.QueryString["UseGroups"] == "True"       //note that I am not a C# expert - this line /may/ throw an exception if it is indeed null. 
  {
    report.IncludeGroupFiltering = true;
  }

Upvotes: 0

Hut8
Hut8

Reputation: 6342

report.IncludeGroupFiltering = Request.QueryString["UseGroups"] == "True";

Upvotes: 14

AsifQadri
AsifQadri

Reputation: 2388

I think Request.QueryString["UseGroups"] == "True" and "True" is a string only , it does not behave like bool. So, you can write in a line 

report.IncludeGroupFiltering = string.IsNullOrEmpty(Request.QueryString["UseGroups"])? 
                                false : (Request.QueryString["UseGroups"] == "True");

Upvotes: 3

Matthew Jones
Matthew Jones

Reputation: 26190

What about using TryParse:

bool includeGroupFiltering;
bool throwaway = Boolean.TryParse(Request.QueryString["UseGroups"], out includeGroupFiltering);
report.IncludeGroupFiltering = includeGroupFiltering;

Upvotes: 0

Dave McClelland
Dave McClelland

Reputation: 3413

report.IncludeGroupFiltering = Request.QueryString["UseGroups"] == "True"

Upvotes: 21

Jon Skeet
Jon Skeet

Reputation: 1499750

Simply a single check:

report.IncludeGroupFiltering = Request.QueryString["UseGroups"] == "True";

There's no need to evaluate Request.QueryString["UseGroups"] twice - it can only be equal to "True" if it's non-null, and the comparison will work perfectly well (and return false) if it is null.

Any solutions still doing two operations are over-complicating matters :)

Upvotes: 47

Emile
Emile

Reputation: 2230

Factor out the Request.QueryString["UseGroups"] part to make it clear you want to refer to the same thing, and then it becomes:

string useGroups = Request.QueryString["UseGroups"];
report.IncludeGroupFiltering = (useGroups != null) && (useGroups == "True");

Upvotes: 1

thelost
thelost

Reputation: 6694

report.IncludeGroupFiltering = "True" == Request.QueryString["UseGroups"];

Upvotes: 2

Dustin Laine
Dustin Laine

Reputation: 38503

Maybe this:

report.IncludeGroupFiltering = false;
if (Request.QueryString["UseGroups"] == "True")
    report.IncludeGroupFiltering = true;

Upvotes: 0

AllenG
AllenG

Reputation: 8190

You're doing it basically the way I would. Just remove the redundant inner else:

if(Request.QueryString["USeGroups"] != null)
{
  if(Request.QueryString["UseGroups"] == "True")
    report.IncludeGroupFiltering = true;
}
else report.IncludeGroupFiltering = false;

Upvotes: 0

Femaref
Femaref

Reputation: 61427

 report.IncludeGroupFiltering = (Request.QueryString["UseGroups"] != null) 
                                && (Request.QueryString["UseGroups"] == "True");

Upvotes: 0

Related Questions