Refactoring a PHP class

Question

I am playing with refactoring a PHP class. My original class had a constructor and one large method that ran a string through various operations and spat out the result at the end.

class String
{
    public function __contstruct()
    {
        // a couple of initialisation things
    }

    public function make($string)
    {
        // very large method that does lots of different types of filtering to an input string
    }
}

My class gets run through scrutinizer-ci and one of the things it was suggesting was to refactor the large method into smaller self contained methods using the Composer method pattern.

https://scrutinizer-ci.com/docs/refactorings/compose-method

I have done this and it does seem neater, but here is my problem/query...

My new class now looks something like this

class String
{
    public function __construct()
    {
        // a couple of initialisation things
    }

    public function make($string)
    {
        $string = $this->smallMethodOne($string);
        $string = $this->smallMethodTwo($string);
        $string = $this->smallMethodThree($string);
        $string = $this->smallMethodFour($string);
        $string = $this->smallMethodFive($string);
    }

    private function smallMethodOne($string) {
        // do some stuff
        return $string;
    }

    private function smallMethodTwo($string) {
        // do some stuff
        return $string;
    }

    private function smallMethodTwo($string) {
        // do some stuff
        return $string;
    }

    private function smallMethodThree($string) {
        // do some stuff
        return $string;
    }

    private function smallMethodFour($string) {
        // do some stuff
        return $string;
    }

    private function smallMethodFive($string) {
        // do some stuff
        return $string;
    }
}

I was just wondering if there was a better way of organising the make() method as it doesn't feel correct doing it like that, just running a bunch of methods in sequence.

Answer 1

What you should ask yourself is: how hard would overriding a single "smallMethod" be without changing other "smallMethod"s behavior?

If you can override a single "smallMethod" without touching others, you are likely doing the correct thing.

Say you have this:

public function deleteMe()
{
    if($this->a == x || $this->b != y){
         $this->cache->delete($this);
         $this->storage->delete($this);
         $this->refs->unlink($this);
    }
}

Now what if you want to extend the class and change the deleteMe() if() condition? You will need to totally rewrite the method, for a one line change. That's not convenient at all. Instead:

protected function checkDeletability()
{
    return $this->a == x || $this->b != y;
}

protected function internalDelete()
{
     $this->cache->delete($this);
     $this->storage->delete($this);
     $this->refs->unlink($this);
}

public function deleteMe()
{
    if($this->checkDeletability())
         $this->internalDelete();
}

This way, you can override single pieces of the procedure, without touching others. That's basically why you "split a big method into smaller ones".

So, check if you are satisfied with this. That's what matters most!

Answer 2

I would like this :

public function __construct($string)
{
    $this->string = $string;
    // a couple of initialisation things
}
public function make()
{
    if(!$this->smallMethodOne()){
        return false;//or custom exception or array with status/description
    }
    ...
    if(!$this->smallMethodFive()){
        return false;//or custom exception or array with status/description
    }

   return $this->string;

}
private function smallMethodOne() {
    // do some stuff
    $this->string = $new_string;
}