Scott Hanselman

Love this component! I have it on all my apps. Watch out though if you turn on the e-mail feature and you have a high traffic site. The datacenter pushed some code of mine last week with a minor bug that didn't show up on the staging server. Our support inbox ended up with 70,000+ messages. Slowed down the server for several hours while it waited on the SMTP server to catch up. Beware...

Scott, please tell me you didn't just write:

if (true == true)
    doSomething();

How about this:

public bool UserAgentIs(ControllerContext controllerContext, string userAgentToTest)
{
    string UA = controllerContext.HttpContext.Request.UserAgent ?? String.Empty;
    return UA.IndexOf(userAgentToTest, StringComparison.OrdinalIgnoreCase) > 0;
}

=)

Travis, string.IsNullOrEmpty(str) is a test similar to:

return (str == null || str == string.empty)

and it is the recommend way to check for (is null or is empty) against strings in .net

The extra "== true" isn't necessary, but does add clarity.

I personally prefer the early terminating version, but I believe yours would work as well. Less branches that way I suppose.

What Scott did fail to do was put curly braces on the second line, making it clear that the if applied to the return only. ;)

== true? Seriously?

That said, I also prefer the "?? """ approach:

string ua = controllerContext.HTTPContext.Request.UserAgent ?? "";

Me too, I prefer
string ua = controllerContext.HTTPContext.Request.UserAgent ?? "";
But "" should be string.Empty, it is more efficient.

Might I also suggest making the method UserAgentIs into an extension method if you plan on using it elsewhere. That way you can write:

bool bAgentTest = controllerContext.UserAgentIs(userAgentToTest);


Also, writing if(something == true) will get you flayed by some...I remember reading somewhere that if you HAVE TO do that, its always better to write if(true == something).

I'll try not to get carried away with all the "if" talk but focus on the topic, you need to have a list of supported user agents and see if it's contained in the list.

Hi
I do approve of the guideline if(something == true). It appears in the .NET QA standard, I believe.

Hackers are nasty business, but they do get us programmers on the true side of the force to make better programs. If you don't, then your business will loose advantage over your competitors. Just like Nicholas Carr (http://www.nicholasgcarr.com/articles/matter.html).

Greetings,
Greendaale

To me "== true" makes things too verbose. And not in a bad way. In your example especially: if (string.IsNullOrEmpty(UA) == true)

The "IsNull" (the Is) already alludes to it checking if it's true or false. And it reads more cleanly without the "== true"

Read it out... if string is null or empty then. As opposed to... if string is null or empty equals true then. The last part is redundant.

Is that a habit from VB?

Just my view.

Colin,

I agree, the if should have curly braces. I read it in Code Complete years ago and I have always done it since. :)

This problem of unexpected input from the client makes me think of fuzz testing - throwing random data at an application to see what happens - does the app handle it nicely or crash? Something I know the theory of but I admit haven't tried in practice. Anybody any experience of fuzz testing an ASP.NET application and suggestions of tools?

"== true" and "== false" in code are always indicators of poor coding practices. Whenever I see that in code I know to examine that person's codebase for more egregious problems, like poor naming conventions (eg "UserAgentIs" - is this a name a sensible native English speaker would come up with?), ineffective grasp of the SOLID principles (maybe Law of Demeter issues), and so on.

I always associate "== true" and "== false" with inexperienced developers (no matter how many years they have had in the job)

and btw the code ">0" is wrong. it must be ">=0" because the match could occur at the first position...

Resharper catches stuff like this with a 'possible null reference' hint. Yet another reason to fork out the cash.

@ad: good catch indeed. That's why I would use StartsWith in this case:

string ua = controllerContext.HTTPContext.Request.UserAgent ?? "";
return ua.StartsWith(userAgentToTest, StringComparison.OrdinalIgnoreCase);

I haven't tried ELMAH myself although from all the good things I've been reading about it I always wanted to. However I am somewhat obsessed with Exception Driven Development, scanning the system log of the web server to find unusual exceptions, identify why they occurred and then fix them.

By the way, I can't believe how "nitpicky" some of the commenters are. For me bad developers are those that argue that one way of writing a working code is better then other way of writing working code with same functionality. As long as there is some consistent standard in the code I don't see a problem with it

These comments are amazing(ly bad). At least you got a legit bug report out of it.

@Jim Cooper

