🚀 go-pugleaf

Andy Bulka wrote:
> I've just been using the "Change Unidirectional Association to
> Bidirectional" on p199 of M. Fowler's Refactoring book.
[snip]
> So anyway, turning our attention back to the ONE-to-many (which ends up not
> being so elegant, once you flesh it all out - that's my claim...)   I went
> ahead and invented/implemented the 'missing' removeOrder() method  thus: 

[minor corrections to the example method]
> class Customer ...
>  void removeOrder( Order arg ) {
>   _orders.remove( arg );
>   arg._customer = null;
> }

actually you should be able to do the above with:
    void removeOrder( Order order ) {
        order.setCustomer(null);
    }

which would keep all the association-controlling logic in the
'setCustomer' method.  Since this is a nice simple one-liner, it would
be good if it was in the book, but it was 430 pages as is :-) and he may
have thought the one-liner was apparent.

--Mark
mark.fussell@chimu.com

  i   ChiMu Corporation      Architectures for Information
 h M   info@chimu.com         Object-Oriented Information Systems
C   u    www.chimu.com         Architecture, Frameworks, and Mentoring

abulka@my-deja.com wrote:
> Mark Fussell <mark.fussell@chimu.com> wrote:
> > actually you should be able to do the above with:
> >
> >     void removeOrder( Order order ) {
> >         order.setCustomer(null);
> >     }
> >
> > which would keep all the association-controlling logic in the
> > 'setCustomer' method.
> 
> I don't think this would work because the Order class / controlling
> side is not being told WHICH customer to no longer point to.  The hash
> set _orders that lives in Customer needs to be updated, somehow.
> 
> Which was exactly my point, the Customer side needs to be:
> 
>  void removeOrder( Order order ) {
>   _orders.remove( order )   // this line is inelegant, but needed.
>   order.setCustomer(null);  // this line is ok, calls controlling side
>  }

Please review the pattern's example code again or simply try out the
version I gave.  Order::setCustomer already contains the functionality
you are trying to put into 'removeOrder' -- it already knows how to
remove itself from its (only) customer's collection of orders.  Remember
that an Order can only have a single customer because of the cardinality
rules (Martin gave a different example for the many-to-many
implementation).  

I am not saying you can't do it multiple ways, but I would say the
simple:
   void removeOrder( Order order ) {
      order.setCustomer(null);
   }
works correctly and is probably in the spirit of what Martin was trying
to show.  

Now I would agree that you might want to enhance the above to do a
simple integrity check:

     /**
      *@require order.customer = self
      */
   void removeOrder( Order order ) {
      order.setCustomer(null);
   }

Since it is a bit peculiar to allow a customer to clear out an order
that wasn't for that customer.  You could also simply ignore a request
to remove an order that doesn't belong to this customer if you want to
be more tolerant.  

   void removeOrder( Order order ) {
      if (!_orders.contains(order)) return;
      order.setCustomer(null);
   }

But in either case, Order::setCustomer is still controlling the
association.

--Mark
mark.fussell@chimu.com

 Mark Fussell <mark.fussell@chimu.com> wrote:
> actually you should be able to do the above with:
>
>     void removeOrder( Order order ) {
>         order.setCustomer(null);
>     }
>
> which would keep all the association-controlling logic in the
> 'setCustomer' method.

I don't think this would work because the Order class / controlling
side is not being told WHICH customer to no longer point to.  The hash
set _orders that lives in Customer needs to be updated, somehow.

Which was exactly my point, the Customer side needs to be:

 void removeOrder( Order order ) {
  _orders.remove( order )   // this line is inelegant, but needed.
  order.setCustomer(null);  // this line is ok, calls controlling side
 }

-Andy


Sent via Deja.com http://www.deja.com/
Before you buy.

 Mark Fussell <mark.fussell@chimu.com> wrote:
> Order::setCustomer already contains the functionality
> you are trying to put into 'removeOrder' -- it already knows how to
> remove itself from its (only) customer's collection of orders.

Well - let's see if it really does.  The code from the Refactoring
book, p.199 for setCustomer reads:

