Breaking up a PHP class between a master class and subclasses

Question

I have a huge class taking care of companies informations in my app. Arrived at 4000 lines of code (!), I finally decided to break it up. However I am not too sure if the way I am doing this is the best. For example I had shareholders functions in the company class taking care of returning one or all, editing, adding, and deleting shareholders. And I had a whole bunch of other functions built in the same way for representatives, warrantors, offices, etc...

I did a new company class with some basic functions that I want shared with all subclasses, and the idea was to call from within this class the specialized subclasses.

However I would like my shareholder constructor to be protected in a way that it can only be called through the company class, which is impossible with the structure I chose. Also, I shouldn't be allowed to instantiate a new shareholder from the shareholder class which again I can with this construction.

I think I should be using traits and abstract class to achieve this, but I am not sure how to. Sorry if this is a bit confused, but any help is welcome.

class company{
  private $id;
  protected $db;

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

  public function get_id(){
    return $this->id;
  }
  ...
  public function shareholder(){
    if ( !isset($this->shareholder) ){
      $this->shareholder = new adherent\shareholder($this->get_id(), $this->db);
    }
    return $this->shareholder;
  }
}

// This will be implemented for all subclasses dealing with lists
interface _list {

  public function count($incl_deleted = false);

  public function get($id);

  public function get_all($incl_deleted = false);

  public function insert($cfg);

  public function update($cfg);

  public function delete($id);

}

class link extends company implements _list{
  public function count($incl_deleted = false){
    ...
  }

  public function get($id){
    ...
  }

  public function get_all($incl_deleted = false){
    ...
  }

  public function insert($cfg){
    ...
  }

  public function update($cfg){
    ...
  }

  public function delete($id){
    ...
  }
}

class shareholder extends link{

  public function insert($cfg)
  {
    // Specific to this class
    if ( is_array($cfg) &&
      isset($cfg['parts'], $cfg['id_tiers']) &&
      \bbn\str\text::is_number($cfg['parts']) &&
      ($cfg['parts'] > 0) ){
      return parent::insert($cfg);
    }
    $this->error("Problème dans la configuration pour l'insertion");
  }

  public function update($cfg)
  {
    // Specific to this class
    if ( is_array($cfg) &&
      isset($cfg['id'], $cfg['parts'], $cfg['id_tiers']) &&
      \bbn\str\text::is_number($cfg['parts']) &&
      ($cfg['parts'] > 0) ){
      return parent::update($cfg);
    }
    $this->error("Problème dans la configuration pour l'update");
  }
  // Other functions use the link class
}

Answer 1

Coding standards

I'm not going into details here. But there are coding standards psr-1 tot psr-4. I would advice you to follow them.

Another quick remark: if you need something, ask for it. And if you cant do it, throw an exception. This code shows exactly what I mean:

  public function insert($cfg)
  {
    // Specific to this class
    if ( is_array($cfg) &&
      isset($cfg['parts'], $cfg['id_tiers']) &&
      \bbn\str\text::is_number($cfg['parts']) &&
      ($cfg['parts'] > 0) ){
      return parent::insert($cfg);
    }
    $this->error("Problème dans la configuration pour l'insertion");
  }

In order to be able to do something, you need parts, id_tiers. parts needs to be a number larger then 10. So ask it:

public function insert($parts, $tiers)
{
    if (!is_number($parts)) throw new IllegalArgumeInvalidArgumentExceptionntException('parts needs to be a number');

    if ($parts < 0) throw new InvalidArgumentException('parts needs to be larger then 0');

    //do the actual inserting
}

But the problem you had to solve with that nifty && stuff is that you are extending a Link that extends a Company. And that is how I come to my conclusion:

I don't think you understand the concept of 'OO'. You use words like interface, class, protected and that's it.

Encapsulate everything

What you did is create one GODObject. It knows all, does all and will eventually break all. A good thing to start is to define all your Domain Models. Domain Models are defined by state, and are nearly always stored in some persisting storage (e.g. a Database).

Domain Models

Domain Models represent state and give us a set of tools to easily access that state. A Domain model is by definition stupid:

class MyDomainModel
{
    private $attribute;

    public function getAttribute()
    {
        return $this->attribute;
    }

    public function setAttribute($value)
    {
        $this->attribute = $value;
    }
}

It gives you a set of tools to pass around state. In your case, there will be multiple Domain models:

• Company
• Shareholder
• Representative
• Warrantor
• Office
• ...

A Shareholder and a Representative will probably be special cases of a User:

class User
{
    private $name;
    /** getter + setter for name */
}

class Shareholder extends User
{
    private $someShareholderStuff;
}

class Representative extends User
{
    private $someRepresentativeStuff;
}

That makes sense. Your code doesnt, here is why: A link is a Company and implements the a _list. A Shareholder is a link (and thus also a Company). A Company has multiple Shareholders. Thus, a Company has multiple Companies. And just sounds strange.

What is beging done? And who is doing it?

This is a mistake a lot of people make. They add methods to the wrong class. This comes from the 'We use MVC and cram everything into either a Model, a View or a Controller'. And they end up writing code like this:

class User extends BasicModel
{
    public function sendRegistrationMail()
    {
        $this->mailer->sendEmail($this->email, 'Lorem Ipsum registration');
    }
}

and use it

$user->sendRegistrationMail();

Some might event argue that there is nothing wrong with this code. But lets look at what is going on. Somewhere down the road, the USer Object - that was a simple representation of state - learned how to use the MailerInterface. And now everytime we create a User, we also need a Mailer to pass to the constructor. The target has become the actor.

Much better code would be:

$mailer->sendEmail($email, $user);

The Email and the User care less about how the email is send. They don't even know an email is send. They shouldn't, they are but dum objects that represent state.

I want my constructors to be protected

This reminds me of the Singleton anti-pattern. If you don't want multiple instances to be created, then don't create multiple instances. Same goes for your Shareholder class. If you don't want it to be created outside a given context, then don't. Code doesn't magically appear.

And think of testing. One day you will have to stub a Shareholder, and then writing new Shareholder('name'); without the app complaining will be heaven.