Beefy Boxes and Bandwidth Generously Provided by pair Networks
No such thing as a small change

Refactoring Perl #2 - Inline Method

by agianni (Hermit)
on Jun 26, 2007 at 19:10 UTC ( [id://623470] : perlmeditation . print w/replies, xml ) Need Help??

A method's body is just as clear as its name.

Put the method's body into the body of its callers and remove the method.

(Fowler, p. 117)

Fowler's second refactoring pattern is remarkably simple, but it is a classic example of a refactoring that breaks tests for me. It's not generally a big problem but sometimes it makes me wonder if I'm doing something wrong.

Fowler's example in Perl looks like this:

sub get_rating{ my $self = shift; return $self->more_than_five_late_deliveries() ? 2 : 1; } sub more_than_five_late_deliveries{ my $self = shift; return $self->{_number_of_late_deliveries} > 5; }


sub get_rating{ my $self = shift; return ( $self->{_number_of_late_deliveries} > 5 ) ? 2 : 1; }

Get the code

The concept is simple, and Fowler puts it well in the summary. But there are two issues that are introduced with what appears to be a simple refactoring pattern. The first and most obvious issue is that this refactoring breaks my tests (see inline_method_refactored.t in the example code for the fixed test). This isn't a big deal exactly, although with larger refactorings of this type it takes some effort to rewrite the tests. The upshot is that, ideally, you really need to rewrite your tests before you perform the refactoring so that you aren't testing the methods that are going away. At the same time, you need to make sure that anything you may have overridden for the sake of isolating tests to specific methods.

The second issue is more of a philosophical question about when and how to refactor. For example, while the get_rating method is small, it is useful to have it extracted because it includes some specific logic. Were I to need this in more than one place, I wouldn't have to worry about whether I repeated the code correctly in more than one place. Even in the case where I am only going to call this method once, it is useful to have it extracted.

I might actually consider refactoring this differently given the right context. From a business logic perspective, it appears that $self->{_number_of_late_deliveries} has some business significance, so I might simply change change the name of the original more_than_five_late_deliveries method to too_many_late_deliveries. Alternately, I might refactor it as Fowler does, but extract the value of the number 5 to a config-ish method called something like late_delivery_threshold.

All of this is simply to point out that refactoring isn't generally as simple as having a technical understanding of a programming language and having a developers understanding of how to make code more maintainable. In a lot of cases, having an understanding of the business domain can be helpful in organizing code effectively.

perl -e 'split//,q{john hurl, pest caretaker}and(map{print @_[$_]}(joi +n(q{},map{sprintf(qq{%010u},$_)}(2**2*307*4993,5*101*641*5261,7*59*79 +*36997,13*17*71*45131,3**2*67*89*167*181))=~/\d{2}/g));'

Replies are listed 'Best First'.
Re: Refactoring Perl 2 - Inline Method
by hossman (Prior) on Jun 26, 2007 at 20:35 UTC

    It's been a while since i looked at the Refactoring book, but as i recall "Inline Method" is only recommended when the method is only used in a very few number of cases (typically only within the class that defines it)

    As you point out, for examples like this it might instead make sense to refactor the constant (5) to a global variable (or to a new method call to get it from configuration) ... either of which might then ultimately lead to a decision to inline the method anyway because it's now easier to read/understand then it was before.

Re: Refactoring Perl 2 - Inline Method
by agianni (Hermit) on Jun 27, 2007 at 16:05 UTC

    One more thing I forgot to mention: Fowler suggests:

    Don't inline if subclasses override the method; they cannot override a method that isn't there (Fowler p. 118)

    In other words, if the method has been factored out to provide an easy override hook, leave it alone. This can present challenges if you have a large code base but I usually find that:

    find . -exec grep that_method {} \; -print

    does the trick nicely for finding such things

    perl -e 'split//,q{john hurl, pest caretaker}and(map{print @_[$_]}(joi +n(q{},map{sprintf(qq{%010u},$_)}(2**2*307*4993,5*101*641*5261,7*59*79 +*36997,13*17*71*45131,3**2*67*89*167*181))=~/\d{2}/g));'