fredag, juni 08, 2007

This is what's wrong

I must say, I got some really good responses to my post about what was wrong with the code I posted. Most of those responses concerned the design of the code, and I agree, this part of Rails could have been done much better. But what I was thinking about was actually a bug. And Lars Westegren (my former colleague) nailed it at the first try. Let me show two important excerpts from this code:
column_type_sql = native.is_a?(Hash) ? native[:name] : native
and here:
column_type_sql << "(#{limit})" if limit
Obviously, double left arrow is append, and for all cases where there is a limit, this append will change the String. This is one of the cases where it's kind of annoying that strings are mutable. If I cache away the values that native_database_types should return, then the next time anyone wants a string SQL type, that will generate VARCHAR(255)(255). The next time again, VARCHAR(255)(255)(255). And so one. So either I need to recreate the hash every time, or I need to do a deep clone of it every time. Neither of these options are very good, and it seems the deep clone option isn't fast enough, even when done in Java, so I decided to go with a hash literal instead. Was that the right choice? I don't know. It improves performance, but on the other hand it churns objects and creates new objects all the time. All of this because of some sloppy coding in Rails.

What's the lesson learned? Never modify your arguments, unless that is an explicit part of the contract for that method and part of the documentation.

2 kommentarer:

Erock sa...

column_type_sql is not an argument though, its a local variable. Or do you mean its the return argument? I don't see where anything in native_database_types are being changed. I must be missing something...

sujeet sa...

column_type_sql points to the same string as native[:name] (I'm using C lingo here), so column_type_sql << "something" modifies native[:name], and as Ola pointed out, the native data object is something that usually will be generated only once and cached (not created on every method call),
so its going to return wrong results on later calls.
I'm a Ruby newbie so please be gentle if this is off the mark :-)