Suggestions for better oop designs with real example (file services) I'm watching Uncle Bob to improve code maintainability and readability.

I've built a system in which can upload various file formats. Bellow I've posted the pdf one.

The below code works perfect but I need help if there is a way to improve code design and quality.

Any suggestion is welcome.

I use an abstract class FileServices:

abstract class FileServices
{
    protected $file;
    protected $fileName;

    public function __construct($file)
    {
        $this-&gt;file = $file;
        $this-&gt;fileName = pathinfo($this-&gt;file-&gt;getClientOriginalName(), PATHINFO_FILENAME);
        $this-&gt;setNewFileNameIfExist();
        $this-&gt;saveToDB();
        $this-&gt;saveToServer();
    }

    public function saveToServer()
    {
        $path = $this-&gt;path;
        $realNameWithExtension = $this-&gt;fileName  . $this-&gt;fileExtension;
        Storage::put($path . $realNameWithExtension, File::get($this-&gt;file));
    }

    abstract function saveToDB();
    abstract function setNewFileNameIfExist();
}


And PDF child class:

class PDF extends FileServices
{
    protected $fileExtension = '.pdf';
    protected $path = 'public/pdf/';

    public function saveToDB()
    {
        $newFile = new \App\PDF();
        $newFile-&gt;title = $this-&gt;fileName . $this-&gt;fileExtension;
        $newFile-&gt;save();
    }

    public function setNewFileNameIfExist()
    {
        $counter = 0;
        $realName = $this-&gt;fileName;
        $allPDF = \App\PDF::all();
        foreach ($allPDF as $pdf) {
            if($pdf-&gt;title === $this-&gt;fileName . $this-&gt;fileExtension) {
                while($pdf-&gt;title === $this-&gt;fileName . $this-&gt;fileExtension) {
                    $this-&gt;fileName = $realName . '(' . $counter . ')';
                    $counter++;
                }
            }
        }
    }

}


In the client:

$file = $request-&gt;file('pdf');
new PDF($file);


Many thanks to @tim for his answer.
Protected Field vs Getter and setter

I prefer to not access fields directly, because it's difficult to use and not very flexible.

When I need to implement a new fileservice, I just need to know that I need to define a field called fileExtension. If you do not thoroughly document this, implementing new services in the future will be a pain (and even with documentation it's not ideal, as nobody really reads it anyways).

The second problem is that I can only define a string as file extension. What if I want to create an image file service, that allows the uploading of jpg and png files? I would need to dynamically set fileExtension, and the only place I could do that is in saveToDB or setNewFileNameIfExist, neither of which should really be responsible (and even though it may work right now, it may easily break once you change your base FileServices class).

It would be a lot better to just add an abstract getter for the file extension.

The same problem exists with path as well, and also works the other way around: in PDF you work directly on fileName, instead of using some setter method.

Field location

It is odd that you store the filename in FileServices, but the fileExtension and path in PDF. Just put them all in FileServices to simplify things.

File class

You may also consider creating a File class, which could hold file, fileName and fileExtension, as they all belong together. It may also contain common functions such as the one to get a filename (without extension).

Various Store Methods

I think that it is not ideal to have saveToDB and saveToServer in your base service. What if I only want to save the file in the database, but not the server?

Instead, I would have a generic, abstract storeFile function in FileServices.

Need for abstract FileServices?

Of course, if you follow the advise above, it's not clear that FileServices is actually needed as an abstract class. The only thing it does is provide a function to get the name of a file (which may as well be a utility function in some file utility class, as it is likely used elsewhere as well), and a function to save a file to the server (which is really just a call to a different function.)

If you expect more common functionality in the future, this isn't really a problem, otherwise you may consider changing the abstract class to an interface.

Misc


the PDF class isn't really a PDF, but a specific kind of file service, so PDFFileService would be a better name.
I prefer constructors to not do the actual work. It seems unclean, and the resulting calling code looks wrong. Reading new PDF($file), I would assume that a new object was created, but not that anything else was actually done. Something like new PDF($file)-&gt;store() would read more logical to me. 
Your variable names are not consistent with what you consider real names. In saveToServer it's actually the new file name, in setNewFileNameIfExist, it's the original file name.