Class Order...
 void setCustomer (Customer arg) ...
   if (_customer != null) _customer.friendOrders().remove(this);
   _customer = arg;
   if (_customer != null) _customer.friendOrders().add(this);
}

So given we are using the 'elegant' one liner version of removeOrder
> Class Customer...
>    void removeOrder( Order order ) {
>       order.setCustomer(null);
>    }

Notice that when we pass null to order.setCustomer, then setCustomer
skips the add/remove calls!  This results in the Customer object's hash
set STILL POINTING to the order object, even though the order object is
no longer pointing to the customer.

The only way to keep our elegant one liner version of removeOrder, and
keep the spirit of what Martin was trying to show viz. have the Order
class controlling the association, I think, is to make setCustomer a
little smarter.

Class Order...
 void setCustomer (Customer arg) ...

   if (arg = null) and (_customer <> null)   // EXTRA LOGIC
      _customer.friendOrders().remove(this);

   if (_customer != null) _customer.friendOrders().remove(this);
   _customer = arg;
   if (_customer != null) _customer.friendOrders().add(this);
}

Now admittedly, Martin Fowler says in the text that "The exact code in
the controlling modifier varies with the multiplicity of the
association.  If the customer is not allowed to be null, I can forgo
the null checks, but I need to check for a null argument."

In our case, customer is allowed to be null, so we need to keep the
null checks. And as it turns out, we need to "check for a null
argument", though that is only one third the story - the extra logic I
added, above, hopefully fills out the rest of the story.

-Andy Bulka
WindowWare Multimedia
ATUG


Sent via Deja.com http://www.deja.com/
Before you buy.

abulka@my-deja.com wrote:
>  Mark Fussell <mark.fussell@chimu.com> wrote:
> > Order::setCustomer already contains the functionality
> > you are trying to put into 'removeOrder' -- it already knows how to
> > remove itself from its (only) customer's collection of orders.
> 
> Well - let's see if it really does.  The code from the Refactoring
> book, p.199 for setCustomer reads:
> 
> Class Order...
>  void setCustomer (Customer arg) ...
>    if (_customer != null) _customer.friendOrders().remove(this);
>    _customer = arg;
>    if (_customer != null) _customer.friendOrders().add(this);
> }
> 
> So given we are using the 'elegant' one liner version of removeOrder
> > Class Customer...
> >    void removeOrder( Order order ) {
> >       order.setCustomer(null);
> >    }
> 
> Notice that when we pass null to order.setCustomer, then setCustomer
> skips the add/remove calls!  ...

No it doesn't.  Are you actually testing this out?  In a transcript of
VisualAge or a little testing main for a command-line?  I again suggest
you do, but will walk through the code line by line to help the process.

     void setCustomer (Customer arg) {
        if (_customer != null) _customer.friendOrders().remove(this);
         _customer = arg;
        if (_customer != null) _customer.friendOrders().add(this);
    }

Coming into the method, '_customer' is not null: it contains the
customer the order is currently linked with.  So the first line
(order-removal) will execute.  Then the second line will execute and
'_customer' is set to 'null'.  Finally the third line does not execute
because '_customer' is now null.  So given a 'null' argument to
'setCustomer' the execution behavior is equivalent to:

     void setCustomer (Customer arg) {
        _customer.friendOrders().remove(this);
         _customer = arg;
     }

This is exactly what we want and does not require your extra additions. 
Note that your last suggestion:
>    if (arg = null) and (_customer <> null)   // EXTRA LOGIC
>       _customer.friendOrders().remove(this);
> 
>    if (_customer != null) _customer.friendOrders().remove(this);

will cause TWO calls to remove 'this' order from the customer if
'arg==null'.  Considering that you keep making syntax errors (there are
two in the above snippet), you must not be trying this out.  Why not
simply let the computer show you how the code really works and then
build a deeper understanding from there?  Walk a debugger trace or put
in a bunch of print statements.  Or you could create some postcondition
check or XP-style unit tests to verify that the postcondition is true:

     /**
      *@require order.customer = self
      *@ensure  !self.orders.contains(order) && order.customer = null
      */
   void removeOrder( Order order ) {
      order.setCustomer(null);
   }

