loading...

A small refactoring example

phpandcigars profile image PHP and cigars ・2 min read

I'm currently reviewing a small piece of code of a new co-worker. As he comes back to work next week I will share a small example how to refactor the code here, so I won't forget to mention it to him.

I found this:


...

foreach ($p->mProductUnits as $unit) {
    $ret .= hHtml::Clear();
    $ret .= $this->RenderInfoRow(
        $unit->showUnit(),
        $unit->RenderPriceWithCurrency(),
        $unit->AvailabilityMessage()
    );
}

...

private function RenderInfoRow($name, $price, $availbility){

    return
        '<div class="col-l-4 col-m-4">'.$name.'</div>'
        .'<div class="col-l-2 col-m-2">'.$price.'</div>'
        .'<div class="col-l-4 col-m-4">'. $availbility.'</div>';

}

What problems do we have here? Hungarian notation? Well, yes, but that is my fault and is due to the framework i wrote. Besides that?

Wright. The single responsability principle is hurt. Also Uncle Bob teached us, that one parameter to a function is ok. In this example it is absolutely possible to use only one parameter to the function RenderInfoRow()
So lets see how this gets refactored in a first step:


foreach ($p->mProductUnits as $unit) {
    $ret .= $this->RenderInfoRow($unit);
}

private function RenderInfoRow(oProductUnit $u) {

    return
    hHtml::Clear()
    . '<div class="col-l-4 col-m-4">' . $u->showUnit() . '</div>'
    . '<div class="col-l-2 col-m-2">' . $u->RenderPriceWithCurrency() . '</div>'
    . '<div class="col-l-4 col-m-4">' . $u->AvailabilityMessage() . '</div>'
    ;

}

This has more than one advantages. First: It's easier to read and less to write. But second (and in this case maybe more important): Now the IDE knows, that $u is of the type oProductUnit. So now you don't have to open the class to see the function-names. You get them presented as soon as you type $u in the function.

Discussion

pic
Editor guide