How can I write this shorter?

Question

public static string GetUa(HttpRequest hr) { try { string visitorBrowser = hr.UserAgent.ToString(); string originalBrowser = hr.ServerVariables["X-OperaMini-Phone-UA"]; string anotherOriginalBrowser = hr.ServerVariables["X-Device-User-Agent"]; //novarra if (!String.IsNullOrEmpty(originalBrowser)) { return "OPERAMINI " + originalBrowser; } else { if (!String.IsNullOrEmpty(anotherOriginalBrowser)) { return "NOVARRA " + anotherOriginalBrowser; } else { return visitorBrowser; } } } catch { return "No UA Found"; } }

Calling ToString() on hr.UserAgent is not necessary because it's already a string and will eliminate using exceptions as part of your method flow. — statenjason
– statenjason, Commented May 2, 2010 at 14:56

Kent Boogaart · Accepted Answer · 2011-09-27 15:05:56Z

9

I'd be more concerned with readability. This seems nicer to me:

var operaAgent = hr.ServerVariables["X-OperaMini-Phone-UA"]; var deviceAgent = hr.ServerVariables["X-Device-User-Agent"]; operaAgent = string.IsNullOrEmpty(operaAgent) ? null : "OPERAMINI" + operaAgent; deviceAgent = string.IsNullOrEmpty(deviceAgent) ? null : "NOVARRA" + deviceAgent; return operaAgent ?? deviceAgent ?? hr.UserAgent ?? "Not Found";

Of course, if you didn't need to prefix those strings onto the UAs and didn't need to concern yourself with empty string user agents, then it would simply be:

return hr.ServerVariables["X-OperaMini-Phone-UA"] ?? hr.ServerVariables["X-Device-User-Agent"] ?? hr.UserAgent ?? "Not Found";

edited Sep 27, 2011 at 15:05

user142162

answered May 2, 2010 at 15:01

Kent Boogaart

180k39 gold badges401 silver badges398 bronze badges

Sign up to request clarification or add additional context in comments.

12 Comments

John Rudy Over a year ago

I completely forgot about the coalesce operator. Good call. Except one detail: You're not handling the Empty portion of IsNullOrEmpty. A minor detail, admittedly, one you can correct with a quick edit. (I still +1ed you; right idea.)

tzaman Over a year ago

+1 for readability as well as conciseness; although I'd turn the second if statement into an else if (shorter, and faster too if the first check succeeds).

Foxfire Over a year ago

However the code is wrong. != null is not the same as in the original code.

Kent Boogaart Over a year ago

Wow, picky picky :) I don't have enough context to know whether an empty string is expected. That would only happen if someone is populating that value with an empty string, and I assumed that that wouldn't happen in order to simplify the code. The OP has the context to know whether that is OK.

nLL Over a year ago

There are many bots, half baked mobile browsers that sends empty or no UA

|

Benjamin Geiger · Accepted Answer · 2010-05-02 15:00:20Z

I don't see any way to significantly shorten that.

One way to save a few lines is to get rid of the braces around the first else:

if (!String.IsNullOrEmpty(originalBrowser)) { return "OPERAMINI " + originalBrowser; } else if (!String.IsNullOrEmpty(anotherOriginalBrowser)) { return "NOVARRA " + anotherOriginalBrowser; } else if (!String.IsNullOrEmpty(visitorBrowser)) { return visitorBrowser; } else { return "No User Agent Detected"; }

You should be careful about using exceptions for flow control, too. statenjason has the right idea.

John Rudy · Accepted Answer · 2010-05-02 15:02:24Z

Right now, what you have is clear and legible. If you're trying to get there with less processing time, I don't think you're going to make it. If you're trying to get there with fewer lines of code, you can, but it's going to be ugly.

One easy way to shorten it on-screen (same LOC count, -1) is to remove some of your curly braces and not store visitorBrowser:

public static string GetUa(HttpRequest hr) { try { string originalBrowser = hr.ServerVariables["X-OperaMini-Phone-UA"]; string anotherOriginalBrowser = hr.ServerVariables["X-Device-User-Agent"]; //novarra if (!String.IsNullOrEmpty(originalBrowser)) return "OPERAMINI " + originalBrowser; else if (!String.IsNullOrEmpty(anotherOriginalBrowser)) return "NOVARRA " + anotherOriginalBrowser; else return hr.UserAgent.ToString(); } catch { return "No UA Found"; } }

To me, this is slightly less readable, but probably still livable.

Now you can make it really short by using the conditional operator (?:), but it's going to also be really nasty for readability. If I saw code like the following in a code review, I'd make the developer rewrite it for clarity:

public static string GetUa(HttpRequest hr) { try { string visitorBrowser = hr.UserAgent.ToString(); string originalBrowser = hr.ServerVariables["X-OperaMini-Phone-UA"]; string anotherOriginalBrowser = hr.ServerVariables["X-Device-User-Agent"]; //novarra return !(string.IsNullOrEmpty(originalBrowser)) ? "OPERAMINI " + originalBrowser : !(string.IsNullOrEmpty(anotherOriginalBrowser)) ? "NOVARRA " + anotherOriginalBrowser : visitorBrowser); } catch { return "No UA Found"; } }

Seriously, please don't do the second example. (I'm not 100% sure that will compile; I'm writing it off the top of my head on my Mac at the moment. But I'm 99.9% sure it will, and will work, and the next developer will HATE you for it.)

Am I the only person who finds that second piece extremely readable then? :)
I love the conditional operator, but I hate chaining them together. With two conditions, it's not too bad, but with 3 or more, I find the readability seriously lacking.
Fair enough. For me, if I can line them up the way you have here, it's still readable, no matter how many there are. I can see there's a '?' on each line, so I know what you're doing. I might be temped to drop 'visitorBrowser' to it's own line to say "this is the catch-all, at the end here". Where I do hate chained ternaries is where the chain is not always at the end. eg. 'A=1 ? B : C=1 ? D : E=1 ? F : G' is readable to me 'A=1 ? B=1 ? C : D : E=1 ? F : G' is not, even if you parenthesise it.

Guffa · Accepted Answer · 2010-05-02 15:02:39Z

Like this, for example:

public static string GetUa(HttpRequest hr) { try { string originalBrowser = hr.ServerVariables["X-OperaMini-Phone-UA"]; string anotherOriginalBrowser = hr.ServerVariables["X-Device-User-Agent"]; return !String.IsNullOrEmpty(originalBrowser) ? "OPERAMINI " + originalBrowser : !String.IsNullOrEmpty(anotherOriginalBrowser) ? "NOVARRA " + anotherOriginalBrowser : hr.UserAgent; } catch { return "No UA Found"; } }

statenjason · Accepted Answer · 2010-05-02 15:06:12Z

Here's a condensed version. Because you're returning inside each if statement, you can eliminate your elses. Also, I eliminated the need for using exceptions for flow.

 public static string GetUa(HttpRequest hr) { string visitorBrowser = hr.UserAgent; string originalBrowser = hr.ServerVariables["X-OperaMini-Phone-UA"]; string anotherOriginalBrowser = hr.ServerVariables["X-Device-User-Agent"]; //novarra if (string.IsNullOrEmpty(visitorBrowser)) return "No UA Found"; if (!String.IsNullOrEmpty(originalBrowser)) return "OPERAMINI " + originalBrowser; if (!String.IsNullOrEmpty(anotherOriginalBrowser)) return "NOVARRA " + anotherOriginalBrowser; return visitorBrowser; }

Foxfire · Accepted Answer · 2010-05-02 15:20:09Z

Like this (everything else is just extra code doing nothing):

public static string GetUa(HttpRequest hr) { if (!String.IsNullOrEmpty(hr.ServerVariables["X-OperaMini-Phone-UA"])) return "OPERAMINI " + hr.ServerVariables["X-OperaMini-Phone-UA"])) ; if (!String.IsNullOrEmpty(hr.ServerVariables["X-Device-User-Agent"])) return "NOVARRA " + hr.ServerVariables["X-Device-User-Agent"])) ; return hr.UserAgent ?? "Not Found"; }

