Am I utilizing classes and OOP efficiently?

Question

In my last question on Stack Overflow I was a very confused newcomer about classes and OOP and how to correctly implement everything to make things easier. So I started a different example to practice.

This has the classes of a band and person. The people audition but only some get in and the rest leave to form their own group. Then there's a battle of the bands and bands face off. Here is my code.

<?
class Start{
    var $name;
    var $type;
    //If construct not set, this is the construct.
    function __construct($name){
        $this->name = $name;
    }
}


class Band extends Start{
    var $name;
    var $type;
    var $membersAndInstruments = array();
    var $badArray = array();

    //Specify starting parameters.
    function __construct($type, $name){
        $this->type = $type;
        $this->name = $name;
        echo isset($this->name)
        ? "This " . $type . " band called " . $this->name . " has just been formed!" //If name is set
        : "This band was formed. Genre: " . $type; //If name isn't.
    }

    //Add new members. Instrument => Member
    function recruitMembers($instrument, $member){
        $this->membersAndInstruments[$instrument] = $member;
        //echo $instrument;
    }
    //Reject members from the audition
    function reject($instrument, $rejectee){
        $this->badArray[] = array($instrument => $rejectee);
    }

    //Show the ones who didn't make it
    function sorryReject($msgStart, $msgEnd){
        echo "<br /><br />$msgStart, ";
        foreach($this->badArray as $k => $v){
            //$k = array_values($k);
            $v = array_values($v);
            echo $v[0];
            if ($k != count($this->badArray)-1){
                echo ", ";
            }
        }
    echo ". $msgEnd</br>";
    }

    //Show the members in the group and their instruments.
    function displayMembers(){
        foreach($this->membersAndInstruments as $craft => $member){
            echo "<br />" . $member . " as " . $craft ;
        }
    }

    //Challenge other bands. Input a band or an Array of bands.
    function battle($band2, $winMsg){
        echo "<br /><br /> BATTLE OF THE BANDS <br /><br />";
        $band1 = $this;
        if (!is_array($band2)){
            $contestants = array($band1, $band2); //Ready bands
        }else{
            $contestants = array($band1);
            foreach($band2 as $bandX){
                $contestants[] = $bandX;
            }

            //var_dump($contestants);
        }

        //Count members once and store in array of the same order the bands were entered
        for($i = 0; $i < count($contestants); $i++){
            $numOfMembers[] = count($contestants[$i]->membersAndInstruments);
        }


        //echo "<br />";

        //var_dump($contestants);
        //var_dump($numOfMembers);

        $moreMembers = ($numOfMembers[0] > $numOfMembers[1]) ? 0 : 1; //Set to the index that has more.
        $winner = Randomizer($contestants, $contestants[$moreMembers], $numOfMembers[$moreMembers]);

        echo $winner->name . " $winMsg";
    }

}

class Person extends Start{
    var $goodAtInstrument;

    function __construct($name, $good){
        $this->name = $name;
        $this->goodAtInstrument = $good;
    }
    function audition($band, $good, $instrument){
        $this->goodAtInstrument = $instrument;
        $instrument = killTheLastLetter($instrument, "s");
        $instrument != "Drum" ? $instrument .= "ist" : $instrument .= "mer";
        if ($good){
            $band->recruitMembers($instrument, $this->name);
        }else{
            $band->reject($instrument, $this->name);
            //echo "<br /> Sorry, " . $this->name . "....you didn't make it.";
        }
    }
}




//Looks for a specified ending letter and cuts it if found and returns.
function killTheLastLetter($input, $cutThisLetter){
    if (substr($input, -2, 2) == "ss"){
        return $input;
    }
    substr($input, -1) == $cutThisLetter
    ? $input = substr($input, 0, -1)
    : $input = $input;
    return $input;
}


function Randomizer($choiceArray, $chanceChoice, $chances = 0){
    !$choiceArray
    ? $TorF = array(true, false)
    : $TorF = $choiceArray;
    for($i = 0; $i < $chances; $i++){
        //echo "DOJNE";
        $TorF[] = $chanceChoice;
    }
    //var_dump($TorF);
    $random = array_rand($TorF);
    //echo $random;
    return $TorF[$random];
}

//====================================================================================================
$FirstRockBand = new Band("Progressive Rock","Erais");


$bandMembers = array (
    "Bass" => $M1 = new Person("Shepard", Randomizer()),
    "Guitar" => $M2 = new Person("Sy", Randomizer()),
    "Drums" => $M3 = new Person("Chris", Randomizer()),
    "Keyboards" => $M4 = new Person("Jill", Randomizer()),
    "Vocals" => $M5 = new Person("Michele", Randomizer()),
    "Cowbell" => $M6 = new Person("Ben", Randomizer())
);


echo "<br /> Welcome to the auditions. <br />";

foreach($bandMembers as $k => $v){
    $v->audition($FirstRockBand, $v->goodAtInstrument, $k);
}


$FirstRockBand->displayMembers();


//echo count($FirstRockBand->badArray);

if (count($FirstRockBand->badArray) > 0){


    $FirstRockBand->sorryReject("Sorry", "you didn't make it in.");


    echo "<br /><br /> The rejected members have started their own group: " . $SecondRockBand->name . "<br /><br />";


    $SecondRockBand = new Band("Death Metal", "Sideshow Hubris");
    $RockBands = array(
       $FirstRockBand,
       $SecondRockBand
            );


    foreach($FirstRockBand->badArray as $k => $v){
        foreach($v as $inst => $member){
            $SecondRockBand->recruitMembers($inst, $member);
        }

    }



    //echo '<div style = "background-color: 00FFFF">';
    $SecondRockBand->displayMembers();
    //echo "</div>";

//Battle of the Bands! Scales tipped on the side that has more members. Need to work on the whole talent thing.

    $FirstRockBand->battle($SecondRockBand, "blew the crowd away!");

}