Glad to see that you have a very simple, infallible rule for determining whether someone is good at their job or not. How comforting to be able to make such sweeping judgments on such little information.

@Jim

Do you also associate people who prefer to say "did not" to "didn't" as being ineffective and poor speakers? Explicitness is not a sign of bad programmers.

@jeroenh: That's why I would use StartsWith in this case:

string ua = controllerContext.HTTPContext.Request.UserAgent ?? "";
return ua.StartsWith(userAgentToTest, StringComparison.OrdinalIgnoreCase);

What if you want the method to match a substring of the user agent that does not begin at the beginning? (i.e. - "MSIE 8.0")

Of course, in that case, it might be better to name the method UserAgentContains().

Does EMLA work in silverlight???

I just bet Scott is baiting us with the '== true'

The name "UA" burns in my eyes.
What about this:

public bool UserAgentIs(ControllerContext controllerContext, string userAgentToTest)  
{  
    string userAgent= controllerContext.HttpContext.Request.UserAgent;
    if (string.IsNullOrEmpty(userAgent))
    {
        return false;
    }
    return (userAgent.IndexOf(userAgentToTest, StringComparison.OrdinalIgnoreCase) > 0);  
}

We adopted AVICODE, its pretty good.

Scott,

You must reply now. Put these people out of their misery.... :)

GONeale - Heh. I think it's a pretty silly discussion, honestly. We all know this is a style issue. I was a C Programmer many years ago and I guess this habit stuck around. It all gets compiled the same. I think that saying it's the sign of a weak mind, or a programmer with bigger problems is a bit of a stretch.

As for UserAgentIs, that's just a fluency issue; I liked the way it flows. It's not killing kittens. ;)

I'm a little surprised at the ?? crowd, suggesting that I assign "" to a variable, then check IndexOf on it. I suppose it's easy to read at least.

Only "ad's" comment was useful to me, as he/she pointed out the > vs. >= latent bug.

Thanks to everyone, however, for your passion on this crucial issue! :)

FUN READ!

That's the Scott I have come to know and appreciate!

What's a null?

What's a null?

Scott,

Using capitals for a variable 'UA' ... :)
We have just put ELMAH into our mvc site and its working great so far....

Mark

@KevDog

Yes, it is good to have such a rule, seriously. For example, when you're interviewing someone for a programming position, you (or at least I) like to give them a small programming task, and/or see some code they've written. But you can't really test how well they write maintainable code in the large in such a short time. However, things like that (there are other indicators too) are a big clue.

I base this on 20-odd years of experience where I've paid attention to such things.

@Ryan C Smith

Clearly not. Why would I? I don't expect people to end every written sentence with a semicolon either. However, there are indicators of how good developers are, and all my experience suggests that is one of them. I'm all in favour of explicit code, but it already is without the == stuff. Where do you stop? Do you have this?:
if ((x == 7) == true)

@Scott

UserAgentIs is a fluency issue?!!? Maybe if Yoda you are. Jeez. First we have to fight against Hungarian notation, and now we get a naming convention based on how a puppet talks.

And how do we know you don't kill kittens? A man who writes code like that, and is not ashamed to show it to people, is clearly capable of anything.

I guess you don't run resharper. That has static null analysis built in.

@Jim Cooper said "I base this on 20-odd years of experience where I've paid attention to such things."

Built upon 20-odd years of being full of himself.

Here's some code for you to analyze:

If((Jim==TOTALLY_MISSED_POINT_OF_THE_POST)==true)
{
KillKittens();
}

Sheesh!

We did not wory too much about such things back in the day when we typed our code on an IBM 029 card punch.

Anyone remember Fortran and how it thrived on the GOTO statement.

(...then along came structured programming and GOTO statements were banned.
Just remember that GOTO statements are still required in machine code.)

Imagine you had a 1,000 line (or card) program.

I remember one Guy, when fixing bugs, he kept the the cards with the buggy code in the deck and just added a couple of cards to the deck which had GOTO statements to navigate around the buggy code.

In these days we did not get instant compiles. It sometimes took 24 hours just to find out if your code compiled.

I'm not in favour of going back to these days, but you need to keep a proper perspective on what your code is trying to achieve and what really matters:

1. The code should do what you want.
2. Should be easy to fix and maintain.
3. You hopefully can figure out what you did 6 months later.

