Using get_class feels "wrong"...

[mich2004]

Hi Guys, still new to OOP. What I'm trying to achieve is quite straightforward and I've had some success, however, it feels "wrong" how I'm implementing the classes I have created which makes me feel I have a design problem or there is a better way to achieve what I want which right now I cannot see.

 

The task is quite clear, I receive a numerical user_id, after setting up the DB connection I pass it to a factory which executes a few queries and then generates an array of objects, the array will hold the objects relevant to what the person has been, i.e. has been a player, has been a coach, referee or whatever (when I pass the id I obviously have no idea of the role a person has held, it may be player only, it could be player and coach, etc).

 

My person class is extended to specific roles, player, referee, coach etc and obviously the array of returned objects enables me to access the properties required.

 

The issue is during implementation I obviously need to know what objects I have as to what methods I can call. So attempting $my_person->refGame will throw an error if $my_person is only holding a player object.

 

So my imlementation is along the lines of this the moment..

$my_person= PersonFactory::createPerson($database, $person_id);

// my_person can be an array of objects, getName from parent class valid for all objects
$details = $my_person[0]->getName();

// We need to know what class we have returned to call specific methods...
 foreach($my_person as $dis_array){
 if (get_class($dis_array) == 'Player'){echo "I've been a player"; $my_person[0]->playerMethod; etc}
  if (get_class($dis_array) == 'Manager'){echo "I've been a manager"; $my_person[1]->somemanagerMethod; etc}
   if (get_class($dis_array) == 'Referee'){echo "I've been a Ref"; $my_person[2]->refMethod}
    }

Really using the if block to identify the classes returned feels wrong. I feel like I'm missing something here, how should I be handling a returned array of objects in implementation?

 

Also accessing each object via $my_person[0] (player) or $my_person[1] (manager) is wrong in implementation.

 

Any help much appreciated.

Edited May 29, 2014 by mich2004

[kicken]

If you want to test what type an object is, you can use the instanceof operator.

foreach ($my_person as $dis_array){
   if ($dis_array instanceof Player){ echo "I've been a player"; }
   if ($dis_array instanceof Manager){ echo "I've been a manager"; }
   if ($dis_array instanceof Referee){ echo "I've been a ref"; }
}

With regard to the echo's you could just make each class return it's name via a method like getPlayerType or something:

foreach ($my_person as $object){
   echo "I've been a ".$object->getPlayerType();
}

For the other method calls, more information about what the methods are would be needed to see if there might be another solution besides testing with instanceof.

[Psycho]

I think the foreach() loop is a problem. I would assume you want the roles of the person to display in a certain order. If the person has been both a player and a coach you likely want one to always display before the other. This layout will not enforce that.

 

I'm not sold on this array of objects within the array. But, I don't know what I would do without more analysis. But, if you are going to do it this way, then put the roles within a subarray of the object with textual keys. It will provide better structure and make the process much easier. You won't need the foreach loop and you can ensure the values will be displayed in the order you specify. I would also put the properties of each role in those subarries rather than [0], [1], [2], etc.

 

I don't know all the details of the information you are collecting for the person. But, let's say you have some generic properties such as name and age. I would store those as $person->name & $person->age. But, let's say you have a role of coach and the coach object has properties such as start_date and refMethod. You could put those into the object such that they would be accessed like so: $person->roles['Referee']->start_date and $person->roles['Referee']->refMethod

 

So, instead of a foreach() loop to try and figure out what data to display, you just need a block of code for each possible role with a check to see if that role exists.

if (array_key_exist('Player', $my_person->roles){
    echo "I've been a player"; $my_person->roles['Player']->playerMethod; etc
}
if (array_key_exist('Manager', $my_person->roles){
    echo "I've been a manager"; $my_person->roles['Manager']->somemanagerMethod; etc
}
if (array_key_exist('Referee', $my_person->roles){
    echo "I've been a Ref"; $my_person->roles['Referee']->refMethod
}
 

Edited May 29, 2014 by Psycho

[KevinM1]

I'm not sold on this array of objects within the array.

Yeah, same here.

 

Mich, can you explain your roles a bit better? Can more than one be active at the same time (as in, can a person be both a Manager and Player, or Player and Referee, or any combination of them)? Is there any shared functionality between roles (as in, do they share properties, methods, etc.)?

[maxxd]

Personally, I'd set up abstract methods in either the parent class (assuming PersonFactory is abstract) or in an implemented interface in order to present a consistent class interface, much like kicken suggested. For instance, if PersonFactory includes

public abstract function personTypeMethod();

this can be implemented in each of your person type subclasses. Manager can implement it as such

public function personTypeMethod(){
     return 'I am a Manager! Yay me!';
}

while Player includes

public function personTypeMethod(){
     return 'I am a Player! That is awesome!!';
}

Then your main code simply does something like this

$my_person= PersonFactory::createPerson($database, $person_id);

// my_person can be an array of objects, getName from parent class valid for all objects
$details = $my_person[0]->getName();

//run your stuff
foreach($my_person as $person){
    echo "{$details}: {$person->personTypeMethod()}";
}

This way your code doesn't care what type of person sub-object it's dealing with, you simply know that you can call personTypeMethod() and get a legitimate and expected response, and you've done away with long or nested conditionals and/or a giant switch() statement.

 

Obviously, the code above is a mish-mash of stuff and it looks like there'd only be one returned result from PersonFactory::createPerson(), but the thing is you could loop through an array of Person objects this way without having to specify which method you're calling and everything will work, even if you've got 2 players, 3 refs, and 1 manager in the returned array.