2
\$\begingroup\$

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.

\$\endgroup\$
5
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Hey thanks sorry it took so long to accept and get back to you. Before I never knew anything about the existence of abstract classes and interfaces or toString...why do you say that battle is not a concern of Band or audition the concern of Person? The way I understand it is appropriate methods of classes are things that the object would do, a verb. A person can audition for a band and a band can challenge another band so I don't quite understand... and I'm not sure why I set good in two places....I think I was intending something different at the time. \$\endgroup\$ Jun 6 '12 at 4:54
  • \$\begingroup\$ @user1159454 added an example for seperation (see edit), hope that helps to clear things up. \$\endgroup\$
    – braunbaer
    Jun 7 '12 at 20:31
3
\$\begingroup\$

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.

\$\endgroup\$
6
  • \$\begingroup\$ Thanks The only reason that I made the Start class was so even if nothing was set in a new class it would still have or want a name. I'll look into parent::construct. Now by calling the $k and $v variables as instrument and musician, I won't be interfering with other variables will I? I usually call foreach vars k and v so that I never interfere with variables outside of the loop…I just had some exceptions this time around. \$\endgroup\$ Jun 6 '12 at 5:10
  • \$\begingroup\$ I don't intend to have var_dump in final releases, I use it simply to see what's going on in the background, what's wrong with it? On ternary, isn't ternary or not a matter of preference? What is wrong with it? I usually use ternary if the if/else statement is very very short (1 line). I can understand if it gets complicated and nested, but this is one line and one if/else condition. I'd figure it's slightly faster as well, only being on one line. When would it "benefit code"? \$\endgroup\$ Jun 6 '12 at 5:11
  • \$\begingroup\$ On strings, I know what you mean, I just haven't broken the habit of using double quotes everywhere. I'll have to work on that. Single in static strings and double in strings with escape characters. However I find it easier to read when concatenating variables instead of just putting them in the quotes. Does this interfere with speed? Thanks! \$\endgroup\$ Jun 6 '12 at 5:11
  • \$\begingroup\$ @user1159454: Better code separation by method will help ensure no variable conflicts, but as long as you don't use the same variables in the same scope you should be fine. Variable conflicts are only a major concern in procedural code. \$\endgroup\$
    – mseancole
    Jun 6 '12 at 12:21
  • \$\begingroup\$ @user1159454: There's nothing wrong with var_dump(), just that it is an inelegant way of outputting data to the browser. Ternary is a matter of preference. You say you are doing it only for one line statements, but the ones I see are spread across two.. If they are so large that you are spreading them across multiple lines to improve legibility, you might as well be using those if statement. Its true they also shouldn't be nested. And I actually think ternary is slower... It benefits your code when it can help shorten your code without reducing legibility. \$\endgroup\$
    – mseancole
    Jun 6 '12 at 12:28
2
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Alright, the only reason I called it Start was because it looked like default was already taken. Maybe a constant. I'll keep those in mind thanks. Are these the biggest problems you see? \$\endgroup\$ Jun 5 '12 at 8:26
  • \$\begingroup\$ @braunbaer was faster, +1 for him :) \$\endgroup\$
    – palacsint
    Jun 5 '12 at 19:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.