Short Refactoring Tip: let the object do it for you

[Ovid]

I was working on some code today when I noticed the following two conditionals:

if ($po->status->name eq 'Pending') {
    # po can be sent
}

# later

if ($po->status->name eq 'Pending' || $po->status->name eq 'Sent') {
    # po can be cancelled
}

Ugh! What horrible code. If we need to change those names, this code will be break pretty quickly. We'll also have to hunt all over our code base for instances of code like this. If we need to check the value of an object's properties, push it back into the object.

if ($po->can_send) {
    # po can be sent
}

# later

if ($po->can_cancel) {
    # po can be cancelled
}

Much nicer :)

Cheers,
Ovid

New address of my CGI Course.
Silence is Evil (feel free to copy and distribute widely - note copyright text)

[dws]

If we need to change those names, this code will be break pretty quickly.

Not only that, but forcing a client to write

  if ($po->status->name eq 'Pending') { ...

requires them to know that a purchase order has a status, and that the status has a name. Both of those details are probably inappropriate for client code that deals with purchase orders to have to know. It also makes purchase orders harder to unit test, since you then have to either unit test a Status at the same time, or rig up mock objects to use in places of a Status. By structuring the API like

  if ($po->can_send) { ...

you reduce the complexity of testing.

There's a design principle--whose name I'm blanking on (Demeter's Law -- thanks, lachoy)--that says that if you find yourself writing expressions that navigate more than one level into an object structure, the top-level object needs a different protocol. In the case above, can_send is that new protocol, and status can be made private (e.g., by renaming it to _status).

[lachoy]

That's the Law of Demeter, or at least part of it.

Chris
M-x auto-bs-mode

[demerphq]

Id like to point out that this doesn't scale very well. Sure you have a valid point in a limited case, such as when there are only a few relevent states. But what happens when there are lots? Or the system is dynamic? Or if you want to say things like

if ($po->status->name =~ /^(sent|pending|waiting|procratinating|delaye
+d)$/i) {

I'd rather write the above than call a bunch of methods, let alone write the methods in the first place. :-)

I thinks its unlikely I would write an object that require on a regular basis the double indirection like that. And if I did face a situation like my example above I'd probably do something more like

my $status=$pos->status;
if ($status->is('Pending')) {}
if ($status->in(qw(sent pending waitning procrastinating delayed)) { }

The issue here for me is that the use of 'Pending' isn't wrong, its the use of 'Pending' in such a way that I can't intercept that usage that is wrong. The subs above allow use to redefine the back end behaviour just as yours does, but its a lot more flexible an approach, while still providing us a way to block inapropriate checks. For instance $status->is('Pneding') should produce an error. Albeit a run time one. Oh well you cant have everything can you? :-)

Anyway, I use both styles as seems appropriate for the task at hand, often both at the same time even. :-)


---
demerphq

_{<Elian> And I do take a kind of perverse pleasure in having an OO assembly language...}

[hardburn]

I'd probably work that out as a dispatch table instead:

my %STATES = (
    sent    => sub { . . . }, 
    pending => sub { . . . }, 
    waiting => sub { . . . }, 
    procrastinating => sub { . . . }, 
    delayed => sub { . . . }, 
);

# Make 'pending' and 'waiting' the same thing
$STATES{pending} = $STATES{waiting};

# Later

$STATES{$status->state->name}->();

It's faster than a speeding regex. And I personally think dispatch tables are sexy.

----
I wanted to explore how Perl's closures can be manipulated, and ended up creating an object system by accident.
-- Schemer

Note: All code is untested, unless otherwise stated

[dws]

I'd like to point out that this doesn't scale very well. ...

if ($po->status->name =~ /^(sent|pending|waiting|procratinating|delaye
+d)$/i) {

I'd rather write the above than call a bunch of methods, let alone write the methods in the first place. :-)

But you (well, I) wouldn't write individual accessor methods for all of those status names. Instead, write a predicate (a method that returns true or false) that describes what that combination of states means in terms of the behavior that a PurchaseOrder object exports, and in terms of the protocol that clients of PurchaseOrder expect to speak to it with. It's a lot easier to write unit tests that way, since you don't have to worry about arbitrary combinations of status bits. The valid combinations are hidden behind testable predicates. In fact, starting from predicates and working backwards to status values is one way to ensure that you don't have status values that you don't really need.

[crenz]

if ($po->status->name =~ /^(sent|pending|waiting|procratinating|delayed)$/i) {

Shouldn't that be "procrastinating"? That typo already shows the dangers that lurk in this approach :).

[castaway]

I would have thought this was common knowledge/sense, but maybe that's just me :) (Different programming/logic backgrounds etc.) Generally its good to reduce such things to the essentials that will be needed by anything calling the object, and not make them know how it works internally. (The class interface to the outside world, that is ,)


C. *forgets we're dealing with newbie programmers here quite often*

[dragonchild]

Ah, but it isn't common sense. In old-time C, for example, you wouldn't write something like that. Instead, you would write something like:

#define STATUS1 (1 << 0)
#define STATUS2 (1 << 1)
#define STATUS3 (1 << 2)
#define STATUS4 (1 << 3)
...
typedef int Status;
...
Status product_status = 0;
...
if (product_status & (STATUS1 | STATUS2))
{
// Do stuff here
}

It's imperative programming, man! You gotta be in control or you lose your job! (Think about it - if you couldn't point to incomprehensible code, your PHB would find someone who could. I mean, if anyone could write code that works, then how do you justify your 6-figure consulting fee?)

Also, writing good class interfaces requires both time and understanding of the business domain. Maybe your world allows for these, but no application I've ever had the (mis)fortune to work on has ever been in that most blessed of states. (No, that isn't Texas, either.)

------
We are the carpenters and bricklayers of the Information Age.

Don't go borrowing trouble. For programmers, this means Worry only about what you need to implement.

Please remember that I'm crufty and crochety. All opinions are purely mine and all code is untested, unless otherwise specified.

[castaway]

I have no idea what common sense has to do with C.. (Or rather why something shouldn't be, just because someone did it in C differently).


(For the record, I avoid C/C++ like the plague, or try to..)
Not everyone *can* write code that works, much less code that is maintainable, usable etc. Obfuscating for the sake of it never appealed to me.
We do take time here and define our interfaces (between products+components) - it saves lots of hassle later.


C.

[mvc]

You are on the right road, but it seems as if nobody has mentioned your solution is not good OO design:

BAD:           if ($po->status->name eq 'Sent') ...
ALMOST AS BAD: if ($po->can_send) ...
GOOD:          $po->send

You are breaking encapsulation in all but the last case. Getters are a design smell. They are just a way to make private data public.

Instead of asking objects questions, tell them to do things. The object will figure out how to do it.

Allen Holub says about this:

"Never ask an object for the information you need to do 
something; rather, ask the object that has the information 
to do the work for you."

[Ovid]

That's a very interesting observation. Given what you've written, how would you handle this?

[% IF item.can_receive %]
    <input type="text" name="received" value="[% item.received %]" cla
+ss="input2"/>
[% ELSE; 
       item.received; 
   END %]

In the above case, I am querying whether or not an item can accept a value for given attribute (those are Template Toolkit directives). If so, an input box is presented to the end user, with the current value filled in. If not, they only see the value. As this data can be represented in many other ways (and not just on HTML forms), it doesn't seem to make sense to have the object present HTML code to the end user. I like the idea of moving the conditional logic into the object, but I don't see a clean way of doing that.

Cheers,
Ovid

New address of my CGI Course.
Silence is Evil (feel free to copy and distribute widely - note copyright text)

[mvc]

Here we have a classic conflict of design forces:

• Encapsulation
• Seperation of concerns (cohesion)

If you put the UI code in the object, you are encapsulated but your object is now doing its regular work + UI work (low cohesion). If you keep it as is, you get clean seperation of concerns, but break encapsulation.

There are other solutions that could balance these forces better. The balance of forces you want depends on your requirements.

You could, for example, have the template code look like this:

<% item.getReceivedUI("<input ...>") %>

The item object getReceivedUI() will check if receive is possible, if so interpolate the parameter as a TT template with item.received in the stash. If not, it returns item.received. Then you remove the UI code from the item object by delegating the actual interpolation to some strategy object, that may use TT or anything. Thus the item knows not about UI, just about presentation logic. The template is still in charge of the UI. Is this extra work worth it? Depends on your requirements.

[dws]

BAD:           if ($po->status->name eq 'Sent') ...
ALMOST AS BAD: if ($po->can_send) ...
GOOD:          $po->send

You are breaking encapsulation in all but the last case.

How, pray tell, does a predicate function "break encapsulation"? can_send is not a getter; it reveals nothing about how the purchase order answers the question. Encapsulation is quite safely preserved.

[mvc]

Do you define "predicate function" here as "method that returns a boolean value"? If so, then it reveals something about the object state- information that should be encapsulated.

What if you decide that "you can always send!". Then you have to remove the conditionals from all clients of your object. Or what if there are now 3 states for sending, each requiring a different send method? We want the clients to know as little as possible about the object. Knowing only one method, is better than knowing a method, its return type, and another method.

"...The key question is: How much of one module must be 
known in order to understand another module?..."

     Yourdon, Ed and Larry L. Constantine. Structured Design: Fundamentals of a Discipline of Computer Program and Systems Design. (1979) On Coupling, p.85

Breaking encapsulation here increases coupling between the clients of the object and the object itself. Note I am not saying: never use boolean methods. There may be other design forces besides "encapsulate hermetically", so we do not know which solution is best here.

Code like: "if an object can do this, tell it to do it" smells.