Scott Hanselman

ELMAH and Exception Driven Development FTW

May 5, '09 Comments [53] Posted in ASP.NET | ASP.NET MVC
Sponsored By

Jeff blogged last month about Exception-Driven Development. I've been using ELMAH for years and you should too. Having great instrumentation in your app is such a joy. I added ELMAH to NerdDinner and have learned all sorts of things. It's amazing that someone would care to hack a site about Nerds eating dinner, but they try.

This wasn't a hack, but a great bug found in my Nerd Dinner Mobile code that I wouldn't have thought to look for. Here I'm getting a NullReference Exception...why?

image

Well, here's the code:

private bool UserAgentIs(ControllerContext controllerContext, string userAgentToTest)
{
return (controllerContext.HttpContext.Request.UserAgent.IndexOf(userAgentToTest,
StringComparison.OrdinalIgnoreCase) > 0);
}

Of course, I'm breaking the "Law Suggestion of Demeter" with all that digging, but what's the real issue? I'm assuming that the request has a UserAgent string at all! Exactly as the YSOD that ELMAH "tivo'ed" for me above.

So I changed it to this. Yes, I know that this could all be on one line and really baroque, but I find a few more lines to be easier to read.

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

I likely would have never thought of this bug had I not had logs and instrumentation. A smart user could have told me, or I could have used a Unit Test Generator like Pex, OR I could have just used my head and thought of it first. ;) Assert your assumptions. I didn't, and I assumed, wrongy, UserAgent would be non-null.

About Scott

Scott Hanselman is a former professor, former Chief Architect in finance, now speaker, consultant, father, diabetic, and Microsoft employee. I am a failed stand-up comic, a cornrower, and a book author.

facebook twitter subscribe
About   Newsletter
Sponsored By
Hosting By
Dedicated Windows Server Hosting by ORCS Web
Tuesday, May 05, 2009 11:24:19 PM UTC
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...
Tuesday, May 05, 2009 11:40:49 PM UTC
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;
}


=)
Wednesday, May 06, 2009 12:06:17 AM UTC
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. ;)
Colin Dabritz
Wednesday, May 06, 2009 2:48:28 AM UTC
== true? Seriously?

That said, I also prefer the "?? """ approach:
string ua = controllerContext.HTTPContext.Request.UserAgent ?? "";
Wednesday, May 06, 2009 3:08:09 AM UTC
Me too, I prefer
string ua = controllerContext.HTTPContext.Request.UserAgent ?? "";
But "" should be string.Empty, it is more efficient.
Wednesday, May 06, 2009 3:44:25 AM UTC
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).
Wednesday, May 06, 2009 5:50:50 AM UTC
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.
Wednesday, May 06, 2009 6:38:07 AM UTC
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
Wednesday, May 06, 2009 6:44:50 AM UTC
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.
Diego
Wednesday, May 06, 2009 9:22:45 AM UTC
Colin,

I agree, the if should have curly braces. I read it in Code Complete years ago and I have always done it since. :)
rtpHarry
Wednesday, May 06, 2009 9:40:29 AM UTC
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?
Wednesday, May 06, 2009 10:12:59 AM UTC
"== 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)
Jim Cooper
Wednesday, May 06, 2009 10:13:43 AM UTC
and btw the code ">0" is wrong. it must be ">=0" because the match could occur at the first position...
ad
Wednesday, May 06, 2009 10:50:28 AM UTC
Resharper catches stuff like this with a 'possible null reference' hint. Yet another reason to fork out the cash.
Wednesday, May 06, 2009 11:08:05 AM UTC
@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);
Wednesday, May 06, 2009 12:07:21 PM UTC
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
Dejan
Wednesday, May 06, 2009 12:18:39 PM UTC
These comments are amazing(ly bad). At least you got a legit bug report out of it.
huey
Wednesday, May 06, 2009 12:58:35 PM UTC
@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.
Wednesday, May 06, 2009 2:09:05 PM UTC
@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.
Wednesday, May 06, 2009 2:30:02 PM UTC

@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().
Wednesday, May 06, 2009 2:37:05 PM UTC
Does EMLA work in silverlight???
Ori Almog
Wednesday, May 06, 2009 4:29:32 PM UTC
I just bet Scott is baiting us with the '== true'
Wednesday, May 06, 2009 6:09:03 PM UTC
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);
}
Wednesday, May 06, 2009 8:13:21 PM UTC
We adopted AVICODE, its pretty good.
geeth
Wednesday, May 06, 2009 11:59:52 PM UTC
Scott,

You must reply now. Put these people out of their misery.... :)
Thursday, May 07, 2009 12:09:21 AM UTC
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! :)
Thursday, May 07, 2009 12:38:08 AM UTC
FUN READ!

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

Thursday, May 07, 2009 2:28:23 AM UTC
What's a null?

JimmyD
Thursday, May 07, 2009 2:28:25 AM UTC
What's a null?

JimmyD
Thursday, May 07, 2009 2:44:30 AM UTC
Scott,

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

Mark
Mark
Thursday, May 07, 2009 11:31:18 AM UTC
@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.
Jim Cooper
Thursday, May 07, 2009 5:09:44 PM UTC
I guess you don't run resharper. That has static null analysis built in.
richb
Thursday, May 07, 2009 6:59:33 PM UTC
@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!
Thursday, May 07, 2009 7:08:19 PM UTC
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.

Thursday, May 07, 2009 7:22:17 PM UTC
@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.
Thursday, May 07, 2009 8:13:09 PM UTC
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
Thursday, May 07, 2009 10:53:08 PM UTC
@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?
Thursday, May 07, 2009 11:43:49 PM UTC
Is there a similar exception logging framework for .NET desktop applications in VB or C#?
Thursday, May 07, 2009 11:50:12 PM UTC
"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.
Diego
Friday, May 08, 2009 7:30:41 AM UTC

> 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
Friday, May 08, 2009 4:05:35 PM UTC
@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).

Friday, May 08, 2009 8:04:43 PM UTC
@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.
Monday, May 11, 2009 11:12:29 AM UTC
@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.
Jim Cooper
Monday, May 11, 2009 4:09:18 PM UTC
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
Pop Catalin
Tuesday, May 12, 2009 12:33:30 AM UTC
@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.
Wednesday, May 13, 2009 1:20:14 PM UTC
@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.
Jim Cooper
Wednesday, May 13, 2009 1:27:13 PM UTC

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.
Jim Cooper
Friday, May 15, 2009 6:07:02 PM UTC
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.
Monday, May 18, 2009 12:41:27 PM UTC
@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?
Jim Cooper
Tuesday, May 19, 2009 3:58:57 AM UTC
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); }
Monday, June 01, 2009 5:35:40 AM UTC
@Jim Cooper

as in James W?
Monday, June 01, 2009 5:38:05 AM UTC
@Jim Cooper

as in James W?
Wednesday, June 17, 2009 5:44:16 PM UTC
@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
Comments are closed.

Disclaimer: The opinions expressed herein are my own personal opinions and do not represent my employer's view in any way.