C# is there a nicer way of writing this?

Question

int uploadsID;
int pageNumber;
int x;
int y;
int w;
int h;

bool isValidUploadID = int.TryParse(context.Request.QueryString["uploadID"], out uploadsID);
bool isValidPage = int.TryParse(context.Request.QueryString["page"], out pageNumber);
bool isValidX = int.TryParse(context.Request.QueryString["x"], out x);
bool isValidY = int.TryParse(context.Request.QueryString["y"], out y);
bool isValidW = int.TryParse(context.Request.QueryString["w"], out w);
bool isValidH = int.TryParse(context.Request.QueryString["h"], out h);

if (isValidUploadID && isValidPage && isValidX && isValidY & isValidW & isValidH)
{

This is an ajax handler, checking all passed params are OK. Is this considered bad, and is there a better way to write this, or is it not that important?

You can use an object containing all properties and one constructor that require a NameValueCollection (Request.QueryString or just Request) and prepare object, this object expose one method: "IsValid" and check if all parameters are ok for the current request. — Roberto Conte Rosito, Feb 09 '11 at 10:02
You might want to consider posting this on [codereview](http://codereview.stackexchange.com/) — Benjol, Feb 09 '11 at 10:05

score 7 · Accepted Answer · answered Feb 09 '11 at 10:04

Assuming you're not going to use the individual bool variables elsewhere, you could write that as:

int uploadsID, pageNumber, x, y, w, h;
if (int.TryParse(context.Request.QueryString["uploadID"], out uploadsID) &&
    int.TryParse(context.Request.QueryString["page"], out pageNumber) &&
    int.TryParse(context.Request.QueryString["x"], out x) &&
    int.TryParse(context.Request.QueryString["y"], out y) &&
    int.TryParse(context.Request.QueryString["w"], out w) &&
    int.TryParse(context.Request.QueryString["h"], out h))
{
}

You may want to extract out int.TryParse(context.Request.QueryString[name], out variable into a separate method, leaving you with something like:

int uploadsID, pageNumber, x, y, w, h;
if (TryParseContextInt32("uploadID", out uploadsID) &&
    TryParseContextInt32("page", out pageNumber) &&
    TryParseContextInt32("x", out x) &&
    TryParseContextInt32("y", out y) &&
    TryParseContextInt32("w", out w) &&
    TryParseContextInt32("h", out h))
{
}

Alternatively, you could encapsulate all this context data into a new type with a TryParse method, so you'd have something like:

PageDetails details;
if (PageDetails.TryParse(context.Request.QueryString))
{
    // Now access details.Page, details.UploadID etc
}

That's obviously more work, but I think it would make the code cleaner.

Benjol · Answer 2 · 2011-02-09T10:18:55.257

Yes, start by factoring out your int.TryParse(etc.) into a separate function.

(Possibly over-influenced by F#)

//return a tuple (valid, value) from querystring of context, indexed with key
private Tuple<bool, int> TryGet(HttpContext context, string key)
{
    int val = 0;
    bool ok = int.TryParse(context.request.QueryString[key], out val);
    return Tuple.New(ok, val);
}

Then:

var UploadId = TryGet(context, "uploadID");
//...
if(UploadId.Item1 && etc..) 
{
    //do something with UploadId.Item2;

To make things slightly clearer, you could

private class ValidValue
{
    public bool Valid { get; private set; }
    public int Value { get; private set; }
    public ValidValue(bool valid, int value)
    { 
        Valid = valid;
        Value = value;
    }
    //etc., but this seems a bit too much like hard work, and you don't get 
    // equality for free as you would with Tuple, (if you need it)

More like a separate class, that validates it own properties. — Turrau, Feb 09 '11 at 10:05

score 3 · Answer 3 · answered Feb 09 '11 at 10:05

I would probably go for a formatting like this


int uploadsID, pageNumber, x, y, h;

if (Int32.TryParse(context.Request.QueryString["uploadID"], out uploadsID)
    && Int32.TryParse(context.Request.QueryString["page"], out pageNumber)
    && Int32.TryParse(context.Request.QueryString["x"], out x)
    && Int32.TryParse(context.Request.QueryString["y"], out y)
    && Int32.TryParse(context.Request.QueryString["w"], out w)
    && Int32.TryParse(context.Request.QueryString["h"], out h))
{
    ...
}

but I don't see anything wrong with your approach.

score 2 · Answer 4 · answered Feb 09 '11 at 10:02

2

One thing you can do is to replace this:

int uploadsID;
int pageNumber;
int x;
int y;
int w;
int h;

With this

int uploadsID, pageNumber, x, y, w, h;

answered Feb 09 '11 at 10:02

Øyvind Bråthen

59,338
27
124
151

Downvote is ok, but a reason would be appreciated. You obviously feel that the first code has a superior readability to my changed code? – Øyvind Bråthen Feb 09 '11 at 11:33

score 1 · Answer 5 · answered Feb 09 '11 at 10:08

1

try
{
    // use Parse instead of TryParse

    // all are valid, proceed
}
catch
{
    // at least one is not valid
}

answered Feb 09 '11 at 10:08

Turrau

452
1
4
11

It's usually a bad idea to include exceptions as a part of the normal program flow, and in this case, the user input being wrong is obviously a normal part of program flow, and should *not* be handled through exceptions. It's not very exceptional to have a user input something wrongly. – Øyvind Bråthen Feb 09 '11 at 10:15
That's plain bad. Not only you are using exceptions for control flow but also hurting performance. – Stilgar Feb 09 '11 at 10:57
Looks like a good solution to me if the catch statement returns an error code or re-throw. @Stilgar: Do you really care about performance in error cases? – adrianm Feb 09 '11 at 11:12
Depends on what you do in the error case and how many errors you generate. What if the page can be served with no arguments and this is the most common (default) case? – Stilgar Feb 09 '11 at 11:36
@Stilgar: Are you sure, this hurts performance? I remember an blog article where someone compared Guid.TryParse and try{Guid.Parse} ... and they performed equally ... – Turrau Mar 10 '11 at 13:51

score 0 · Answer 6 · answered Feb 09 '11 at 10:25

You could write a helper that gets rid of the ugly out passing style of TryParse, such as:

public delegate bool TryParser<T>(string text, out T result) where T : struct;

public static T? TryParse<T>(string text, TryParser<T> tryParser)
                             where T : struct
{
    // null checks here.
    T result;
    return tryParser(text, out result) ? result : new T?();
}

And then (assuming you are only interested in validity):

bool isValid = new [] { "uploadID" , "page", "x", "y", "w", "h" }
              .Select(q => context.Request.QueryString[q])
              .All(t => TryParse<int>(t, int.TryParse).HasValue);

If you need the individual values:

var numsByKey = new [] { "uploadID" , "page", "x", "y", "w", "h" }
               .ToDictionary(q => q,
                             q => TryParse<int>(context.Request.QueryString[q], 
                                                int.TryParse));

bool isValid = numsByKey.Values.All(n => n.HasValue);

This retains pretty much the same information as before, except the fine-grained info needs a lookup rather than a local-variable access.

C# is there a nicer way of writing this?

6 Answers6