Some things seem iffy to me, like plugging members into an array and then going through other arrays and so on. But PHP isn't exactly interactive so unless I'm reading from a DB all answers should already be in there....

Anyway, if you could please look this over and tell me if this is confusing or if I seem to be using classes correctly and what else I could fix.

Answer 1

I think you have to rethink your code (- ;

as palacsint pointed out, there is some bad naming involved but whats more troublesome is why you extend "start".

the only reason i can think of why you did this is that person and band has "name" in common. Well thats not how it works. You do not put common things into parent classes and extend these as you see fit (at least it is not a good design)

Band and Person are totaly seperate enteties. There is nothing to gain to have the same parentclass.

If you want to make sure that everything which has a name will implement something, use interfaces. You could do something like this

interface HasName {
    public function getName();          
}

class Band implements HasName {}
class Person implements HasName {}

Also there is the possibility to implement multiple interfaces but you can extend only from one parent. The name of the interface "HasName" is already a hint that it is not a good interface. If interested you should read about interfaces and naming guides.

So how would i do it?

If you want "default stuff" to inherit from use an abstract class

abstract class Band {
    //...
    public function addMember(Person $Person) {

    }

    public function getMembers() {

    }
    //...
}

You put all the stuff every band has in common in there, you forge the behavior of "Band" every Band will behave like the Abstract class this has many benefits.

Now you have the possibility to extend this class if you would like to add features like

class RockBand extends Band {
    public function smashGuitar() {

    }
}

Also you should seperate concerns, i dont think "battle" is a concern of Band nor is "audition" a concern of Person. These are methods you should seperate in own classes or functions.

+++ EDIT

interface CanBattle {
    public function battle();
}

abstract class BandBattle {
    public function addParticipant(CanBattle $Participant) {}
    public function battle() {}
    public function getWinner() {}
}

class HiphopBattle {
    public function battle() {
        // other logic then rock band battle
    }
}

class RockBattle {
    public function battle() {
        // other logi then hiphop battle
    }
}

As you can see you have now the power to implement as many different battles as you please. You do not have even to touch Person or Band to do this (thats the power of seperation) The only thing Band and Person have to do is impelment the interface "CanBattle" as the battle does not care if you ar a Band (rock) or a person (hiphop), i hope this examples will help you.

--- EDIT

Same for the output methods. You could use the __toString method and implement it for Band to output all Members but a better idea would be to seperate output into another class or function. So Band would only return the Data and the other one would present it.

Why do you set goodAtInstrument in Person once in the constructor and then again in the audition?

I think there would be more but i have to wait for your answers befor going to far (- : Hope i could help.

Answer 2

I would accept braunbaer's answer, this is merely an addition.

OOP Review

Inheritance

As braunbaer said, these classes are nothing alike and so they shouldn't be inheriting from the same source. However, you are not using inheritance right anyways. If class "Start" has a method that sets the "name" property, there is no need to type that again in the overriding child method, you should call parent::__construct(); within the child constructor method at the appropriate point instead. But this is a bad example because it only performs one task and that task isn't any shorter than rewriting it. Assume all of your classes set the "type" and "name" properties and performed a shared method. Of course for this the parent class should also have this shared method as well, but we will assume it does for now. Then you can immediately see the benefit of calling the parent::construct() method instead of retyping all of that for each child method. What you have right now might as well be an interface because you are not reusing any of its code :)

Encapsulation

The true purpose for OOP is not just code reuse, but encapsulation. In other words, hiding information from the users. Your classes don't do this. This isn't really a necessity, but it is one of the key features of OOP and a good one. Right now all of your methods and properties have defaulted to public because none of them are properly defined. PHP 5 still supports declaring class properties with var but it is an old and outdated method that will probably become deprecated at some point. The proper way is to use the property's state (public, private, or protected) to declare them. This also holds true for class methods, though they still require the "function" keyword.

General Code Review

Variables

Your variables should be descriptive. For example, $k and $v as defined in your foreach loop in the sorryReject() method could be renamed $instruments and $musicians respectively. This is a form of self documentation that ensures everyone who reads your code understands what those variables indicate.

var_dump()

You shouldn't dump variables into the view like this. It is very messy. var_dump(), exit(), die(), etc... (even echo() in my opinion) are all debugging tools that should not make it into the final release of a program. They are inelegant and counter-intuitive to separation of code, but that is a more advanced venture into OOP (see MVC) so I won't get into it here :)

Ternary Operations

Ternary operations are nice, but should only be used when they benefit your code, not as a short hand. So the following should be expanded.

if( isset($this->name) ) { echo "This $type band called {$this->name} has just been formed!";
else { echo "This band was formed. Genre: $type"; }

So a better example of a "good" ternary operation:

$name = isset($this->name) ? $this->name : 'default';

Though it is argued that ternary is never good practice. I myself believe it ok if it is implemented similar to the later example.

Strings

You'll also notice I fiddled with your strings in the above code. Double quotes (" ") tell PHP that it needs to process the incoming string, that there are PHP variables or entities needing to be escaped inside. Single quoted (' ') strings are not processed and will produce the same string that you typed, letter-for-letter, symbol-for-symbol. So your double quoted string was not being used appropriately. You told PHP there were variables or entities to escape and then manually escaped them yourself.

Answer 3

Just two quick notes:

1. From Clean Code by Robert C. Martin:

   A class name should not be a verb.

   So, Start does not seem a good name.

2. Furthermore, it seems that Band "is-a" Start is not true. Check HAS-A, IS-A terminology in object oriented on StackOverflow and the linked resources in that question.