views:

50

answers:

2

Is this a sign, that some design pattern is needed here?

/**
 * @return string
 */
public function filter($string)
{
    // underscored means private
    $this->_prepareSomething($string); 
    $this->_prepareSomething2();
    $this->_prepareSomething3();
    $this->_prepareSomething4();
    $this->_prepareSaveSomethingToFile();

    return $this->getFiltered();
}
+1  A: 

It's all about readability and the right level of abstraction. For me as the reader it's hard to see what data is exchanged between the _prepare* functions. And, let's pretend they are calculations of some sort, then saving some data to file is mixing business logic with persistance. Which looks like mixing of abstraction levels.

Also, the getFiltered() call is also confusing, as it looks like a public method is called with a similar naming scheme as the original function.

Patterns: See http://c2.com/ppr/wiki/WikiPagesAboutRefactoring/ComposedMethod.html for the composed method pattern explanation, and http://www.markhneedham.com/blog/2009/06/12/coding-single-level-of-abstraction-principle/ for the SLAP principle.

WardB
+1: Plus, I'd HIGHLY recommend reading [Code Complete 2](http://www.amazon.com/Code-Complete-Practical-Handbook-Construction/dp/0735619670), as it does a great job explaining all of this...
ircmaxell
A: 

It looks like you're already implementing a Template Method pattern. If your class will have subclasses which may need to override certain steps in that process, then I'd say you've already designed your base class effectively.

mikemanne