Re: svn commit: r819067 [1/3] - in /ofbiz/trunk/framework: base/src/org/ofbiz/base/util/ entity/src/META-INF/services/ entity/src/org/ofbiz/entity/

[Adam Heath-2]

[hidden email] wrote:

> Author: adrianc
> Date: Fri Sep 25 23:36:13 2009
> New Revision: 819067
>
> URL: http://svn.apache.org/viewvc?rev=819067&view=rev
> Log:
> Refactored GenericDelegator.java based on a design proposed by Adam Heath on the dev mailing list.
>
> DelegatorInterface.java has been replaced by Delegator.java.
>
>
> Added:
>     ofbiz/trunk/framework/base/src/org/ofbiz/base/util/Factory.java   (with props)
>     ofbiz/trunk/framework/entity/src/META-INF/services/org.ofbiz.entity.DelegatorFactory   (with props)
>     ofbiz/trunk/framework/entity/src/org/ofbiz/entity/Delegator.java   (with props)
>     ofbiz/trunk/framework/entity/src/org/ofbiz/entity/DelegatorFactory.java   (with props)
>     ofbiz/trunk/framework/entity/src/org/ofbiz/entity/DelegatorFactoryImpl.java   (with props)
> Removed:
>     ofbiz/trunk/framework/entity/src/org/ofbiz/entity/DelegatorInterface.java
> Modified:
>     ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilObject.java
>     ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java
>
> Added: ofbiz/trunk/framework/base/src/org/ofbiz/base/util/Factory.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/Factory.java?rev=819067&view=auto
> ==============================================================================
> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/Factory.java (added)
> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/Factory.java Fri Sep 25 23:36:13 2009
> @@ -0,0 +1,32 @@
> +package org.ofbiz.base.util;
> +
> +/** Factory interface. */
> +public interface Factory<T> {

public interface Factory<T, O>

> +
> +    /** Returns an instance of <code>T</code>.
> +     *
> +     * @param obj Optional object to be used in <code>T</code>'s construction,
> +     * or to be used as a selector key
> +     * @return An instance of <code>T</code>
> +     */
> +    public T getInstance(Object obj);

public T getInstance(O obj);

> +
> +}
>
> Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilObject.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilObject.java?rev=819067&r1=819066&r2=819067&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilObject.java (original)
> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilObject.java Fri Sep 25 23:36:13 2009
> @@ -23,6 +23,8 @@
>  import java.io.IOException;
>  import java.io.ObjectOutputStream;
>  import java.io.InputStream;
> +import java.util.Iterator;
> +import javax.imageio.spi.ServiceRegistry;
>  
>  /**
>   * UtilObject
> @@ -171,4 +173,17 @@
>          if (o1 == null) return 0;
>          return o1.hashCode();
>      }
> +
> +    @SuppressWarnings("unchecked")
> +    public static <T> T getObjectFromFactory(Class<T> factoryInterface, Object obj) throws ClassNotFoundException {

public static <T, O> getObjectFromFactory(Class<Factory<T, O>>
factoryInterface, O obj), then remove the cast below, and then
@SuppressWarnings.

> +        Iterator<Factory<T>> it = (Iterator<Factory<T>>) ServiceRegistry.lookupProviders(factoryInterface);
> +        while (it.hasNext()) {
> +            Factory<T> factory = it.next();
> +            T instance = factory.getInstance(obj);
> +            if (instance != null) {
> +                return instance;
> +            }
> +        }
> +        throw new ClassNotFoundException(factoryInterface.getClass().getName());
> +    }
>  }
>
> Added: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/Delegator.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/Delegator.java?rev=819067&view=auto
> ==============================================================================
> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/Delegator.java (added)
> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/Delegator.java Fri Sep 25 23:36:13 2009
> +public interface Delegator {
> +    /**
> +     * Finds all Generic entities NOTE 20080502: 14 references; all changed to
> +     * findList
> +     *
> +     * @param entityName
> +     *            The Name of the Entity as defined in the entity XML file
> +     * @return List containing all Generic entities
> +     * @deprecated Use findList() instead
> +     */
> +    @Deprecated
> +    public List<GenericValue> findAll(String entityName) throws GenericEntityException;

