Ayende @ Rahien

It's a girl

Exception handling for flow control is EVIL!!!

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
10/27/2010 10:09 AM by
NC

Link doesn't work :(

Link makes bunny cry.

wally
10/27/2010 11:20 AM by
wally

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

Grimace of Despair
10/27/2010 11:37 AM by
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
10/27/2010 01:21 PM by
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
10/27/2010 01:22 PM by
configurator

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

Alex Simkin
10/27/2010 04:16 PM by
Alex Simkin

Yeah, blame everything on Hybernate

Alessandro Riolo
10/27/2010 06:05 PM by
Alessandro Riolo

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

Asmur Trashir
10/28/2010 08:10 AM by
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
10/28/2010 07:10 PM by
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
10/28/2010 10:25 PM by
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
11/03/2010 08:47 AM by
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.

Comments have been closed on this topic.