4
\$\begingroup\$

I'm trying to learn OOP with PHP (I know it's hard because It's not an OOP language). I'm rewriting my procedural code to OOP.
Please provide any feedback on my code, naming and file/folder structure.

Structure:

\classes
  Procurement.php
  ProcurementDetails.php
\services
  DatabaseService.php
  ProcurementService.php
  ProcurementsService.php
\public
  Index.php
\routes
  Procurements.php

classes\Procurement.php

<?php

class Procurement {
    public int $id;
    public null | array | string $group;
    public null | string $object_name;
    public null | string $procurement_type;
    public null | string | object $responsible_user;
    public null | string $announcement_time;
    public null | string $status;
    public null | string | object $creator;

    public function __construct()
    {
        if ($this->responsible_user) $this->responsible_user=json_decode($this->responsible_user);
        if ($this->group) $this->group=json_decode($this->group);
        if ($this->creator) $this->creator=json_decode($this->creator);
    }

    private function __set($name, $value) {}
};

classes\ProcurementDetails.php

<?php
require_once __DIR__ . '/Procurement.php';

class ProcurementDetails extends Procurement {
    public null | string $description;
    public null | int $sum;
    public null | string $procurement_method;
    public null | string | array $contact_person;

    public function __construct()
    {
        if ($this->contact_person) $this->contact_person=json_decode($this->contact_person);
        parent::__construct();
    }
}

services\DatabaseService.php

<?php
interface IDatabaseService {
    /**
     *
     *
     * @return PDO
     */
    public function GetPDO();
}

class DatabaseService implements IDatabaseService { 
    /**
     * PDO object
     *
     * @var PDO
     */
    private $DB;
    private $host   = '';
    private $dbname = '';
    private $user   = '';
    private $pass   = '';
    
    /**
     * Creates the connection
     *
     * @return void
     */
    public function __construct () {
        $this->DB = new PDO("mysql:host={$this->host};dbname={$this->dbname};charset=utf8mb4",$this->user,$this->pass);
        $this->DB->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
        $this->DB->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
        $this->DB->setAttribute(PDO::ATTR_STRINGIFY_FETCHES, false);
    }
    
    public function getPDO() {
        return $this->DB;
    }

    /**
     * __destruct
     *
     * @return void
     */
    public function __destruct () {
        $this->DB=null;
    }

}

services\ProcurementsService.php

<?php
require_once __DIR__ . '/../classes/Procurement.php';
require_once __DIR__ . '/ProcurementService.php';

interface IProcurementsService {    
    /**
     * Returns all procurements from database
     * Procurement[]
     *
     * @return Procurement[]
     */
    public function GetAllProcurements();
    /**
     * GetById
     *
     * @param  int $id
     * @return IProcurementService
     */
    public function GetById(int $id);
}

class ProcurementsService implements IProcurementsService {

    private PDO $_db;
    
    /**
     * __construct
     *
     * @param  PDO $db
     * @return void
     */
    public function __construct(PDO $db)
    {
        $this->_db = $db;
    }
    
    /**
     * GetById
     *
     * @param  int $x
     * @return IProcurementService
     */
    public function GetById(int $id): IProcurementService {
        return new ProcurementService($id, $this->_db);
    }
    
    /**
     * Returns all procurements from database
     * Procurement[]
     *
     * @return Procurement[]
     */
    public function GetAllProcurements(): iterable {
        $stmt=$this->_db->query("SELECT ....");
        return $stmt->fetchAll(PDO::FETCH_CLASS, 'Procurement');
    }
}

classes\ProcurementService.php

<?php
require_once __DIR__ . '/../classes/ProcurementDetails.php';

interface IProcurementService {
    /**
     * Returns more info about the procurement
     *
     * @param  int $id
     * @return ProcurementDetails
     */
    public function GetDetails();
    public function GetFiles(); //Not implemented
    public function GetComments(); //Not implemented
}

class ProcurementService implements IProcurementService {
    private int $id;
    private PDO $_db;

    public function __construct(int $id, PDO $db)
    {
        $this->id = $id;
        $this->_db = $db;
    }

    /**
     * Returns more info about the procurement
     *
     * @param  int $id
     * @return ProcurementDetails
     */
    public function GetDetails(): ProcurementDetails
    {
        $stmt = $this->_db->prepare("SELECT with details....");
        $stmt->execute([$this->id]);
        return $stmt->fetchObject('ProcurementDetails');
    }

}

routes\Procurements.php

<?php
use Psr\Http\Message\ResponseInterface as Response;
use Psr\Http\Message\ServerRequestInterface as Request;

require_once __DIR__ . '/../services/DatabaseService.php';
require_once __DIR__ . '/../services/ProcurementsService.php';

$app->get('Procurements', function(Request $request, Response $response) {
    $DB=new DatabaseService();
    $ProcurementsService = new ProcurementsService($DB->getPDO());
    $response->getBody()->write(json_encode($ProcurementsService->GetById(100)->GetDetails()));
    return $response->withHeader('content-type','application/json')->withStatus(200);
});

I felt like I got off track with OOP, so I don't have much code. BTW! I am using PHPSLIM.

