Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I just want an indication as to whether or not I'm on the right track regarding PHP OOP, at least on a basic level. Positive criticism welcome.

P.S. Excuse the visuals of the code. This is how I usually remember code.

<html>
<?php

//Blueprint

class Human{

    // properties of human
    private $gender;
    private $name;
    private $age;
    private $talk;

    public function __construct($gender, $name, $age, $talk){
        $this->gender   = $gender;
        $this->name     = $name;
        $this->age      = $age;
        $this->talk     = $talk;
    }

    public function getGender(){
        return $this->gender;
    }   

    public function getName(){
        return $this->name;
    }

    public function getAge(){
        return $this->age;
    }

    public function getTalk(){
        return $this->talk;
    }
}

class female extends Human{

    //properties of female
    private $bum;
    private $feet; 

    public function __construct($gender, $name, $age, $talk, $bum, $feet){
        parent::__construct($gender, $name, $age, $talk);

        $this->bum  = $bum;
        $this->feet = $feet; 
    }

    public function  getBumSize(){
        return $this->bum;
    }

    public function getFeetSize(){
        return $this->feet;
    }
}

class male extends Human{

    //properties of male
    private $facialHair;
    private $adamsApple;

    public function __construct($gender, $name, $age, $talk, $facialHair, $adamsApple){
        parent::__construct($gender, $name, $age, $talk);

        $this->beard    = $facialHair;
        $this->throat   = $adamsApple; 
    }

    public function getBeard(){
        return $this->beard;
    }

    public function getThroat(){
        return $this->throat;
    }
}

//instantiate objects
$malePerson     = new male('male', 'John Doe', '32', 'clear', 'beard', 'adams apple in my throat');
$femalePerson   = new female('female', 'Jane Doe', '28', 'loud', 'big and round', 'small');


echo "Hi, My name is {$malePerson->getName()}, {$malePerson->getAge()} years of age, I talk {$malePerson->getTalk()} 
        so i guess that makes me {$malePerson->getGender()} a.k.a man....I also have 
        a {$malePerson->getThroat()} and a {$malePerson->getBeard()} which is very very long .lol";
echo "<br>";
echo "Hi, My name is {$femalePerson->getName()}, {$femalePerson->getAge()} years of age, I talk {$malePerson->getTalk()} so i 
        guess that makes me {$femalePerson->getGender()} a.k.a mamalicious....I also have 
         {$femalePerson->getFeetSize()} feet and a {$femalePerson->getBumSize()} bum.lol";
?>
</html>
share|improve this question
    
thanks for the edit Jamal, how did you do it so quick. –  Wayne Links Feb 14 at 17:19
5  
I've been editing here for quite a while. :-) Plus the formatting only requires selecting all the code and pressing CTRL+K. –  Jamal Feb 14 at 17:27
    
so Jamal what do you think, am i on the right track, i am trying to wrap my head around PHP OOP. I think the example covers atleast the basis –  Wayne Links Feb 14 at 17:29
    
I'm not familiar with PHP, so I'm afraid I cannot offer much. –  Jamal Feb 14 at 17:31
3  
The Human class must be abstract. Think it like this, you can "create" a Female or a Male, even a Transgender :P, but they are Humans. You can not create such thing as a human, therefore it must be abstract. –  gonzalon Feb 15 at 0:05

4 Answers 4

up vote 8 down vote accepted

If you would put one or more introduceYourself() functions inside your class, then you wouldn't need that many getter-methods. It is easier, and often recommended, to tell the object to do something, don't ask for it's information and then do something with that information. This is a principle called: Tell, don't ask.

Why do you pass $gender variable to your constructor? Aren't all males male and all females female?

Your a.k.a. text is hard-coded into the calling code and not a property of any human. Switching the $malePerson to become a female would print that makes me female a.k.a man. You would have to modify code in several places to fix this, which indicates that this is a "code smell".

I really think there are more interesting aspects of females than storing their "bum" size. Try their hair color instead.

Rewriting your code a little would create something like this:

$malePerson     = new male('male', 'John Doe', '32', 'clear', 'beard', 'adams apple in my throat');
$femalePerson   = new female('female', 'Jane Doe', '28', 'loud', 'big and round', 'small');
$malePerson->introduceYourself();
$femalePerson->introduceYourself();

As for the introduceYourself code, it could look something like this:

public function introduceYourself() {
    echo "Hi, my name is $name, $age years of age I talk $talk so I guess that makes me {$this->getGender()}...";
}

You could override the getGender() method for males and females so that instead of passing the variable to the constructor, the class returns the proper value for this.

public function getGender(){
    return "female";
}

You might want to read up on Class Abstraction in the PHP Manual and consider making your Human class into an abstract class.

