Re: svn commit: r359584 - in /db/torque/runtime/trunk/src/java/org/apache/torque: ./ dsfactory/ manager/ util/
Thomas Fischer wrote: 1.4). Maybe you are using jdk 1.5 to run the tests ? Yep, I have 1.5 installed. That could be it. Bye, Thomas. - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: svn commit: r359584 - in /db/torque/runtime/trunk/src/java/org/apache/torque: ./ dsfactory/ manager/ util/
Maven clean should work. You can check it by verifying that the "target" folder is gone. I have seen that serialization behaves different on different sdks(1.5 and 1.4). Maybe you are using jdk 1.5 to run the tests ? Thomas Thomas Vandahl <[EMAIL PROTECTED]> schrieb am 03.01.2006 08:12:04: > Thomas Fischer wrote: > > 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. > > That's strange. I ran the test and it worked fine. Could it be that my > "maven clean" didn't work right? > > Bye, Thomas. > > > - > To unsubscribe, e-mail: [EMAIL PROTECTED] > For additional commands, e-mail: [EMAIL PROTECTED] > - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: svn commit: r359584 - in /db/torque/runtime/trunk/src/java/org/apache/torque: ./ dsfactory/ manager/ util/
Thomas Fischer wrote: 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. That's strange. I ran the test and it worked fine. Could it be that my "maven clean" didn't work right? Bye, Thomas. - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: svn commit: r359584 - in /db/torque/runtime/trunk/src/java/org/apache/torque: ./ dsfactory/ manager/ util/
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: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: svn commit: r359584 - in /db/torque/runtime/trunk/src/java/org/apache/torque: ./ dsfactory/ manager/ util/
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. If an exception is thrown in a catch block, the finally block is executed, and an exception is thrown again in the finally block, and the exception is caught later on, the second exception is caught, and not the first exception. For example: [...] This was just copied from another place where a similar construct was used. See my comment below. 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 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. Just for the logs, same as above. What is better in iterating over the entry set instead of the key set ? Same as above: performance. Plus, if the iterator does not iterate over keys but over values, it should be renamed from keys to values. "entries" is probably correct. I'll do this. Bye, ThomasV - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: svn commit: r359584 - in /db/torque/runtime/trunk/src/java/org/apache/torque: ./ dsfactory/ manager/ util/
Nice clean-up. I have a few remarks, though:
[EMAIL PROTECTED] schrieb am 28.12.2005 19:36:43:
> Author: tv
> Date: Wed Dec 28 10:36:21 2005
> New Revision: 359584
>
> URL: http://svn.apache.org/viewcvs?rev=359584&view=rev
> Log:
> fixed some Findbugs and PMD related issues.
>
> Modified:
>
db/torque/runtime/trunk/src/java/org/apache/torque/dsfactory/JndiDataSourceFactory.
> java
> URL: http://svn.apache.
>
org/viewcvs/db/torque/runtime/trunk/src/java/org/apache/torque/dsfactory/JndiDataSourceFactory.
> java?rev=359584&r1=359583&r2=359584&view=diff
>
==
> ---
>
db/torque/runtime/trunk/src/java/org/apache/torque/dsfactory/JndiDataSourceFactory.
> java (original)
> +++
>
db/torque/runtime/trunk/src/java/org/apache/torque/dsfactory/JndiDataSourceFactory.
> java Wed Dec 28 10:36:21 2005
> @@ -48,7 +48,6 @@
> */
> public class JndiDataSourceFactory
> extends AbstractDataSourceFactory
> -implements DataSourceFactory
> {
>
> /** The log. */
> @@ -243,12 +242,12 @@
> {
> log.debug("InitialContext ---");
> Map env = ctx.getEnvironment();
> -Iterator qw = env.keySet().iterator();
> +Iterator qw = env.entrySet().iterator();
> log.debug("Environment properties:" + env.size());
> while (qw.hasNext())
> {
> -Object prop = qw.next();
> -log.debug("" + prop + ": " + env.get(prop));
> +Map.Entry entry = (Map.Entry)qw.next();
> +log.debug("" + entry.getKey() + ": " +
entry.getValue());
> }
> log.debug("--");
> }
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...
>
> Modified:
> db/torque/runtime/trunk/src/java/org/apache/torque/util/BasePeer.java
> URL: http://svn.apache.
>
org/viewcvs/db/torque/runtime/trunk/src/java/org/apache/torque/util/BasePeer.
> java?rev=359584&r1=359583&r2=359584&view=diff
>
==
> ---
> db/torque/runtime/trunk/src/java/org/apache/torque/util/BasePeer.
> java (original)
> +++
> db/torque/runtime/trunk/src/java/org/apache/torque/util/BasePeer.
> java Wed Dec 28 10:36:21 2005
> @@ -47,6 +47,7 @@
>
> import com.workingdogs.village.Column;
> import com.workingdogs.village.DataSet;
> +import com.workingdogs.village.DataSetException;
> import com.workingdogs.village.KeyDef;
> import com.workingdogs.village.QueryDataSet;
> import com.workingdogs.village.Record;
> @@ -259,8 +260,9 @@
> {
> statement.close();
> }
> -catch (SQLException ignored)
> +catch (SQLException e)
> {
> +throw new TorqueException(e);
> }
> }
> }
If an exception is thrown in a catch block, the finally block is executed,
and an exception is thrown again in the finally block, and the exception is
caught later on, the second exception is caught, and not the first
exception. For example:
try
{
try
{
throw new Exception("Exception in try block");
}
finally
{
throw new Exception("Exception in finally block");
}
}
catch (Exception e)
{
System.out.println(e.getMessage);
}
prints:
Exception in finally block
For diagnostic purposes, it would be much better to catch the first
exception, and this was the reason why the exception in the finally block
was ignored. On the other hand, if no exception occurs in the try block and
an Exception occurs in the finally block, one would like to catch the
exception from the finally block.
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).
> @@ -515,7 +510,15 @@
> // not the fully qualified name, insertOrUpdateRecord
> wants to use table as an index...
> BasePeer.insertOrUpdateRecord(rec, table, dbName, criteria);
> }
> -catch (Exception e)
> +catch (DataSetException e)
> +{
> +throwTorqueException(e);
> +}
> +catch (SQLException e)
> +{
> +throwTorqueException(e);
> +