\$\endgroup\$
6
  • 4
    \$\begingroup\$ If you down vote a question please leave a comment explaining why you down voted the question. I don't see anything here that suggests there should be a down vote. \$\endgroup\$
    – pacmaninbw
    Oct 17 at 22:30
  • \$\begingroup\$ Just one quick note. The "classes" directory would better be called "entities". \$\endgroup\$
    – slepic
    Oct 18 at 5:05
  • \$\begingroup\$ Thanks @pacmaninbw , was about to delete the post because of the downvote... slepic Ok, that makes more sense. \$\endgroup\$ Oct 18 at 5:46
  • \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$
    – BCdotWEB
    Oct 18 at 6:59
  • \$\begingroup\$ I didn't downvote, but I could understand how someone would downvote simply due to the title not following our guidelines. Moreover, the question's body also doesn't explain what the code does. \$\endgroup\$
    – BCdotWEB
    Oct 18 at 7:00
4
\$\begingroup\$

Firstly, PHP is 100% an 'OOP' language. It provides you with all the necessary functionality to conform to all of the OOP principles.

Secondly, you should definitely familiarise yourself with the PHP Standards Recommendations (PSR) as they will guide you in keeping everything inline with the standard conventions that PHP developers should follow when using the language.

Ok, with those nitpicking points out of the way, let's have a quick look at your code.

Utilisation of Interfaces

It's good to see you have the foresight to use interfaces, but your utilisation of them is a little off. One of the benefits of using interfaces is to ensure anything that implements it, conforms to the contract it has defined. This allows us to write a piece of code and swap out the implementation of the interface with something else, knowing full well that we won't need to change any of the utilising code.

Example:

We declare an interface for connecting to our database. As we may decide to implement different types of database connections, we need to selectively identify functions that will be needed for all database implementations. Note: I've kept it simple to ensure you grasp the concept, as opposed to what It's actually doing...

<?php

namespace App\EXAMPLE;

interface DatabaseInterface
{
    public function connect();

    public function disconnect();

    //Any other functions
}

This interface allows us to create different types of database connections and ensures the code we write to connect to the database, will remain untouched when swapping it out.

Here's what an implementation may look like utilising PDO.

<?php

namespace App\EXAMPLE;

class PDODatabase implements DatabaseInterface
{
    public function __construct(/* Any relevant params */)
    {
    }

    public function connect()
    {
        //Connect using PDO
    }

    public function disconnect()
    {
        //Disconnect
    }
}

Now in the area where we connect to the database, we can use the interface to define the type and then use any of the interface functions.

<?php

namespace App\EXAMPLE;

class DatabaseService
{
    private DatabaseInterface $database;
    
    public function __construct(DatabaseInterface $database)
    {
        $this->database = $database;
    }

    public function setup()
    {
        //Any setup that needs to be done
        $this->database->connect();
    }
}

When we define the type as an interface, it allows us to swap out the implementation with another. For example, say we wanted to change our PDODatabase to a MySQLi connection. We could easily write a MYSQLiDatabase.php class which implements the DatabaseInterface.php and pass that into our DatabaseService.php constructor. And because any class that is passed into the construct must conform to the interface, we know that there will be a connect and a disconnect function. This means we don't need to touch anything in the DatabaseService.php class because we have written code that requires an interface instead of a concrete implementation.

Avoid Ambiguity

I see that you're declaring your class variables with union types, which is PHP 8 and above, but just because you can doesn't mean you should.

public null | array | string $group;
public null | string $object_name;
public null | string $procurement_type;
public null | string | object $responsible_user;

Imagine coming back to this code in 6-12 months time. Trying to debug this would be a nightmare because PHP is very forgiving when it comes to types. I would personally recommend to only one type with the allowance of null if it is required.

public ?string $group;

The same can be said for functions that return something. PHP developers are renowned for returning many different types (mixed) from functions. My personal opinion on this is that it should be avoided because it just over complicates everything later down the track.

If you want to further your knowledge on building extendable code, I would highly recommend researching the SOLID Principles. There are a lot of great resources out there to help you along the way. Start off small and get a good understanding of everything before getting too adventurous and biting off more than you can chew.

\$\endgroup\$
6
  • \$\begingroup\$ Thank you so much for the detailed answer! I tried to recreate the databaseservice, but what would I pass to the ProcurementService? Previously I passed the PDO object (connection). \$\endgroup\$ Oct 18 at 6:29
  • \$\begingroup\$ That’s where you pass an interface as opposed to a concrete implementation. Code to interfaces, not concrete classes. PDO is a concrete implementation. This means that if you ever need to swap it out for something else, you’ll need to rewrite your ProcurementService class. \$\endgroup\$
    – Savlon
    Oct 18 at 7:02
  • \$\begingroup\$ So in the ProcurementService class I cannot access PDO directly? So I should create a query function in the DatabaseService class? \$\endgroup\$ Oct 18 at 7:21
  • 1
    \$\begingroup\$ Yeh why not give that a go. If you are using this to actually create something though, I’d suggest using some kind of framework instead because it’s much cleaner and you can focus on the actual program instead of designing your own core functionality. Designing your own framework is biting off more than you can chew in my opinion. You will learn very quickly why the structure and adhering to SOLID principles is important. If you use someone else’s framework, all the complex/advanced stuff has been thought of and implemented already. If you’re doing this to learn then go for it \$\endgroup\$
    – Savlon
    Oct 18 at 7:26
  • 1
    \$\begingroup\$ There are old versions of PHP that I would not try to write OOP in. \$\endgroup\$
    – pacmaninbw
    Oct 18 at 12:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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