And you should NEVER use exceptions in your normal application flow path.

But were do you get originalBrowser and anotherOriginalBrowser from?
Out of curiosity which one is faster? Assigning to string or requesting same servervariable couple of times?
It doesn't really matter. It's not "requesting" it from a server g. It just retrieves it from an index and the complexity is still O(1) which is the same as a simple string assignment.

Michiel Borkent · Accepted Answer · 2010-05-02 15:03:33Z

public static string GetUa(HttpRequest hr) { try { string visitorBrowser = hr.UserAgent.ToString(); string originalBrowser = hr.ServerVariables["X-OperaMini-Phone-UA"]; if (!String.IsNullOrEmpty(originalBrowser)) return "OPERAMINI"+originalBrowser; string anotherOriginalBrowser = hr.ServerVariables["X-Device-User-Agent"]; //novarra if (!String.IsNullOrEmpty(anotherOriginalBrowser)) return "NOVARRA" + anotherOriginalBrowser; return visitorBrowser; } catch { return "No UA Found"; } }

tkhduracell · Accepted Answer · 2010-05-02 15:06:26Z

second if-statment:

return (!String.IsNullOrEmpty(anotherOriginalBrowser) ? ("NOVARRA " + anotherOriginalBrowser) : visitorBrowser);

You can probably use this combined with the first if-statment too

JBrooks · Accepted Answer · 2010-05-02 15:20:05Z

I don't like when it is doing work that it doesn't have to do. So I would write it as follows:

 public static string GetUa(HttpRequest hr) { string browser = hr.ServerVariables["X-OperaMini-Phone-UA"]; if (!String.IsNullOrEmpty(browser)) return "OPERAMINI " + browser; browser = hr.ServerVariables["X-Device-User-Agent"]; //novarra if (!String.IsNullOrEmpty(browser)) return "NOVARRA " + browser; if (!String.IsNullOrEmpty(hr.UserAgent)) return hr.UserAgent; return "No UA Found"; }

Daniel Renshaw · Accepted Answer · 2010-05-02 15:57:07Z

A slightly more dynamic .NET 4 approach:

 private static readonly Tuple<string, string>[] SpecialUas = { Tuple.Create("X-OperaMini-Phone-UA", "NOVARRA"), Tuple.Create("X-Device-User-Agent", "OPERAMINI") }; public static string GetUa(HttpRequest r) { return ( from specialUa in SpecialUas let serverVariable = r.ServerVariables[specialUa.Item1] where !string.IsNullOrEmpty(serverVariable) select string.Concat(specialUa.Item2, " ", serverVariable) ).FirstOrDefault() ?? ( string.IsNullOrEmpty(r.UserAgent) ? "No UA Found" : r.UserAgent ); }

This can be customised with additional special UAs very easily by adding further tuples.

It wouldn't be difficult to replace the tuples with something else if you're not running on .NET 4.

Collectives™ on Stack Overflow

How can I write this shorter?

10 Answers 10

12 Comments

Comments

3 Comments

Comments

Comments

4 Comments

Comments

Comments

Comments

Comments

Hot Network Questions

Collectives™ on Stack Overflow

10 Answers 10

12 Comments

Comments

3 Comments

Comments

Comments

4 Comments

Comments

Comments

Comments

Comments

Related