Don't add @Deprecated methods to *new* interfaces or classes.  That
makes no sense.

Also, interfaces don't need the public keyword attached to methods;
it's superflous.

[Adrian Crum]

Adam,

Thanks for the tips!

The public modifier and @Deprecated in the Delegator interface are
carried over from GenericDelegator.java. I used Eclipse to extract the
interface - hence the public modifier. David Jones deprecated the
methods - not me. We can remove the deprecated methods when they are no
longer used.

-Adrian

Adam Heath wrote:

> [hidden email] wrote:
>> Author: adrianc
>> Date: Fri Sep 25 23:36:13 2009
>> New Revision: 819067
>>
>> URL: http://svn.apache.org/viewvc?rev=819067&view=rev
>> Log:
>> Refactored GenericDelegator.java based on a design proposed by Adam Heath on the dev mailing list.
>>
>> DelegatorInterface.java has been replaced by Delegator.java.
>>
>>
>> Added:
>>     ofbiz/trunk/framework/base/src/org/ofbiz/base/util/Factory.java   (with props)
>>     ofbiz/trunk/framework/entity/src/META-INF/services/org.ofbiz.entity.DelegatorFactory   (with props)
>>     ofbiz/trunk/framework/entity/src/org/ofbiz/entity/Delegator.java   (with props)
>>     ofbiz/trunk/framework/entity/src/org/ofbiz/entity/DelegatorFactory.java   (with props)
>>     ofbiz/trunk/framework/entity/src/org/ofbiz/entity/DelegatorFactoryImpl.java   (with props)
>> Removed:
>>     ofbiz/trunk/framework/entity/src/org/ofbiz/entity/DelegatorInterface.java
>> Modified:
>>     ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilObject.java
>>     ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java
>>
>> Added: ofbiz/trunk/framework/base/src/org/ofbiz/base/util/Factory.java
>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/Factory.java?rev=819067&view=auto
>> ==============================================================================
>> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/Factory.java (added)
>> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/Factory.java Fri Sep 25 23:36:13 2009
>> @@ -0,0 +1,32 @@
>> +package org.ofbiz.base.util;
>> +
>> +/** Factory interface. */
>> +public interface Factory<T> {
>
> public interface Factory<T, O>
>
>> +
>> +    /** Returns an instance of <code>T</code>.
>> +     *
>> +     * @param obj Optional object to be used in <code>T</code>'s construction,
>> +     * or to be used as a selector key
>> +     * @return An instance of <code>T</code>
>> +     */
>> +    public T getInstance(Object obj);
>
> public T getInstance(O obj);
>
>> +
>> +}
>>
>> Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilObject.java
>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilObject.java?rev=819067&r1=819066&r2=819067&view=diff
>> ==============================================================================
>> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilObject.java (original)
>> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilObject.java Fri Sep 25 23:36:13 2009
>> @@ -23,6 +23,8 @@
>>  import java.io.IOException;
>>  import java.io.ObjectOutputStream;
>>  import java.io.InputStream;
>> +import java.util.Iterator;
>> +import javax.imageio.spi.ServiceRegistry;
>>  
>>  /**
>>   * UtilObject
>> @@ -171,4 +173,17 @@
>>          if (o1 == null) return 0;
>>          return o1.hashCode();
>>      }
>> +
>> +    @SuppressWarnings("unchecked")
>> +    public static <T> T getObjectFromFactory(Class<T> factoryInterface, Object obj) throws ClassNotFoundException {
>
> public static <T, O> getObjectFromFactory(Class<Factory<T, O>>
> factoryInterface, O obj), then remove the cast below, and then
> @SuppressWarnings.
>
>> +        Iterator<Factory<T>> it = (Iterator<Factory<T>>) ServiceRegistry.lookupProviders(factoryInterface);
>> +        while (it.hasNext()) {
>> +            Factory<T> factory = it.next();
>> +            T instance = factory.getInstance(obj);
>> +            if (instance != null) {
>> +                return instance;
>> +            }
>> +        }
>> +        throw new ClassNotFoundException(factoryInterface.getClass().getName());
>> +    }
>>  }
>>
>> Added: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/Delegator.java
>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/Delegator.java?rev=819067&view=auto
>> ==============================================================================
>> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/Delegator.java (added)
>> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/Delegator.java Fri Sep 25 23:36:13 2009
>> +public interface Delegator {
>> +    /**
>> +     * Finds all Generic entities NOTE 20080502: 14 references; all changed to
>> +     * findList
>> +     *
>> +     * @param entityName
>> +     *            The Name of the Entity as defined in the entity XML file
>> +     * @return List containing all Generic entities
>> +     * @deprecated Use findList() instead
>> +     */
>> +    @Deprecated
>> +    public List<GenericValue> findAll(String entityName) throws GenericEntityException;
>
> Don't add @Deprecated methods to *new* interfaces or classes.  That
> makes no sense.
>
> Also, interfaces don't need the public keyword attached to methods;
> it's superflous.
>
>

