Mailing-List: contact torque-dev-help@db.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Apache Torque Developers List" <torque-dev@db.apache.org>
Received-SPF: pass (asf.osuosl.org: local policy)
In-Reply-To: <20051228183644.56862.qmail@minotaur.apache.org>
Subject: Re: svn commit: r359584 - in
 /db/torque/runtime/trunk/src/java/org/apache/torque: ./ dsfactory/ manager/
 util/
To: "Apache Torque Developers List" <torque-dev@db.apache.org>
Message-ID: 
 <OFD21CD5AE.B95DBE60-ONC12570E7.003316F6-C12570E7.00372DD4@seitenbau.net>
From: Thomas Fischer <fischer@seitenbau.net>
Date: Fri, 30 Dec 2005 11:02:20 +0100
MIME-Version: 1.0
Content-type: text/plain; charset=US-ASCII


Nice clean-up. I have a few remarks, though:

tv@apache.org 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);
> +        }
> +        catch (TorqueException e)
>          {
>              throwTorqueException(e);
>          }

I would not use throwTorqueException() on a Torque exception, but simply
rethrow it.


> Modified:
> db/torque/runtime/trunk/src/java/org/apache/torque/util/Criteria.java
> URL: http://svn.apache.
>
org/viewcvs/db/torque/runtime/trunk/src/java/org/apache/torque/util/Criteria.

> java?rev=359584&r1=359583&r2=359584&view=diff
>
==============================================================================

> ---
> db/torque/runtime/trunk/src/java/org/apache/torque/util/Criteria.
> java (original)
> +++
> db/torque/runtime/trunk/src/java/org/apache/torque/util/Criteria.
> java Wed Dec 28 10:36:21 2005
> @@ -3090,13 +3090,13 @@
>
>          // Criteria.put() differs somewhat from Hashtable.put().
>          // This necessitates some corrective behavior upon
deserialization.
> -        for (Iterator iter = keySet().iterator(); iter.hasNext();)
> +        for (Iterator iter = entrySet().iterator(); iter.hasNext();)
>          {
> -            Object key = iter.next();
> -            Object value = get(key);
> +            Map.Entry entry = (Map.Entry)iter.next();
> +            Object value = entry.getValue();
>              if (value instanceof Criteria.Criterion)
>              {
> -                super.put(key, value);
> +                super.put(entry.getKey(), value);
>              }
>          }
> Modified:
> db/torque/runtime/trunk/src/java/org/apache/torque/util/VillageUtils.java
> URL: http://svn.apache.
>
org/viewcvs/db/torque/runtime/trunk/src/java/org/apache/torque/util/VillageUtils.

> java?rev=359584&r1=359583&r2=359584&view=diff
>
==============================================================================

> ---
> db/torque/runtime/trunk/src/java/org/apache/torque/util/VillageUtils.
> java (original)
> +++
> db/torque/runtime/trunk/src/java/org/apache/torque/util/VillageUtils.
> java Wed Dec 28 10:36:21 2005
> @@ -24,8 +24,8 @@
>  import java.math.BigDecimal;
>  import java.util.Hashtable;
>  import java.util.Iterator;
> +import java.util.Map;
>
> -import org.apache.torque.TorqueException;
>  import org.apache.torque.om.SimpleKey;
>
>  import com.workingdogs.village.QueryDataSet;
> @@ -113,18 +113,15 @@
>          throws Exception
>      {
>          Hashtable saveData = new Hashtable(hash.size());
> -        String key = null;
> -        Object value = null;
>          byte[] byteArray = null;
>
> -        Iterator keys = hash.keySet().iterator();
> +        Iterator keys = hash.entrySet().iterator();
>          while (keys.hasNext())
>          {
> -            key = (String) keys.next();
> -            value = hash.get(key);
> -            if (value instanceof Serializable)
> +            Map.Entry entry = (Map.Entry)keys.next();
> +            if (entry.getValue() instanceof Serializable)
>              {
> -                saveData.put(key, value);
> +                saveData.put(entry.getKey(), entry.getValue());
>              }
>          }
>

Just for the logs, same as above. What is better in iterating over the
entry set instead of the key set ?
Plus, if the iterator does not iterate over keys but over values, it should
be renamed from keys to values.

   Thomas


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