Ayende @ Rahien

Hi!
My name is Oren Eini
Founder of Hibernating Rhinos LTD and RavenDB.
You can reach me by phone or email:

ayende@ayende.com

+972 52-548-6969

, @ Q c

Posts: 10 | Comments: 37

filter by tags archive

Exception handling for flow control is EVIL!!!

time to read 4 min | 792 words

I just spent over half a day trying to fix a problem in NHibernate. To be short one of the updates that I made caused a backward compatibility error, and I really wanted to fix it. The actual error condition happen only when you use triple nested detached criteria with reference to the root from the inner most child. To make things fun, that change was close to 9 months ago, and over a 1,000 revisions.

I finally found the revision that introduced the error, and started looking at the changes. It was a big change, and a while ago, so it took some time. Just to give you some idea, here is where the failure happened:

public string GetEntityName(ICriteria criteria)
{
  ICriteriaInfoProvider result;
  if(criteriaInfoMap.TryGetValue(criteria, out result)==false)
    throw new ArgumentException("Could not find a matching criteria info provider to: " + criteria);
  return result.Name;
}

For some reason, the old version could find it, and the new version couldn’t. I traced how we got the values to criteriaInfoMap every each way, and I couldn’t see where the behavior was different between the two revisions.

Finally, I resorted to a line by line revert of the revision, trying to see when the test will break. Oh, I forgot to mention, here is the old version of this function:

public string GetEntityName(ICriteria criteria)
{
  string result;
  criteriaEntityNames.TryGetValue(criteria, out result);
  return result;
}

The calling method (several layers up) looks like this:

private string[] GetColumns(string propertyName, ICriteria subcriteria)
{
  string entName = GetEntityName(subcriteria, propertyName);
  if (entName == null)
  {
    throw new QueryException("Could not find property " + propertyName);
  }
  return GetPropertyMapping(entName).ToColumns(GetSQLAlias(subcriteria, propertyName), GetPropertyName(propertyName));
}

But even when I did a line by line change, it still kept failing. Eventually I got fed up and change the GetEntityName to return null if it doesn’t find something, instead of throw.

The test passed!

But I knew that returning null wasn’t valid, so what the hell(!) was going on?

Here is the method that calls the calling method;

public string[] GetColumnsUsingProjection(ICriteria subcriteria, string propertyName)
{
  // NH Different behavior: we don't use the projection alias for NH-1023
  try
  {
    return GetColumns(subcriteria, propertyName);
  }
  catch (HibernateException)
  {
    //not found in inner query , try the outer query
    if (outerQueryTranslator != null)
    {
      return outerQueryTranslator.GetColumnsUsingProjection(subcriteria, propertyName);
    }
    else
    {
      throw;
    }
  }
}

I actually tried to track down who exactly wrote this mantrap, (this nasty came from the Hibernate codebase). But I got lost in migrations, reformatting, etc.

All in all, given how I feel right now, probably a good thing.


Comments

NC
NC

Link doesn't work :(

Link makes bunny cry.

wally

I think I've seen that one in other projects...

Grimace of Despair

Wasn't there a time in which the only way to foolproof convert a string to an integer in .NET was to wrap the conversion in a try...catch?

Patrick Huizinga

I would like to add that the only exception (heh) I can think of is an OperationCanceledException or equivalent.

I once rewrote a few-hundred line method with "if canceled then return" all over the place (up to four levels deep) into a dozen methods with "if canceled then throw" and a catch in the entry method. Imho that made it much easier to follow.

configurator

@Grimace: Yes, int.tryParse was only introduces in .Net 2.0...

Alex Simkin

Yeah, blame everything on Hybernate

Alessandro Riolo

Such things are the reason why Ctrl+Alt+E is the most commonly pressed key combination on my keyboards :)

Asmur Trashir

And how about this, I saw this left by a previous worker of the company where I work and it was an ASP.NET application and this was broadly used:

public bool IsLeapYear(int year)

{

 DateTime leapYear = DateTime.MinValue;

 try

 {

      leapYear = new DateTime(year,2,29);

 }

 catch

 {

      // Year is not leap

 }

 return (leapYear != DateTime.MinValue);

}

Nestor

Nice to see you again talking and working on NHibernate. I think you got very busy with RabenDB. Do not forget that there are still many people who follow you with the intention to know more about your experiences and advices with NHibernate.

Steve Py

@Asmur: You've got to be kidding?! I mean, had that guy never heard of MSDN/Google/ etc?! IsLeapYear is already a method on DateTime since .Net 1.1, Anytime I come across a requirement like that that I haven't come across before, the first thing I do is search the web. More than half the time, it's already part of the framework. The remaining 49.5% of the time someone has already come up with a pretty good solution.

Heaven help any developer I catch checking in something like that. They will be bringing in donuts and/or coffee for the team for a week.

Asmur Trashir

@Steve Py: I wish I were kidding. First day in the company and I caught that.

We were having issues with web services proxies. The ASP.NET web site was instantiating correctly every web service proxy (another ASP.NET project in the same solution) but executing any method would result in exceptions.

The problem was that the programmer (yes, programmer. Programmers only type code, they dont understant what happens behind the scenes like a Developer) placed proxy service url's in VS designer sections, the ones that get deleted everytime You make a change in design. In devbox they were running fine, but at the client exceptions were thrown. Finding that resulted in finding the method I posted. Actually, I just copy-paste the original code, because I keep it as a reminder of what can go wrong when dealing with programmers instead of developers.

That same programmer made another web application that was single-user. By single-user I mean a web application that only allows a single user to be logged in and browsing the webapp. As soon as I could I rewrote it as a 3-tier layered desktop application.

What really hit me was my boss telling me "thats why we no longer hire new-grads. Thats why we are hiring now experienced people.", I almost yelled "so we are still wrong, we need people with good common sense...". Nowadays I personally interview candidates and I just ask them a few common sense questions like "I need to know if a Year is Leap, you have the next options as answers, you can select one or more or execute them in any order: google-it, search msdn library, ask for directions/tips, write some code.". If they dont discard write some code or have that as last resort I just end the interview thanking them and black-listing them.

Comment preview

Comments have been closed on this topic.

FUTURE POSTS

  1. Production postmortem: The case of the memory eater and high load - 17 hours from now
  2. Production postmortem: The case of the lying configuration file - about one day from now
  3. Production postmortem: The industry at large - 3 days from now
  4. The insidious cost of allocations - 4 days from now
  5. Find the bug: The concurrent memory buster - 5 days from now

And 4 more posts are pending...

There are posts all the way to Sep 10, 2015

RECENT SERIES

  1. Find the bug (5):
    20 Apr 2011 - Why do I get a Null Reference Exception?
  2. Production postmortem (10):
    14 Aug 2015 - The case of the man in the middle
  3. What is new in RavenDB 3.5 (7):
    12 Aug 2015 - Monitoring support
  4. Career planning (6):
    24 Jul 2015 - The immortal choices aren't
View all series

Syndication

Main feed Feed Stats
Comments feed   Comments Feed Stats