October 10, 2017

...Learn TDD with Codemanship

Manual Refactoring : Move Instance Method


I good deal of the refactoring I do involves shifting responsibilities between the classes in my code to try and minimise coupling between them.

When I see an example of Feature Envy - when a method of one class relies heavily on features of another class - I consider moving that method to where it arguably belongs.

Moving instance methods can be simple, or it can get complicated. Let's start with a simple example.



Here, the area() method of CarpetQuote only uses features of Room. It's only used internally within CarpetQuote, so if I moved it to Room no client code would break. Also, area() uses no features of CarpetQuote, so there'd be no need to create a dependency pointing back the other way.

To move an instance method, there needs to be an instance to move it to. This becomes the new target of any invocations. (i.e., self.area() becomes self.room.area() ).

First, I cut the area() method code and paste it into the Room class.



Then I change references to Room's getters to point at the equivalent internal fields of Room.



Then I change any invocations of area() in CarpetQuote to the new target instance, which i this case is the room field.



NOW RUN THE TESTS!

The accessors get_width() and get_length() are no longer being used, so I can inline these, moving our design into a more "Tell, Don't Ask" style.

As well as being able to move instance methods to fields, we can move them to method parameters. Imagine a slightly different version of our original example, where the Room is passed in to the area() method.



This time, when we move area() to the Room class, the target instance is the parameter value self.room. The invocation is turned around and we remove that parameter, so self.area(self.room) becomes self.room.area().



AND RUN THE TESTS!

Now let's make things a little more complicated. What if area() is called outside of CarpetQuote?

There are three potential solutions here:

1. Leave area() on CarpetQuote as a delegate to room.area()



(RUN THE TESTS!)

The advantage of this approach is when we need to preserve a public API. It leaves the client code untouched, though arguably is less clean as it creates obtuse indirection in our code. If this wasn't a public API, there might be a better way.

2. Expose the room field so the calling code can access it.



(AND RUN THE TESTS!)

Yuck. This is my least favourite solution, although it does work.

3. Switch invocation targets in the client code.



(RUN THE TESTS!)

This, in my opinion, is the cleanest solution. It requires a bit more work, but gets the client talking directly to the object it wants to talk to, and removes the unnecessary "middle man", making the code simpler.

Finally, we need to consider what to do when a method we want to move has internal dependencies.

Imagine when we calculate the area of carpet required, we need to subtract an uncarpeted border that the customer has asked for (i.e., not wall-to-wall carpet).



If we wanted to move area() to Room now, we could go about resolving the references to border in two ways.

1. Pass a reference to this instance of CarpetQuote as a parameter of area()



So Room can now resolve that reference.



(RUN THE TESTS!)

But this bi-directional dependency is less than ideal, and could create problems for us later.

2. Resolve the reference in CarpetQuote and pass the result.



(RUN THE TESTS!)

So that Room doesn't need to depend at all on CarpetQuote.







Posted 1 week, 5 days ago on October 10, 2017