[Adam Heath-2]

Adrian Crum wrote:
> Adam,
>
> Thanks for the tips!
>
> The public modifier and @Deprecated in the Delegator interface are
> carried over from GenericDelegator.java. I used Eclipse to extract the
> interface - hence the public modifier. David Jones deprecated the
> methods - not me. We can remove the deprecated methods when they are no
> longer used.

Nono.

You've added a *new* interface.  Adding immediately deprecated methods
to a *new* interface or class is stupid.

I'm not suggesting removing the methods from GenericDelegator.  Just
the definitions from the interface.

[Adrian Crum]

I understand what you're suggesting. But if the intention is to replace

GenericDelegator delegator = GenericDelegator.getDelegator(delegatorName);

with

Delegator delegator = (Delegator)
UtilObject.getObjectFromFactory(DelegatorFactory.class, delegatorName);

then the Delegator interface will need to support the same methods as
GenericDelegator.

-Adrian

Adam Heath wrote:

> Adrian Crum wrote:
>> Adam,
>>
>> Thanks for the tips!
>>
>> The public modifier and @Deprecated in the Delegator interface are
>> carried over from GenericDelegator.java. I used Eclipse to extract the
>> interface - hence the public modifier. David Jones deprecated the
>> methods - not me. We can remove the deprecated methods when they are no
>> longer used.
>
> Nono.
>
> You've added a *new* interface.  Adding immediately deprecated methods
> to a *new* interface or class is stupid.
>
> I'm not suggesting removing the methods from GenericDelegator.  Just
> the definitions from the interface.
>

[Adam Heath-2]

Adrian Crum wrote:

> I understand what you're suggesting. But if the intention is to replace
>
> GenericDelegator delegator = GenericDelegator.getDelegator(delegatorName);
>
> with
>
> Delegator delegator = (Delegator)
> UtilObject.getObjectFromFactory(DelegatorFactory.class, delegatorName);
>
> then the Delegator interface will need to support the same methods as
> GenericDelegator.

Nope.  Leave GenericDelegator.getDelegator, which returns
GenericDelegator.  This way, all existing code continues to work, no
changes at all.  Then, existing scripts can be modified one at a time.

Base helper methods should be modified first, so that they take a
Delegator instead of a GenericDelegator.  This will just require a
recompile(as calling code will still be using a GenericDelegator,
which implements Delegator).

Basically, find all methods that have an incoming GenericDelegator
object, but do *not* forward it on to any other method(be careful
about services that call out).  Modify those methods first.  Then move
slowly up the chain.