Beefy Boxes and Bandwidth Generously Provided by pair Networks
The stupid question is the question not asked

Programming patterns

by McA (Priest)
on Jun 22, 2012 at 10:00 UTC ( #977817=perlmeditation: print w/replies, xml ) Need Help??

Hi all

chromatic has an interesting blog entry. And Piers Cawley commented it in a way that raised my interest. But I have to admit that I didn't understand the comment completely. So I wrote an email to him asking whether he could explain it a little bit more. I'm sure my mail was lost in the big spam space.

So I want to ask or discuss the three mentioned patterns which have in his opinion code smell and are signs to refactor the code. I want to understand why this is the case. This is more related to object orientation than Perl by itself.

Has anyone more insight than me?

Best regards

Replies are listed 'Best First'.
Re: Programming patterns (cohesion)
by tye (Sage) on Jun 22, 2012 at 16:29 UTC

    Replacing an "if" statement with inheritance is Kool-Aid smell. I've seen it done. I've seen the really bad consequences several times (code that is much harder to understand or to even find the right code to read and tying things in knots so fixing a minor problem requires serious restructuring). So don't interpret that as the lesson.

    But I'm not sure why people express the good lesson as "replace 'if'". The good lesson is to keep the logic about an object inside the class. Having logic outside the class means that you have to repeat that logic each time you use the class and that it is harder to fix the logic or change the logic. It makes the code that uses the class more complex and it makes the class harder to understand because you have to go read places that use it in order to see some of the logic. And it requires people get this logic right each time they use the class, which is a waste of energy and encourages bugs.

    The bad design smell for me is when you can detect non-trivial knowledge of the class' structure in code outside of the class.

    For example, we have blacklists and whitelists. A good design would be to have the request-handling code look like:

    if( is_blacklisted($source,$dest,$acct,$parent,$optout) ) {

    But our code currently has a big block of logic inside the method for handling requests that demonstrates a ton of knowledge about how the blacklists and whitelists are organized (there are global lists and per-account lists, etc.). That block of code calls two other methods in the request-handling class: one checks against a passed-in list of whitelists, the other checks against a passed-in list of blacklists.

    There are several problems with that.

    1. There is a bloat of code inside the already-too-complex request-handling method. That should just be a single method call.
    2. There are two methods that are specific to blacklists/whitelists inside the method-handling class. Those should be in a different class.
    3. The code for setting up these lists is not in the same class as the code for consuming these lists.
    4. This structure doesn't actually include an interface to the lists so you can't easily tell which inputs determine if a request gets blacklisted (the source, the destination, the destination's account, the parent of that account, and whether the destination opted out of the global blacklist).
    5. The is_whitelisted and is_blacklisted methods share nearly identical structures.

    Note that these problems also mean it is impossible to write unit tests for the list-consuming code.

    So, a much better design would have created a separate class for dealing with these lists and the classes that use this new class would do so by calling a single method in isolation for each such use.

    Also, the is_blacklisted and is_whitelisted methods should be combined because they share way too much structure:

    sub is_listed { my( $self, $source, $color, @lists ) = @_; croak ... if $color !~ /^(black|white)\z/; $color .= "list"; for my $list ( @lists ) { next # Let caller may pass in $parent even if empty if ! $list; if( $self->lookup($color,$list,$source) ) { $self->verbose("Found $source on $list $color"); if( $color eq "blacklist" ) { $self->increment_reject_count($source); } } } }

    The only difference between the two methods was that the is_blacklisted method, when it finds a match, it increments a count and sets a flag on the request. The setting of the flag should be moved to the caller:

    my $is_blacklisted = App::Blacklist->is_blacklisted( $source, $dest, $acct, $parent, $optout, ); if( $is_blacklisted ) { # Next line used to be in $self->is_blacklisted(): $self->set_was_blacklisted(); $self->reject(); return; }

    But this also serves as an example where it is an improvement to replace two methods with an 'if'. The 'if' is using knowledge of the structure of the class inside the implementation of the class so it isn't a violation of the good idea behind the badly worded "'if's are bad" meme.

    I also found it amusing that chromatic's best example in explaining "Replace Conditional with Polymorphism" was that:


    is better than:

    $item->save() if $item->is_dirty();

    Because if he had replaced that 'if' with polymorphism, then that would mean that he has two different classes, a "not dirty" class where save() is a no-op and a "dirty" class where save() does the saving and then causes the object to be transformed into an object of a different class, the "not dirty" class.

    And that is a great example of how replacing an 'if' with polymorphism can be just a terrible thing to try to do.

    The article chromatic linked to is actually talking about something much more specific than what chromatic actually discussed. For that very specific case of "if foo isa bar then foo.barkLikeABar() else foo.barkLikeABiff()", you aren't really replacing the 'if' with polymorphism. You are just fixing the polymorphism so you can remove the bad duplication (and also the 'if').

    So, if you call multiple methods on a single object in one block of code outside of that object's class, then you should consider whether there is a good description for what those multiple calls are doing with that object and whether you should just have a method that does that. It can lead to more cohesive, modular class design.

    A bad class is like a musket, a good class is like a modern pistol:

    $musket->insert( $powder ); $musket->insert( $ball ); $musket->tamp(); $musket->replace_cap( $cap ) if $musket->get_cap()->is_expended(); $musket->aim( $target ); $musket->pull_trigger(); # vs $pistol->shoot( $target ); $pistol->shoot( $target );

    - tye        

      I also found it amusing that chromatic's best example in explaining "Replace Conditional with Polymorphism" was that:

      Neither the post nor that example is about "Replace Conditional with Polymorphism". As you imply, that pattern is a specific case of the more general "Reduce Conditionals".

        It doesn't even reduce conditionals. It moves one conditional from outside to being one conditional inside. Okay, I'll grant that it can reduce conditionals if you have to follow that pattern more than once outside of the class. But I don't find the major benefit to be "fewer conditionals". I find the major benefit to be "isolate more (cohesive) knowledge inside the class". And that latter benefit applies even if the outer 'if' only occurred once.

        Just for counter-point, I'll note that sometimes you can improve cohesion by going in the opposite direction (with an example that actually involves polymorphism). If you have a class that is responsible for intelligently applying changes, knowing whether save() needs to do insert() or update() or nothing [or delete()], then it can be better to write:

        sub save { ... $item->update() if $item->is_dirty(); ... }

        in that one class than to have each type of item include code like:

        sub update_if_dirty { ... return if ! $self->is_dirty(); ... }

        (Though, I suspect that such a system might be even cleaner if is_dirty() isn't even tracked by the items but instead is handled by the "knows how to apply updates" wrapper.)

        But, yes, I did make too strong of a correlation between your opening sentence and your best example. Part of the reason I did that is because I have actually seen (several times) "replace 'if' with polymorphism" used as a justification for doing things almost as crazy as having an "is_dirty" class separate from an "is_clean" class. And I have actually (more than once) felt the pain of trying to make a small change in the resulting system and having this bifurcation (or worse) of classes get profoundly in the way.

        So I see need to highlight the problems with over applying that meme.

        I also did it because I really didn't see anything in your write-up that acknowledged the difference between what the linked article discussed and what you discussed. I was quite surprised to find the linked article so limited in scope. And not just because the scope was much more limited compared to the topic you covered, but because I had repeatedly seen that "replace 'if' with polymorphism" phrase used to describe practices far broader than what you discussed.

        So much so that I've come to associate that phrase with a pretty bad form of OO Kool-Aid. The article shows that I may not be the only one since DanMuller notes "As a blanket guideline, I find this to be bad advice bordering on the absurd. [....] Prefer clarity over dogmatic purity".

        I think it can be valuable to consider whether the interface you use for a class seems likely to be useful under polymorphism. It can help you notice internal details that are not well encapsulated and thus not hidden.

        In only the last few years, I noticed that the "best practices" that I was learning to reject because I was noticing that they lead to maintenance problems in the long term (inheritance being the most popular of those)... I noticed that those practices were interfering with modularity.

        So I am now paying much, much more attention to modularity, that relatively ancient programming best practice that seemed to have become so widely acknowledged that people stopped even talking much about it.

        Minimize (and simplify) the interfaces of your classes. Maximize the cohesiveness of your classes. There are a lot of more specific best practices that can be very useful in helping one see more ways in which they could increase modularity. But many of them also can be followed in ways that hurt modularity.

        So I now tend to double check my application of good ideas to verify that the end result actually improved modularity. I've found that "check" step to be very useful. And extremely practical, especially much later when I have to re-visit some code to perform maintenance.

        - tye        

      I wish tye had posted each sentence above in a separate node so that I could upvote them all.

Re: Programming patterns
by BrowserUk (Pope) on Jun 22, 2012 at 15:40 UTC

    His point seems quite clear to me. When defining an interface for (say) a bank account class that has an calcInterest() method that must only be called if the account is in credit. And then exposing an inCredt() method for the caller to check before calling calcInterest():

    while( my $accnt = $allAccnt->next ) { $accnt->calcInterest() if $accnt->inCredit(); }

    Simply make calcInterest() perform the inCredit() check internally, and have it do nothing at all otherwise. Take no actions and raise no exceptions.

    It adds one line inside calcInterest():

    sub calcInterest { my $self = shift; return unless $self->inCredit(); ... }

    And removes a test (and a scope) in caller code:

    $_->calcInterest() while $allAccnts->next;

    for every caller; and removes the need for, and the need to document externally, a method from the public interface.

    This is simplifies everyones code; the interface and its documentation; and reduces coupling. Everyone wins.

    With the rise and rise of 'Social' network sites: 'Computers are making people easier to use everyday'
    Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.
    "Science is about questioning the status quo. Questioning authority".
    In the absence of evidence, opinion is indistinguishable from prejudice.

    The start of some sanity?

Re: Programming patterns
by tobyink (Abbot) on Jun 22, 2012 at 19:31 UTC

    If your Trained::Monkey object gets passed a Widget object, and is expected to do something with the Widget object, but only if the Widget object is in the correct state; then it should be the Widget object which makes the decision about whether it's in the right state, not the Trained::Monkey object.

    Why? Because you may eventually subclass Widget to create a Widget::Complicated where the assessment of its state isn't as simple. You want to avoid having to give your monkey extra training.

    perl -E'sub Monkey::do{say$_,for@_,do{($monkey=[caller(0)]->[3])=~s{::}{ }and$monkey}}"Monkey say"->Monkey::do'
Re: Programming patterns
by Anonymous Monk on Jun 22, 2012 at 14:45 UTC

    How about a summery of what you do understand?

    I parse his statement as

    So this is usually fine
    sub method { $self->do_this if $self->query(); ...
    So this is usually wrong (conditional on $that
    sub method { ... if $that->query(); ...
    ?? sub method { $self->do_this if $that->query(); ...
    ?? sub method { $that->do_this if $that->query(); ...
    This is a huge red flag ( ?? encapsulation violation ?? )
    sub method { ... if $that->attribute() eq ...
    ???? sub method { ... if $self->attribute() eq ...

    Re-reading chromatic's post, chromatic's $item is probably cawley's $that

    The point is kinda obvious, simple
    Always try /call the method you want ( $item->do_this )
    But let it/that method fail based on its own state ( $self->dirty instead of $item->dirty )

    In other words, let everybody check their own underpants for ticks, no peeking :)

    I think I get it, but as usual chromatic's entry leaves you wanting more. FWIW, I'm not that well versed in OOP.

Re: Programming patterns
by McA (Priest) on Jun 23, 2012 at 00:08 UTC
    Hi all,

    thank you all very much for your comments. I'm really surprised that I got such much reaction on my post.

    I would like to hear how someone can check code for such and other code smells. Is it really only possible with experience? Or is there a way to work down a kind of checklist?

    It's really interesting to hear the opinion that following patterns religiously can also lead to "bad" code. But at the end you hear as often: It depends. I have to live with that in the hope to learn the right feeling to avoid bad design.

    Are tutorials out there which help to understand OO patterns especially for interpreted programming languages with loose types?

    Best regards
      I would like to hear how someone can check code for such and other code smells.

      The first thing I would say is, if you are spending your time worrying about minor interface design details like these, you must have too much time on your hands.

      Unless you are writing an interface for a module that is going to be reused by 100s or 1000s of people for the next decade or so, the difference such small, academic refactoring of this sort will make in the overall scheme of things are negligible.

      The reality is that most classes defined by most people rarely get reused beyond the project that they are defined for. And these days, far more sins are committed in the name of methodology purity (mostly OO purity, but also FP purity; AO purity etc.), than were ever committed in the name of premature optimisation.

      Is it really only possible with experience?

      Yes. But it is possible to simulate some of that experience to some degree -- iff you can justify the effort.

      Ie. If there is very good reason for you to realistically believe that the interface that you are designing will be reused sufficiently widely to justify the time and effort you put into it.

      The way to do that, is to try and use the interface in at least two substantially different (sub)projects. That is, define the interface on paper. If possible, mock up that interface in a "compilable" module that does nothing beyond checking arguments and returning "realistic" values.

      Then get a couple of people -- preferably people other than the interface designer -- to independently -- of each other and the designer -- prototype calling applications using the defined interface. With reasonably experienced people, this process shouldn't take more than 2 or 3 days. If it does, it probably means that it has found a problem.

      Or is there a way to work down a kind of checklist?

      Once the two prototypes are put together, get all three people in a room and go through the way the interface has been used in those prototypes. Contrasting how two people independently try to use the interface; and then contrasting those with how the designer thought it would be used. This process will usually do far more to highlight inconsistencies, omissions and redundancies in an interface design than any amount of running check lists, or other academical, criteria-driven soul searching done in isolation of actual real-use calling code.

      Interface design is all about the 'user experience'. Don't read that as the overused, over-hyped marketing phrase. In this case, the users are the programmers that will use the interface; and it isn't about warm and fuzzy "feelings", it is about the interface lending itself to the use by the calling code, without forcing that code into awkward, repetitive or inefficient hoops.

      The interface should be designed for the ease of learning and programming of the calling code; not the personal philosophies of the designers or the ease of internal implementation.

      Remember, the neo-classical adage that programmer time is more valuable than cpu time isn't true, if the time saved by the programmer that does the internal implementation, is at the expense of every programmer that has to use that implementation.

      Nor, if it makes the end users of every application written using it, sit around waiting, while it burns unnecessary cycles and power.

      With the rise and rise of 'Social' network sites: 'Computers are making people easier to use everyday'
      Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.
      "Science is about questioning the status quo. Questioning authority".
      In the absence of evidence, opinion is indistinguishable from prejudice.

      The start of some sanity?

      It's really interesting to hear the opinion that following patterns religiously can also lead to "bad" code.
      The thing to remember about patterns is that they're meant to be descriptive (a way of communicating what you're doing to other programmers) rather than prescriptive (a way of telling you what you should do). Bad code frequently results when you take a list of patterns and decide to use them purely because they've been blessed as Patterns(TM) rather than analyzing your problem, looking to see the most appropriate way to solve it, and then using patterns to express that solution.

      There's also the side issue that patterns are generally ways of filling gaps in the language that you're using (Perl has no patterns for finding text which has certain characteristics within a string because the language's built-in regex support is better than anything you're likely to write in your application code), so problems also arise from inappropriately applying one language's patterns in another language which has a better way of solving the problem at hand, but that seems to be a less common cause of pattern-derived bad code.

        Absolutely right on both counts, dsheroh. Well said.

        However, I would rephrase "Perl has no patterns for..." as "Perl programmers don't need patterns for...", since, generally, patterns don't exist within a programming language. (I mean, obviously there are patterns in the design of a language, but that's a different animal.) Also, I think you should have chosen a different example than "finding text ... within a string", since, in the context of that kind of problem, the term "pattern" has another meaning, so the description of your example could be (certainly was to me at first) confusing.

        I reckon we are the only monastery ever to have a dungeon stuffed with 16,000 zombies.
Re: Programming patterns
by sundialsvc4 (Abbot) on Jun 23, 2012 at 12:09 UTC

    Wow.   I’m running out of up-votes.

    I think that every method should “do something or die trying.”   Likewise, a property should “return a value or die trying.”   It should be entirely self-contained, knowledgeable of what it is doing and therefore of under what circumstances it does [not] make sense to do it, so that no one else is expected to (because no one else would know anyway).   Functional dependencies can lurk in lots of places, and if callers are (or, worse yet, have to be) making decisions and/or are “experts” in how to call you “correctly,” you have put cart before horse.

    Like BrowserUK, I find that elaborate, supposedly “reusable,” class structures are rarely actually re-used between applications.   (That, after all, creates a dependency between applications!)

    Inappropriate use of inheritance ... i.e. where the problem domain itself does not naturally use it ... also creates functional and implementation dependencies between the various classes.   Suddenly, you can’t change one thing without changing another.   Suddenly, changing one thing does change another ... whether you (heh...) knew it or not.   Code needs to be designed for long-term maintainability over the course of years.

Log In?

What's my password?
Create A New User
Node Status?
node history
Node Type: perlmeditation [id://977817]
Approved by Old_Gray_Bear
Front-paged by erix
and all is quiet...

How do I use this? | Other CB clients
Other Users?
Others taking refuge in the Monastery: (7)
As of 2018-06-20 06:11 GMT
Find Nodes?
    Voting Booth?
    Should cpanminus be part of the standard Perl release?

    Results (116 votes). Check out past polls.