1
\$\begingroup\$

I already learned JAVA, a few design patterns (Controllers, etc.) and OOP. I'm trying too build some PHP scripts in OOP (a blog). I want to collect some articles from the DB and store in an array. I'm doing this with a repository (like I do this is JAVA). Is this correct?

Artikel.php :

class Artikel {
private $id;
private $naam;

function __construct($id, $naam) {
    $this->id = $id;
    $this->naam = $naam;
}
function getId() {
    return $this->id;
}

function getNaam() {
    return $this->naam;
}

function setId($id) {
    $this->id = $id;
}

function setNaam($naam) {
    $this->naam = $naam;
}
}

ArtikelMapper.php

    require_once '../Classes/Artikel.php';
    class ArtikelMapper {
    private $database;

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

    function getArtikels(){
        /* become a mysql query, this is just for showing */
        $array = array();
        $art1 = new Artikel(1, "name1");
        array_push($array, $art1);
        $art2 = new Artikel(2, "name2");
        array_push($array, $art2);
        return $array;
    }
}

ArtikelRepository.php

require_once '../Classes/Artikel.php';
require_once '../Mappers/ArtikelMapper.php';
class ArtikelRepository {
    private $artikelMapper;
    private $artikelArray;

    function __construct($database) {
        $this->artikelMapper = new ArtikelMapper($database);
        $this->artikelArray = $this->artikelMapper->getArtikels();
    }

    function getArtikels(){
        return $artikelArray;
    }

    function addArtikels($artikel){
        array_push($this->artikelArray, $artikel);
    }

}
\$\endgroup\$
  • \$\begingroup\$ "Storing" the artikel instances in a repository property is kind of a pointless exercise in PHP. Unlike Java, PHP is stateless, so whatever you store in that array is lost when the response is sent back to the client. In other words: you'll load the data from the DB for every request over and over again. This will increase memory consumption, slow the response time, and is something that's never really done. Much more common would be to add a separate caching layer to the DBAL, which the repository is unaware of \$\endgroup\$ – Elias Van Ootegem Aug 14 '16 at 15:06
1
\$\begingroup\$

This is pretty straight forward code and there is not really much here to provide much critique to. The biggest concern to me would be in the area of data validation. You are not currently doing anything to validate that you have appropriate parameters being passed to your methods. You should consider adding type hinting for your parameters to better enforce that, for example, you are getting a valid database object to work with or a valid Artikel object to add to the repository.

Similarly for thing like setting article id and name where you might be expecting a postivive integer or non-zero length string, you should validate this in your methods and perhaps throw InvalidArgumentException if validation fails.

I question the need for setters on Artikel class if these objects would only be instantiated in the mapper process and are tied to actual database records. Should these objects actually be mutable like this? If not, I would actually consider using __get() magic method here instead of the getter you specified to allow more clear and concise access to these properties (I.e. $article.id or $article.naam) while still making the object immutable.

I would encourage you to properly indent your code in a consistent manner and always use single empty lines between class methods consistently. Also, place empty lines between your includes and the beginning of class definitions.

One final small thing, and it may just be my preference, is that you might consider using protected visibility instead of private when first writing these base classes. I would generally only use private when there will be at least one inheriting class and I had a specific reason to actually restrict visibility to those inheriting classes.

\$\endgroup\$
  • \$\begingroup\$ Thank you soo much for your help. Offcourse I add exceptions (like I learned in JAVA). And I going to look for the __get() methods! \$\endgroup\$ – jeremy-vdw Aug 13 '16 at 8:37
  • \$\begingroup\$ @jvdw122: Please don't use __get methods. Magic methods too much can significantly reduce performance, they make code harder to test and at the same time make it more error-prone. Say I have a class with a foo property, but write something like $instance->fo. The __get method will get invoked and probably return null (because the property doesn't exist). I will not get a fatal error (as I would with getFoo() being written as getFo()), which is far from ideal \$\endgroup\$ – Elias Van Ootegem Aug 14 '16 at 14:35
  • \$\begingroup\$ @EliasVanOotegem A properly implemented __get() method should throw an appropriate exception when trying to access a non-existing property. Performance concerns would be what I consider a micro-optimization. I think clarity in unit testing and in reading code are the most valid cons against such usage. \$\endgroup\$ – Mike Brant Aug 16 '16 at 17:08
  • \$\begingroup\$ @MikeBrant use of __get can be considered micro-optimization in many cases. I have, however worked on a code base where virtually all model classes used magic methods. After some profiling, we decided to refactor the models, which did make a really significant performance difference. I wrote a python script to facilitate the refactoring work. It's been a while since I ran some benchmarks, but judging by the PHP C implementation of __get & co, the performance difference should still be relatively significant \$\endgroup\$ – Elias Van Ootegem Aug 16 '16 at 23:33
