How can I write this shorter?
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";
}
}
a source to share
I would be more interested in readability. It 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 these strings in UA and didn't need to worry about empty string user agents, this would be as simple as:
return hr.ServerVariables["X-OperaMini-Phone-UA"] ??
hr.ServerVariables["X-Device-User-Agent"] ??
hr.UserAgent ??
"Not Found";
a source to share
I don't see any way to cut this down significantly.
One way to keep multiple lines is to get rid of the curly braces around the first one:
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 also be careful when using exceptions for flow control. statenjason has the right idea.
a source to share
Right now, what you have is clear and legible. If you're trying to get there with less turnaround time, I don't think you will. If you're trying to get there with fewer lines of code, you can, but it will be ugly.
One easy way to shorten it on screen (same number of LOCs, -1) is to remove some 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";
}
}
It's a little less readable to me, but probably still livable.
Now you can make it really short using the conditional operator ( ?:
), but it will also be really nasty for readability. If I saw code similar to the one below in the code review, I would ask the developer to 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 follow the second example. (I'm not 100% sure what I will compile, I'm writing this from my head on my Mac right now, but I'm 99.9% sure it will and will work, and the next HATE developer is for it.)
a source to share
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";
}
}
a source to share
Likewise (everything else is just extra code doesn't do anything):
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.
a source to share
Here's a compressed version. Since you are return
inside each operator if
, you can remove your else
s. Also, I've eliminated the need for thread exceptions.
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;
}
a source to share
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";
}
}
a source to share
I don't like it when he does work that he shouldn't be doing. So I would write it like this:
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";
}
a source to share
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 configured with special UAs very easily by adding extra tuples.
It shouldn't be difficult to replace tuples with something else if you're not using .NET 4.
a source to share