Good luck...

--Mark
mark.fussell@chimu.com

 Mark Fussell <mark.fussell@chimu.com> wrote:
> No it doesn't.  Are you actually testing this out?

You are right.  Martin Fowler is right.  I am wrong.

What I have been doing is testing in Object Pascal and translating back
to Java - hence the little syntax errors you have been picking up.

In my tests I was using an INCORRECT version of setCustomer that was
testing the incoming argument 'arg', rather than the private _customer
field.  Viz:

 void setCustomer (Customer arg) {
   if (arg != null) _customer.friendOrders().remove(this);
   _customer = arg;
   if (arg != null) _customer.friendOrders().add(this);
 }

rather than the original, correct:

 void setCustomer (Customer arg) {
   if (_customer != null) _customer.friendOrders().remove(this);
   _customer = arg;
   if (_customer != null) _customer.friendOrders().add(this);
 }

(All I can say in my defence is that because the Java language
distinguishes and encourages usage like "Order order _order"
and "Customer customer _customer", I need to be more careful in the
future.  In object pascal I am used to TCustomer Customer fCustomer)

Anyway, now that this has been brought to my attention, and re-tested,
I now see the simple beauty of the original Fowler code.  In fact, the
setCustomer algorithm is perfectly described in the Refactoring book
text p199: "first tell the other object to remove its pointer to you,
set your pointer to the new object, and then tell the new object to add
a pointer to you."

> Good luck...

Thanks for your patience and assistance,
-Andy



Sent via Deja.com http://www.deja.com/
Before you buy.

abulka@my-deja.com wrote:
> You are right.  Martin Fowler is right.  I am wrong.

Phew... things were getting a little strange there for a while... :-)

[SNIP]
> (All I can say in my defence is that because the Java language
> distinguishes and encourages usage like "Order order _order"
> and "Customer customer _customer", I need to be more careful in the
> future.  In object pascal I am used to TCustomer Customer fCustomer)

Well, to give some concurring feedback, I personally do not like the
'_order' convention and use a different one ('myOrder') on my projects. 
Also, someone to whom I showed the example had a few problems with it
too (although that may be because I printed it with a 6pt font). 
Changing conventions the code would look like this:

    public void setCustomer (Customer customer) {
        if (myCustomer != null) myCustomer.friendOrders().remove(this);
        myCustomer = customer;
        if (myCustomer != null) myCustomer.friendOrders().add(this);
    }

That may or may not have helped...  It may just take a while to get used
to slightly different syntax and idioms.  But another item you
indirectly suggest may have helped quite a bit...

> ... In fact, the
> setCustomer algorithm is perfectly described in the Refactoring book
> text p199: "first tell the other object to remove its pointer to you,
> set your pointer to the new object, and then tell the new object to add
> a pointer to you."

If Martin had factored the method to match exactly what he wrote, we
would instead get:

    public void setCustomer (Customer customer) {
        unlinkFromCurrentCustomer();
        myCustomer = customer;
        linkToCurrentCustomer();
    }

    protected void unlinkFromCurrentCustomer() {
        if (myCustomer != null) myCustomer.friendOrders().remove(this);
    }

    protected void linkToCurrentCustomer() {
        if (myCustomer != null) myCustomer.friendOrders().add(this);
    }

This is now very descriptive, matches the narrative exactly, and is very
unlikely to allow a misreading or miscopying.  Although it would be
somewhat unusual (i.e. not main-stream) to have methods this short in
Java, that is no reason to not use this approach when it would help
readability and maintainability.  The above is a very Smalltalk (i.e.
Superior :-) way of writing code, and I highly suggest getting [Beck 97]
for examples of this type of cleanup.  Although you would have to deal
with another syntax you may not be familiar with, the patterns/idioms
and transformations described in [Beck 97] are universal to all OO
languages.

> Thanks for your patience and assistance,

Your welcome :-)

--Mark
mark.fussell@chimu.com

