1
\$\begingroup\$

I wrote some classes to convert some documents (PDF, DOC, DOCX and TXT) to only text so that it can be stored in a search engine.

Here's how I use those classes :

$file = 'text.doc'; // comes from the user, hardcoded for the example
$file_extension = pathinfo($file, PATHINFO_EXTENSION);

switch($file_extension){
    case 'pdf':
        $reader = new PDFConverter($file);
        break;
    case 'text': 
        $reader = new TXTConverter($file);
        break;
    case 'doc':
        $reader = new DOCConverter($file);
        break;
    case 'docx':
        $reader = new DOCXConverter($file);
        break;
}

$reader->handle(); 
$text = $reader->getText();

Here's the parent Converter class :

class Converter
{
    protected $file;
    protected $text;

    public function __construct($file)
    {
        $this->file = $file;
    }

    public function getText()
    {
        return $this->text;
    }
}

Here's one of the converter class, PDFConverter :

class PDFConverter extends Converter implements IConverter
{
    public function handle()
    {
        $this->text = 'Text PDF'; // just for demo
    }
}

And finally the IConverter interfaces that all subclasses implement :

interface IConverter
{
    public function handle();
}

How does this look? Is this a good use of inheritance/interfaces? Any pointers on how to structure this better would be appreciated.

I've removed the logic from the subclasses' handle() method to keep things simple for Codereview.

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

You have two methods getText() and handle() of which one is not needed. Doing the conversion and returning the result should be one method. So, like this:

class Converter
{
    protected $file;
    protected $text;

    public function __construct($file)
    {
        $this->file = $file;
    }
}

and:

class PDFConverter extends Converter implements IConverter
{
    public function getPlainText()
    {
      if (is_null($this->text)) // not yet converted?
      {   
        $this->text = 'Text PDF'; // do the conversion
      }
      return $this->text; // return result
    }
}

This way you still buffer the result, but you get rid of the rather weird handle() method.

When using this class you don't have to think about the order in which to call the methods anymore, just ask for the plain version of the text and the method will do the conversion only once.

You might want to move the declaration of the $text field to the other class.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.