HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

City and District class examples for teaching OOP in PHP

Submitted by: @import:stackexchange-codereview··
0
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:

  • Why not return type on the id() functions – You have return types on most of the other functions, but why not on either of the id functions?



  • Why the getArray()? – This seems like something used by functions like var_dump(), but then it would be better to implement __debug_info, which var_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 name and a function name() seems kind of redundant, and possibly unfortunate. I would possibly rename the variable into _name or similar.



Unfortunate connection between District and City

In 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.