[Beck 97]  Smalltalk Best Practice Patterns, Kent Beck, 
           Prentice Hall... 1997. ISBN: 0-13-476904-X.

On Sun, 14 Nov 1999 15:47:01 -0800, Mark Fussell <mark.fussell@chimu.com>
wrote:


> If Martin had factored the method to match exactly what he wrote, we
> would instead get:
> 
>     public void setCustomer (Customer customer) {
>         unlinkFromCurrentCustomer();
>         myCustomer = customer;
>         linkToCurrentCustomer();
>     }
> 
>     protected void unlinkFromCurrentCustomer() {
>         if (myCustomer != null) myCustomer.friendOrders().remove(this);
>     }
> 
>     protected void linkToCurrentCustomer() {
>         if (myCustomer != null) myCustomer.friendOrders().add(this);
>     }
> 
> This is now very descriptive, matches the narrative exactly, and is very
> unlikely to allow a misreading or miscopying.  Although it would be
> somewhat unusual (i.e. not main-stream) to have methods this short in
> Java, that is no reason to not use this approach when it would help
> readability and maintainability.  The above is a very Smalltalk (i.e.
> Superior :-) way of writing code, and I highly suggest getting [Beck 97]
> for examples of this type of cleanup.  Although you would have to deal
> with another syntax you may not be familiar with, the patterns/idioms
> and transformations described in [Beck 97] are universal to all OO
> languages.

Since this thread has touched on readability issues that might affect
maintenance, and also touched on alternative languages like Object Pascal and
Smalltalk, I thought I'd post the equivalent snippet of Dylan for general
interest:

8<----------------------------------------------------------------------

define method set-customer (order :: <order>, customer :: <customer>) => ()
  unlink-from-current-customer(order);
  my-customer(order) := customer;
  link-to-current-customer(order);
end method;

define method unlink-from-current-customer (order :: <order>) => ()
  if (my-customer(order))
    remove!(friend-orders(my-customer(order)), order);
  end if;
end method;

define method link-to-current-customer (order :: <order>) => ()
  if (my-customer(order))
    add!(friend-orders(my-customer(order)), order);
  end if;
end method;

8<----------------------------------------------------------------------

Some points about the code: 

+ Dylan used begin/end style syntax instead of {} to help readability. The bit
after the word "end" is optional. You can say "end", "end method", or even
"end method link-to-current-customer" but the latter _is_ checked if you do
use that form.

+ Dylan is case-insensitive so case cannot be used to distinguish identifiers.
Extra characters _must_ be used.

+ Dylan identifiers can contain a wider variety of characters than C or Java.
Current conventions that help provide visual cues include <> for classes, and
! for some destructive operations. "-" is conventionally used to separate
words within identifiers for readability.

+ Dylan uses ":=" for assignment to avoid confusion with equality testing.

+ Dylan has generalized booleans as well as an explicit <boolean> type. This
means that conditional expressions evaluating to non-false values are treated
as true. eg "if (my-customer(order))" is equivalent to
"if (my-customer(order) ~= #f)". "#f" is Dylan's literal false value. #f is
general used were "null" would be in languages like Java.

+ "x :: y" means "x has type y". This used in the method syntax for
specializing arguments and variable binding syntax. Dylan is dynamically typed
and most type declarations are optional. If present they help the compiler
produce more efficient code and/or insert checks.

+ "=> ()" means there are no return values. Dylan methods can return multiple
values. This declaration is optional.

+ Dylan uses a generalization of message-passing to multiple arguments. A
corollary of this is that all arguments are explicit. There is no hidden
"this" parameter.

+ Dylan has a "." notation for calling functions e.g. "order.my-customer" and
"my-customer(order)" are equivalent, but I've only used the latter above.

+ "Fields" (called "slots" in Dylan) cannot be accessed outside of the object
system as if they are variables. Instead all accesses must go via accessor
methods. E.g. the field access "myCustomer" in Java becomes
"my-customer(order)" in Dylan. This means field access and local variables
can't get confused.

There's more about Dylan at http://www.functionalobjects.com

__Jason