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?
Please or to participate in this conversation.