Most code I see today fails tests 2 & 3 due to the lack of comments in the code.

@The Luddite Developer
If comments are needed to understand the code, there is a dire need for refactoring it... Comments inside of code blocks should only be necessary in the very rare cases where something "not logical" is taking place, or perhaps to explain a (temporary) hack.

Jeeez. So little code, so many opinions.
Most comments are not even about ELMAH.

It's not easy to be you, Scott :)
Just ignore most comments, especially offensive ones.

@GONeale: that was funny man :) And it worked!

@pete thanks! lol. I like you, am just amazed at how easily a crowd can get off topic. I think maybe one comment was about the context of the actual post?

Is there a similar exception logging framework for .NET desktop applications in VB or C#?

"I think it's a pretty silly discussion, honestly. We all know this is a style issue"

Since when is discussing style silly? Maybe not as important as discussions about solving the world's problems. But certainly have some merit.

> Built upon 20-odd years of being full of himself.

Don't be ridiculous. You don't just start out being full of yourself; you have to build up to it. I reckon it took about 5 or 6 years before I was even half full of myself, and I have only been completely full of myself for the last 9 or 10. Like everything worthwhile, you have to work at it.

And, seriously mate, your code is awful. Clearly TotallyMissedThePoint should be a characteristic of the Jim object. Something along these lines would be much better

  if (Jim.TotallyMissedThePointOf(ScottsPost))
  {
      Scott.KillKittens();  // He's the one with the experience, after all
  }

But then you used "== true", and as I pointed out earlier, that's always an indicator of poor coding ability. QED.

@Jim Cooper Okay, I will concede that the Jim object should have a TotallyMissedThePointOf(object justAboutEverything) method. BUT - you should check your documentation on the Scott API, because I'm pretty sure that the KillKittens() method of the Scott class does not provide a parameterless overload and that the proper signature would be Scott.KillKittens(int numKittens).

@Jim Cooper: I would hate to lose a prospective job because I used a "== true". I prefer that approach because it makes the intent much more clear in some cases. I've seen many bugs arise from people using too much if (!someFlag)... or if (SomeBusinessRule.Condition(xyz)) assuming true when false would be a much better assumption. Especially since my eyes keep changing and I need new glasses about every 10 months, sometimes reading text gets blurry and that (!someFlag) in the blurr the "!" escapes my notice. In that case, it is an accessibility issue. That does not make anyone less capable of producing results than someone who types with two fingers instead of touch typing.

In any case, it is a style issue and I prefer to be explicit based on 16 years of experience reading code that wasn't so verbose and on long 18 hour stretches my weary eyes and brain didn't immediately piece it together. For a company that would base a verbosity opion such as "== true" or not, to hire a candidate, I will be all the better to not work at that place.

Be glad it does not go both ways, that I wouldn't reject an applicant because they prefer not to use "== true" and instead omit it. I have much greater skills that I look for in when interviewing fellow architects/Sr. Developers, such as whether that can design, have designed, and the designs are well. Whether they can solve the problems the company needs to be solved, and whether they can create otherwise easy to understand and maintain code (notwithstanding the "== true" preference). Not whether they expressed a true/false condition B out of multiple other choices of style.

Just my two NOP's

@Shawn B

> I would hate to lose a prospective job because I used a "== true".

So don't use it in a job interview :-)

> In any case, it is a style issue

Yes, it is. However, every single one of the developers I've worked with who prefers that style has had poor skills at the more important things. I have no explanation for why that is, I've just seen the correlation. Whenever I have seen that in an interview, then I have always seen a poor grasp of OO principles, design skills and so on in their answers to those (much more important) questions.

So it's a big red flag.

It does not appear to be related to how long a given person has been working as a programmer, BTW.

I really wonder how many versions of .Net until MS finally adds "non nullable types" support? It's the most common exception in .Net apps today, and most common type of exception found in production environments that escapes testing. I wish we move along to other exceptions and be done with NullReferenceExceptions already :P

@Jim: Scott used it. Would you not hire him or do you consider him to have poor design skills or not grasp the basics? Just asking...

Oh, you're right, I could just not use it in an interview. But then I never know which interviewer makes such correlations. Better to just do what I do and take chances. More pressing is whether I skirt pass by the skin of my teethe and miss a pay raise or get canned because I put an angle bracket on the same line instead of the next line: who knows what message that is sending to my boss, or if I omit a turnary operator assignment/test in favor of a verbose if/else causing me a demotion -- because I felt that it would be easier to read or undertand more clearly my intentions that way.

