Refactoring question

[dennis-fedco]

I have a class that adds a special PDF page to an existing PDF.  The page refers to a specific product and there are products A, B, C, D.  Products B, C, D are similar and with a bit of if/then/else magic I can basically reuse the same function for all 3, but product A is different enough to have its own function.

 

My class looks like this:

class AddPdfPage
{
    function addA(&$pdf, $vars) {}
    function addB(&$pdf, $vars) {}
    function addC(&$pdf, $vars) {}
    function addD(&$pdf, $vars) {}
}

//The way I call it from within the code right now is like this:
AddPdfPage::addC($pdf, $vars);
AddPdfPage::addB($pdf, $vars);


//the variable below contains the product line -- A, B, C, or D
//that's the one I said I could use to merge functions B/C/D into one if I wanted to
//I could even include product A if I wanted to even though the code there is different.
$vars['productLine']; 

//The code inside addX() functions is something like:
function addX($pdf, $vars)
{
    $pdf->addPage($vars);
    $pdf->addDescription(".."); //that is different based on each specific product line
}

Assuming I want to add a page to PDF for a paricular product X, how do I write or structure my code?  I am not exactly sure.

 

Do I set it up somehow to use polymorphism?  Do I do something else?

 

I assume I don't really want to merge products B/C/D into the same function, even if I could, since they physically represent different product lines, so I might as well keep them as such -- separate.  But I may want to use some features of the language to clean up and better up my code.  The "how", and what techniques do I use, and what my final code may look like is what my post is about.

 

I am thinking of doing something like:

class AddPdfPage()
{
    private $pdf;
    private $vars;
    private $productLine;

    function __construct($pdf, $vars)
    {
        $this->pdf = $pdf;
        $this->vars = $vars;
        $this->productLine = $vars['productLine'];
    }

    public function addPage()
    {
        switch($this->productLine)
        {
             case 'A': ...break;
             case 'B': ...break;
             case 'C': ...break;
             case 'D': ...break;
        }
    }
}