2
\$\begingroup\$

At first glance, a couple of things I'd change about your code are:

  • Follow coding standards, PSR being the most commonly adopted standard out there. Class definitions and method definitions have the opening { on a separate line.
  • Specify the visibility of properties (as you do) and methods. Relying on the implied public will work for now (might change in the future), but it's best to be explicit: self-documenting code is what you should be aiming for
  • use type-hints as much as you can. In particular your ArtikelRepository class: the constructor takes a $database argument, but what is it supposed to be? You're not performing any checks to see whether or not the value that was passed was indeed the expected one. I could've passed a string, null or anything in between. Same goes for the addArtikels method, and the ArtikelMapper class.
  • Method names should make sense. addArtikels is plural, so going by the name, I expect to pass an array of Artikel instances, but you're using array_push on a property that is a 1-dimensional array of Artikel instances. The correct name for that method would be addArtikel.
  • The ArtikelMapper class should not depend on the DB. It doesn't even look like a mapper. A mapper maps a set of data in a particular format (arrays, class instances, ...) onto another format (eg query results returned as arrays can be mapped onto Artikel instances). That's its job, exposing the DB to this class does not make sense.
    This suggests the mapper class you have is violating the Single Responsibility Principle (SRP).
  • The use of require_once all over the place seems to suggest you're not using an autoloader, which is fine for quick, small tests or proof-of-concept type work, but for actual applications, use namespaces and a PSR compliant autoloader.
  • Personal preference perhaps: but entity setters are, IMHO, best fluent (return $this;) so they can be used in a single statement without having to repeat the variable to which you've assigned the instance.

Basically, here's how I'd write the Artikel entity:

class Artikel
{//new line
    /**
     * @var int
     */
    private $id;

    /**
     * @var string
     */
    private $naam;

    /**
     * @param int $id
     * @param string $naam
     */
    public function __construct($id, $naam)
    {//new line
        $this->id = $id;
        $this->naam = $naam;
    }

    /**
     * @return int
     */
    public function getId()
    {
        return $this->id;
    }

    /**
     * @return string
     */
    public function getNaam()
    {
        return $this->naam;
    }

    /**
     * @param int $id
     * @return $this
     */
    public function setId($id)
    {
        $this->id = $id;
        return $this;
    }

    /**
     * @param string $naam
     * @return $this
     */
    public function setNaam($naam)
    {
        $this->naam = $naam;
        return $this;
    }
}

The setters could perform some basic validation on the arguments passed (eg make sure $id is an integer), in which case, the constructor should not assign the properties directly, but rather call the setters, too.

Along with the (mainly style) changes to the entity class, I'd refactor the repository class, too:

class ArtikelRepository
{
    /**
     * @var ArtikelMapper
     */
    private $artikelMapper;

    /**
     * @var Database <-- what class is this?
     */
    private $database;

    /**
     * @var Artikel[]
     */
    private $artikelArray = [];//initialize to array already

    /**
     * Do NOT create depenendencies in constructors, INJECT dependencies!
     * In a true Respositor, the database connection is a dependency of the Repository, not the mapper
     * @param Database $database
     * @param ArtikelMapper $mapper
     */
    function __construct(Database $database, ArtikelMapper $mapper)
    {
        $this->database = $database;
        $this->artikelMapper = new ArtikelMapper($database);
        //Don't use a constructor to query the DB, only load data if we need it
        //$this->artikelArray = $this->artikelMapper->getArtikels();
    }

    public function getArtikels()
    {
        if (!$this->artikelArray) {
            $results = $this->database->getArtikels();//load from DB
            //use mapper to convert results into Artikel instances
            $this->artikelArray = $this->artikelMapper->mapResults($results);
        }
        return $this->artikelArray;
    }

    /**
     * @param Artikel $artikel
     * @return $this <-- fluent interface
     */
    function addArtikel(Artikel $artikel)
    {
        if (!$this->artikelArray) {
            $this->getArtikels();//initialize artikels if needed
        }
        $this->artikelArray[] = $artikel;//don't use array_push
        return $this;
    }
}

I don't know what it is you're trying to do exactly, but I'm not sure why you're actually loading all existing artikel instances and assigning them to your repository property. A repository fetches data from a persistence layer in a format that the rest of the application understands. Nothing more, or less. You can add some caching mechanisms to avoid hitting the DB for data you already loaded, but in that case, it'd make more sense to use $artikelArray as an associative array (using the ID as key). To be honest, I wouldn't bother with that property at all for now, and perhaps consider adding a specialized caching layer later on.

\$\endgroup\$

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.