db-torque-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Thomas Fischer <tfisc...@apache.org>
Subject Re: svn commit: r359584 - in /db/torque/runtime/trunk/src/java/org/apache/torque: ./ dsfactory/ manager/ util/
Date Mon, 02 Jan 2006 19:27:20 GMT


On Mon, 2 Jan 2006, Thomas Vandahl wrote:

> Thomas Fischer wrote:
>> Just out of curiosity: Why is it better to iterate over the entries of a
>> map instead of the keys, if one needs both key and entry ? I know it is
>> better to use the Entry set if one does not need the key, but here the key
>> is needed...
>
> In the given loop both parts of the entry are used, see the log.debug() call. 
> The code before took the key from the iterator and used get(key) to read the 
> value. This is basically a search call which is rather expensive. If you need 
> both, key and value, the entryset-iterator provides them without an 
> additional search operation.

That sounds reasonable. I did not know that, and it certainly makes sense 
in most places. However, I just happened to run the runtime test an hour 
ago on jdk 1.4.2_05, and it failed in the test case testSerialisation in 
line 568. The problem is that Criteria inherits from a Hashtable, and it 
overrides the get() method from the Hashtable. So criteria.get() does 
something else than entry.getValue(). In the case of Criteria, I'm 
afraid we have to switch back to the old code which iterates over the 
keys.

However, this was not clear to me before I debugged into it. In my 
opinion, the Criteria class being a child of Hashtable is very bad 
style, because it uses undocumented features of the Hashtable class. I'd 
like to change that, and a few other implementation issues (like 
Criterion not being a static inner class) in some stage.

> [...]
>> So in my code, I usually close stuff in the try block, set it to null, and
>> do a not null check in the finally block. If the reference to the resource
>> is not null, then I know an error has occured, try to close the other stuff
>> nevertheless, and ignore all exceptions which occur during clean-up (as
>> there is already an exception in the pipeline). This has the desired
>> behaviour for both cases. I would suggest that we do the same here. I can
>> do it, if noone objects.
>> 
>> If everyone agrees, we should also look at the other places where stuff
>> should be closed again (the connection in doSelect(criteria) and the like).
>
> Feel free. I guess there still are a couple of places where the same things 
> are handled in different ways. To improve code quality, it would be desirable 
> to clean this up, even if it is not strictly an error.

I will put it on my todo list.

>> I would not use throwTorqueException() on a Torque exception, but simply
>> rethrow it.
>
> I thought this was the exact purpose of throwTorqueException(). See the code 
> of this method.

Shame on me for my comment. I should have looked at the code.

    Thomas

---------------------------------------------------------------------
To unsubscribe, e-mail: torque-dev-unsubscribe@db.apache.org
For additional commands, e-mail: torque-dev-help@db.apache.org


Mime
View raw message