Anyway, to each his own. I have not noticed the things you have or worked with the people you have and so do not realize why such a correlation makes a such a strong impression on you.

But I have worked with people in the past that are so passionate and forceful about their style that they themselves truly miss the really important things and I'll make one promise, I will not hire a person that is like that ever again.

Oh well, I'm over it :)

@Shawn B

Scott used it. Would you not hire him or do you consider him to have poor design skills or not grasp the basics? Just asking...

Well, he showed a one line function that had 2 bugs (although to be fair, he picked one with one known bug in it), a naming issue, and a law of Demeter violation, and looks damn near impossible to unit test**. He changed it into a 4 line function that is still pretty dodgy in the Demeter area, still has the naming issue, still has the bug, and now has the == true thing, and is still a bugger to unit test.

Also, he seems to really hate kittens.

So at this point he wouldn't be my number one candidate, no.

But then I don't read this blog to learn how to write code, and as I understand it, it isn't Scott's job to write code either. He has minions for that.

I have not noticed the things you have or worked with the people you have and so do not realize why such a correlation makes a such a strong impression on you.

I guess I'm just lucky to have worked with so many bad programmers over the years.

I have worked with people in the past that are so passionate and forceful about their style

I'm sure we all have, and to a certain extent it is good to adhere to a standard style for any given language. It makes life easier for everybody that way. It's not so good to have your own personal style.

But understand that I am not being "passionate and forceful" about this as a style issue. It's trivial to fix and not that important by itself. With a judicious application of electricity it's even possible to stop people doing it. However, it's the link between it and bigger, more serious problems that I've noticed..

If you use the == true style, then maybe it's time to run a metrics tool over your code and see what it throws up.

** It's hard to tell for sure from such a small snippet, but it seems unlikely a ControllerContext is the right thing to be passing to this routine. But since that is the root cause of the known bug, the six zillion dereferences and the unit testing dilemma, you have to start wondering.

I'm pretty sure that the KillKittens() method of the Scott class does not provide a parameterless overload and that the proper signature would be Scott.KillKittens(int numKittens).

Well, I had assumed he would just kill all the kittens he could find (he's clearly a madman), and hence the implementation would be along these lines:

  foreach (Kitten poorTinyDefencelessKitten in AllKittens)
  {
      poorTinyDefencelessKitten.Kill();
  }

but sure, if you think there's hope for some sort of reform in the future, you could pass a list of kittens to be killed. But just passing a number? I dunno, what're you going to do? Pick kittens using some random number generator until you have enough? That's cold, man.

So, I've gleaned a few things now:

- I rightfully chose ELMAH when it first showed up at GotDotNet some time ago as the logging entity for all of my ASP.Net Work

- Scott can't get a job working with or for Jim. Ever.

- Jim works very hard at making up for some abhorent loss that must have occured close to 20 years ago while he was captain of his High School debate team.

@Tim F

Scott can't get a job working with or for Jim. Ever.

Maybe if he had some sort of therapy about the kitten thing.

Jim works very hard at making up for some abhorent loss that must have occured close to 20 years ago while he was captain of his High School debate team.

I was at high school more than 30 years ago, actually, and I'm afraid the cultural reference has passed me by. Where I went to school, we didn't have "debate teams" (although if we had, they would have been "debating teams"). And that they need captains is news to me.

I was captain of the hockey team, though. Does that help?

All this fuss about a corner case and no one considered the other corner case of which Scott subtlely changed the behavior. Previously, if userAgentToTest == string.Empty, the function returned true even for empty user agent. Now it returns false. Whether this is a bug fix or introducing a new bug no one knows, because the function is underspecified.

And while this particular == true may not kill any kittens, I agree that it is bad business. Consider:

class Kitten : Cat { bool hasRabies() { return false; } }
Cat thisCat = new Kitten();
...
bool thisCatMustDie = thisCat.hasRabies();
...
if (thisCatMustDie = true) { Kill(thisCat); }

@Jim Cooper

as in James W?

@Jim Cooper

as in James W?

@Scott Hanselman
I prefer to call it CDD (Crash Driven Development), an alternative to TDD (Test Driven Development).
If it crashes, fix it :-D hehe