Be part of JetBrains PHPverse 2026 on June 9 – a free online event bringing PHP devs worldwide together.

4unkur's avatar

Can anyone check my refactoring (extracted logic to abstract class)?

I had 1 class which generated merged PDF file out of several PDF files for Certificate model. Now I have to implement the same logic to the other model, but implementation is a bit different So I extracted common logic to abstract class and inherited from it for both classes.

Here's how it was: (CertificatePdfMerger):

class CertificatePdfMerger
{
    private Merger $merger;
 
    public function __construct(private TempFileService $cloud, private PdfPasswordProtector $passwordProtector)
    {
        $this->merger = new Merger();
    }
 
    public function execute(Certificate $certificate): void
    {
        $this->addFiles($certificate);
 
        $mergedFilepath = $this->mergeFiles();
 
        $media = $certificate
            ->addMedia(storage_path("app/$mergedFilepath"))
            ->toMediaCollection(Certificate::COLLECTION_COMPLETE_PDF);
 
        $this->passwordProtector->create($certificate, $media, Certificate::COLLECTION_PROTECTED_COMPLETE_PDF);
    }
 
    /**
     * Adds files to the merger
     *
     * @param  Certificate  $certificate
     * @throws \Illuminate\Contracts\Filesystem\FileNotFoundException
     */
    private function addFiles(Certificate $certificate): void
    {
        $file = $this->cloud->getFile($certificate->getFirstMedia(COLLECTION_PDF));
 
        $this->merger->addFile($file);
 
        $certificate->formCertificates->each(function (Certificate $child) {
            $media = $child->getFirstMedia(COLLECTION_PDF);
            if ($media) {
                $this->merger->addFile($this->cloud->getFile($media));
            }
        });
    }
 
    /**
     * Generates new merged file
     *
     * @return string path to the merged file
     * @throws Exception
     */
    private function mergeFiles(): string
    {
        $mergedCertificateContents = $this->merger->merge();
        $this->merger->reset(); // clearing the merger from added files
        $filename = Str::random(40) . '.pdf';
 
        $result = Storage::put($filename, $mergedCertificateContents);
 
        if (! $result) {
            throw new Exception('Unable to merge PDF files');
        }
 
        return $filename;
    }
}

Now it's like this:

abstract class AbstractPdfMerger
{
    protected Merger $merger;
 
    public function __construct(protected TempFileService $cloud, protected PdfPasswordProtector $passwordProtector)
    {
        $this->merger = new Merger();
    }
 
    abstract public function execute(Convertable $model): void;
 
    abstract protected function addFiles(Convertable $certificate): void;
 
    /**
     * Generates new merged file
     *
     * @return string path to the merged file
     * @throws Exception
     */
    protected function mergeFiles(): string
    {
        $content = $this->merger->merge();
        $this->merger->reset(); // clearing the merger from added files
        $filename = Str::random(40) . '.pdf';
 
        $result = Storage::disk('local')->put($filename, $content);
 
        if (!$result) {
            throw new Exception('Unable to merge PDF files');
        }
 
        return $filename;
    }
}
class CertificatePdfMerger extends AbstractPdfMerger
{
    public function execute(Convertable $certificate): void
    {
        $this->addFiles($certificate);
 
        $mergedFilepath = $this->mergeFiles();
 
        $media = $certificate
            ->addMedia(storage_path("app/$mergedFilepath"))
            ->toMediaCollection(Certificate::COLLECTION_COMPLETE_PDF);
 
        $this->passwordProtector->create($certificate, $media, Certificate::COLLECTION_PROTECTED_COMPLETE_PDF);
    }
 
    protected function addFiles(Convertable $certificate): void
    {
        $file = $this->cloud->getFile($certificate->getFirstMedia(COLLECTION_PDF));
 
        $this->merger->addFile($file);
 
        $certificate->formCertificates->each(function (Certificate $child) {
            $media = $child->getFirstMedia(COLLECTION_PDF);
            if ($media) {
                $this->merger->addFile($this->cloud->getFile($media));
            }
        });
    }
}
	class MixedSurveyResponsePdfMerger extends AbstractPdfMerger
{
    public function execute(Convertable $response): void
    {
        $this->addFiles($response);
 
        $mergedFilepath = $this->mergeFiles();
 
        $media = $response
            ->addMedia(storage_path("app/$mergedFilepath"))
            ->toMediaCollection(COLLECTION_PDF);
 
        $this->passwordProtector->create($response, $media);
    }
 
    protected function addFiles(Convertable $response): void
    {
        $collections = [
            SurveyResponse::COLLECTION_PDF_HEADER,
            SurveyResponse::COLLECTION_PDF_CONTENT,
            SurveyResponse::COLLECTION_PDF_FOOTER,
        ];
 
        foreach ($collections as $collection) {
            $file = $this->cloud->getFile($response->getFirstMedia($collection));
            $this->merger->addFile($file);
        }
    }
}

In general it feels OK for me but I'm a bit concerned about the Convertable interface. Before I could type hint exact model, now I cannot as it's type hinted in the abstract parent. Convertable interface indicates that model has PDF files (which is converted from Word file) Any suggestions or comments?

0 likes
0 replies

Please or to participate in this conversation.