Use that as a jump start and I guess I could use polymorphism by creating separate classes for each product line and extending some base class, if switch statement is not good.. (http://c2.com/cgi/wiki?SwitchStatementsSmell)

but I feel like I will be polluting my file system with too many classes and not sure if that's exactly a benefit.  So just thinking about how to do this.

Edited October 15, 2014 by dennis-fedco

[requinix]

Polymorphism would be a good first step.

interface IProductPdfWriter {

	public function add($pdf, $vars);

}

abstract class BasicProductPdfWriter implements IProductPdfWriter {

	public function add($pdf, $vars) {
		// ...
	}

}

class ProductBPdfWriter extends BasicProductPdfWriter { }
class ProductCPdfWriter extends BasicProductPdfWriter { }
class ProductDPdfWriter extends BasicProductPdfWriter { }

class ProductAPdfWriter implements IProductPdfWriter {

	public function add($pdf, $vars) {
		// ...
	}

}

Class loading could be done a number of ways, like constructing class names

$class = "Product" . ucwords($product) . "PdfWriter";
$writer = new $class();

or using a mapping somewhere

$classes = array(
	"A" => "ProductAPdfWriter",
	"B" => "ProductBPdfWriter",
	"C" => "ProductCPdfWriter",
	"D" => "ProductDPdfWriter"
);
$class = $classes[$product];
$writer = new $class();

(you wouldn't need the dedicated B/C/D classes and could just use BasicProductPdfWriter for the three) Edited October 15, 2014 by requinix

[dennis-fedco]

yep my "issue" with that is that before there was one class and one file.

 

and now there are 6 files -  (1 x interface, 1 x base, 4 x product)

 

I guess the good side though is that it makes software more modular.

 

I am not sure why have the Inteface though, to me it only tells the software/programmer that a certain function must be implemented, that's all, and for that one little thing it wastes so many resources -- takes up entire file on file system.

[requinix]

Unless you're running on a system with memory measured in the hundreds of megabytes, it does not "waste so many resources".

 

But hey, nothing says you have to use so many classes. Do whatever gets you the quick win now.

[dennis-fedco]

To think of it, I was thinking more in terms of resources of the programmer.  Understanding the purpose behind 6 classes takes a bit more cognitive power than seeing one class.

Or maybe a different kind of cognitive processing.

 

There is a difference between seeing a class called ProductPdfWriter and thinking "ah, I know what this class does", it writes to Products to PDF,

 

and seeing classes like:

• BasicProductPdfWriter
• IProductPdfWriter
• ProductAPdfWriter
• ProductBPdfWriter
• ProductCPdfWriter
• ProductDPdfWriter

and going .. what the heck?  how complex of a system are we writing to nothing else but to place bits of text on a PDF template?

 

Not saying "I can't do it" or "someone should not do it", and considering "I can do whatever I want with it", but more so expressing my surprise at such complexity which I think is kind of needless.

Edited October 16, 2014 by dennis-fedco

[maxxd]

Actually, it's not a bad way at all to think about it (admittedly, it's exactly how I'd set it up so I might be a bit biased). Basically, if you're looking at base functionality, look in the BasicProductPdfWriter class. If it's something specific, look in the specific class. The worst that'll happen is that a developer will start by looking in the specific product class and get referred up the chain to the more general class - you're not so much trying to understand 6 different classes, you're trying to understand 1 class and some very specific differences.

 

The reason to use the interface is not only to ensure a consistent api, but to make the polymorphism transparent. By type hinting the interface in your object methods you can pass the basic writer or any of the specific product classes like so:

public function doSomethingCool(IProductPdfWriter $writer){
    $writer->add($pdf,$neatStuff);
}

$writer can then be an instance of ProductAPdfWriter, ProductBPdfWriter, BasicProductPdfWriter, etc. and the method will accept it.

[dennis-fedco]

Found an issue -- in my IDE I can click on a class name where it is being called, and have it jump to the declaration of the class.  If I encode the class name as

$class = new $className();

I will lose that ability.

[requinix]

Found an issue -- in my IDE I can click on a class name where it is being called, and have it jump to the declaration of the class.  If I encode the class name as

$class = new $className();

I will lose that ability.

 

True, but the only way to keep that behavior is to list out every single class being used. Get the typehinting involved as soon as possible to mitigate that, but in my experience as long as you name the classes well it's not a big problem. And that's coming from someone who totally agrees with your concern about not having IDE support: with very few exceptions, like this naming convention thing, I don't write something if it can't be supported by the IDE.

[dennis-fedco]

Thanks.

 

Well, I read up a bit on why switch statements vs polymorphism...

(http://c2.com/cgi/wiki?SwitchStatementsSmell)

 

and it just seems to be a preference of choice.  (see the very last paragraph starting with "I think one can summarize most of the above opinions as follows").

 

I have a LOT of legacy code that currently uses Switch statements.  I am rewriting some of it to use Polymorphism, but I paused long enough to read that article and to ask questions, like here.  It just seems to me that these days the more "popular" technique is the Object Orientedness, which favors Polymorphism.  And that is what people recommend.

 

I don't think I'd agree blindly, and it is probably more of a way of how you like your code, or even how you like to think (in objects or in switches).

 

As otherwise there appears to be no objectively measured improvement of one over the other.  In other words, it's like moving the carpet around.  There is no way to make it fit perfectly, but you can pull it more towards one side, or more towards the other depending on your preferences, or preferences that of others that you choose or are mandated to subscribe to.

 

I wish there was a more objective reasoning behind it but there is not, other than that summary at the bottom of the linked page about function size and about the way one likes to think about code.  So overall, I am kinda glad I looked into it.

But in the end, it didn't really help other then to help solidify that one is not necessarily is better than the other, but mostly interchangeable.