Hi there,

On 27/03/2007, at 1:36 AM, Andrus Adamchik wrote:

Splitting my comment on Lachlan's patch into smaller manageable pieces. See comments on ObjectContext inline...


Index: framework/cayenne-jdk1.4-unpublished/src/main/java/org/ apache/cayenne/BaseContext.java
===================================================================


+    public Persistent localObject(ObjectId id, Object prototype) {
+
+        if (id == null) {
+            throw new IllegalArgumentException("Null ObjectId");
+        }
+

+1, but note that there are some slight differences between DataContext and CayenneContext implementation. Don't recall all the details, but it would be nice to diff them and see what we can do about them.

There was no difference actually. The only diff was a comment in CayenneContext saying that this was a direct copy from DataContext and a comment in DataContext instructing people to remember to copy the implementation to CayenneContext and a TODO saying we need to find inheritance somehow... so it seemed the natural thing to do the todo.

Index: framework/cayenne-jdk1.4-unpublished/src/main/java/org/ apache/cayenne/ObjectContext.java
===================================================================


     /**
+     * Creates a nested context
+     */
+    ObjectContext createNestedContext();
+
+    /**
* Returns EntityResolver that stores all mapping information accessible by this
      * ObjectContext.
      */

-1: I think we should start with implementation of nested contexts on the client, before declaring it in the interface (i.e. before announcing it to the users). Also I've been doing some work as a part of CAY-739. So maybe this method will be moved to the *Utils class all together.

Fair enough. This is an area of particular interest to me (and ish in general). So I'd be keen to help with this where I can.

     /**
+ * Returns an object local to this ObjectContext and matching the ObjectId.
+     * <p>
+ * Unlike [EMAIL PROTECTED] #localObject(ObjectId, Object)}, this method will only do "mapping"
+     * (i.e. finding an object with the same id in this context).
+     * </p>
+     */
+    Persistent localObject(ObjectId id);

...

      /**
+ * Returns an object local to this ObjectContext from the given object.
+     * <p>
+ * This is essentially a convenience method for [EMAIL PROTECTED] #localObject(ObjectId)}.
+     * </p>
+     */
+    Persistent localObject(Persistent aPersistentObject);
+

-1: IMO the interface must be as lean as possible to simplify alt. implementations and decorators. So convenience (in other words derived from other) methods should preferably be defined outside the interface. I think this is such case.

Would localObject(Persistent) then be better as an interface than localObject(ObjectId,Object)? The latter is already possible via DataObjectUtils.objectForPK(ObjectContext,ObjectId). But the former seems more generally useful.

@@ -141,6 +163,14 @@
     void commitChangesToParent();

     /**
+ * Determines if there are any changes in this context that can be flushed to the + * parent DataChannel. i.e., determines if [EMAIL PROTECTED] #commitChangesToParent()} would
+     * flush any changes to the parent DataChannel or, likewise, if
+ * [EMAIL PROTECTED] #commitChanges()} has changes to cascade through the stack to the database.
+     */
+    boolean hasChanges();
+
+    /**
* Resets all uncommitted changes made to the objects in this ObjectContext, cascading
      * rollback operation all the way through the stack.
      */

+1: while it is somewhat redundant, it should allow implementors to optimize analysis of graph changes.

Great. It certainly makes the user's code a bit cleaner ;-)

with regards,
--

Lachlan Deck



Reply via email to