share|improve this answer
4  
Simon, thanks for the comment. The Tell, don't ask principle, it actually makes sense, never thought about it before. It will certainly improve my future code –  Wayne Links Feb 14 at 22:07

First of all: Extract your classes into extra files and require_once them as necessary. I found the Java style of having one class per file a very good one.


Second: Learn and start to use PHPDoc to document your code.


//Blueprint

That's not a helpful comment...only write a comment if it is helpful.


// properties of human
private $gender;
private $name;
private $age;
private $talk;

Such structural comments are unnecessary if your class is correctly structured and indented.


$this->gender   = $gender;
$this->name     = $name;
$this->age      = $age;
$this->talk     = $talk;

Personally I don't like this kind of intending. It can make code easier to read, but in a pure "assign all parameters to all fields" constructors it seems kinda unnecessary.


class female extends Human{

Is it a typo that female is lower case?


private $bum;

If it holds the size of the bum, then it should be called $bumSize. The same goes for $feet.


$malePerson     = new male('male', 'John Doe', '32', 'clear', 'beard', 'adams apple in my throat');

Apart from the unnecessary intending, this is a good example stringly typed programming. You're using a string for everything even though the age should be an integer.

Let's have a look at the basic variables a Human consists of:

  • $gender Should an enumeration with two values, Male and Female.
  • $name Names are strings, so it's okay.
  • $age Is an integer and should therefor be an integer.
  • $talk Is also a string.

And let's go forth with the Male:

  • $facialHair Should be an enum, or an integer to indicate strength.
  • $adamsApple Not sure what this is.

Also forget what I just said about $gender, Human should be an abstract class and getGender() should be an abstract method that is implemented by the next class.


<html>
     <?php Some code with echo... ?>
</html>

That's not a well-formed HTML document.

<html>
    <head>
        <title>Title of your page</title>
    </head>
    <body>
        Content goes here.
    </body>
</html>

Ideally you would perform initializations before the html tag and then just use everything prepared in the body.

share|improve this answer
    
thanks for pointing out the scenario of stringly typed. regarding enums and abstract classes, i have not covered that before, that will be my next study topic –  Wayne Links Feb 14 at 18:57
    
Perhaps I should have read your answer completely before posting my own, I just noticed that I brought up one thing that you already mentioned. But oh well, it is quite important but at least we don't need a big flashy gif this time :) –  Simon André Forsberg Feb 14 at 19:39
    
@SimonAndréForsberg: Nah, it's okay. I consider every answer a review of it's own, if something repeats, there's nothing to worry about. And yes, hopefully we will never need that again. :) –  Bobby Feb 14 at 19:41
    
+1, however, the HTML document should also include the HTML 5 doctype. –  ComFreek Feb 14 at 20:52
1  
That's not a valid HTML document. Actually it is valid, HTML, HEAD and BODY tags can be omitted. For HTML when its contents don't start with a comment, for HEAD when it's empty or if the first thing in it is an element, and BODY when it's empty or it's first element is not a comment, not a space and not an element also used in HEAD. It does need a <!DOCTYPE html> tag to make it valid HTML. w3.org/TR/html/semantics.html#the-html-element and it's containing links to the head and body elements show when they can be omitted. –  Rinze Smits Feb 15 at 13:52

Your modeling approach seems right so far for me. Yet some minor remarks in general:

  • I'd consider it good practice to start class names in upper-case. Either way, you should decide and stick for one way: Human <-> female
  • I'm no native speaker. Yet as far as I do know, bum is slang which should be avoided as someone might feel offended or simply not understand it.
  • // properties of human < that's an awfully obvious statement. It takes up time and space yet provides no information.
  • I'd consider it standard to have a whitespace between a class declaration and the starting bracket: class Human {. Same goes for function declarations: function getTalk() {.
share|improve this answer
    
you're right about the slang my bad, thank you for your observations much appreciated –  Wayne Links Feb 14 at 18:46

An abstract class forces the parent class to be extended. For instance, in general, you cannot have a human that does not have a declared gender so you can write your code as such:

abstract class Human {
    private $name;

    function __construct($name) {
        $this->name = $name;
    }


    abstract protected function getGender(); // let's say the gender is a secret, only the human and his/her ancestors know what gender this person is ;-)
    protected function getName() {
        return $this->name;
    }

    protected function echoInfo() {
        echo "My name is " . $this->getName() . " and because you are a really close friend, I will let you know I am a " . $this->getGender() . ".";
    }
}


class FemaleHuman extends Human {
    protected function getGender() { return "female"; }
}

class MaleHuman extends Human {
    protected function getGender() { return "male"; }
}


$me = new MaleHuman("Jonathan");

$me->echoInfo(); // returns My name is Jonathan and because you are a really close friend, I will let you know that I am a male.
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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