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.
