patternphpMinor
City and District class examples for teaching OOP in PHP
Viewed 0 times
districtphpandexamplesteachingforcityclassoop
Problem
I wrote these two classes for teaching OOP in PHP. Do you think it's breaking some good practices or SOLID principles?
id = $id;
$this->name = $name;
}
public function id()
{
return $this->id;
}
public function name():string
{
return $this->name;
}
public function addDistrict(District $district)
{
if (!isset($this->districts[$district->id()])) {
$this->districts[$district->id()] = new District($district->id(), $this, $district->name());
}
}
public function getArray():array
{
$districts = [];
foreach ($this->districts as $district) {
array_push($districts, $district->getArray());
}
return ['id' => $this->id(), 'name' => $this->name(), 'districts' => $districts];
}
public function districts():array
{
return $this->districts;
}
}
class District
{
private $id;
private $cityId;
private $name;
public function __construct($id, City $city, string $name)
{
$this->id = $id;
$this->name = $name;
$this->cityId = $city->id();
}
public function getArray():array
{
return ['id' => $this->id, 'name' => $this->name];
}
public function id()
{
return $this->id;
}
public function name():string
{
return $this->name;
}
}Solution
Here are some questions I'm asking myself related to your chosen design:
Unfortunate connection between
In a
But I fail to see the good usage of these classes, as they seem to miss any useful methods. Like you can't look up a district and find the corresponding city. Neither can you do anything besides creating or deleting the cities and districts. A good object design would make the object have all needed variables and methods for handling related functions.
Lastly regarding the
- Why not return type on the
id()functions – You have return types on most of the other functions, but why not on either of theidfunctions?
- Why the
getArray()? – This seems like something used by functions likevar_dump(), but then it would be better to implement __debug_info, whichvar_dump()could take usage of directly.
- Possibly rename the private variables – I'm not sure if php has naming standards, but in my view having both a private variable
nameand a functionname()seems kind of redundant, and possibly unfortunate. I would possibly rename the variable into_nameor similar.
Unfortunate connection between
District and CityIn a
City you might add Districts, and each district has links to the id of the cities, making a complete circle. This could be unfortunate, and possibly dangerous. Still you don't have a circular reference as you store only the city id, and not a complete reference to the City object. But I fail to see the good usage of these classes, as they seem to miss any useful methods. Like you can't look up a district and find the corresponding city. Neither can you do anything besides creating or deleting the cities and districts. A good object design would make the object have all needed variables and methods for handling related functions.
Lastly regarding the
addDistrict() function, I think I would find it natural to allow the City class to fully create districts. That is to have function addDistrict($id, $name) within City class, and then let it create the District object.Context
StackExchange Code Review Q#121320, answer score: 2
Revisions (0)
No revisions yet.