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.
Top comments (0)