5

I've searched around but couldn't find a definitive answer (if there is one) on using $this within a PHP class. I'm still trying to wrap my head around using the OOP approach and want to make sure i'm using the best practices.

So my question is around how and when you should define vars and when you should use $this to reference them.

Say I have the following class ....

class Foo {

private $pin;
private $stat;

public function get_stat($pin) {
            $this->stat = shell_exec("blah read $pin");
            return $this->stat;
    }
}

So in the above function, I have the var $pin passed to the class method. This works just fine without having to use $this->pin ... however the below code seems more like it's the right way to do the same thing.....

class Foo {

private $pin = 0;
private $stat = 0;

public function get_stat($pin) {
            $this->pin = $pin;
            $this->stat = shell_exec("blah read $this->pin");
            return $this->stat;
    }
}

Also, I have set the $pin and $stat vars to = 0. I take it this can just be a default value or I can just define them as in the first example private $pin; and private $stat;.

So back to my question, what are the best practices on how to use members and $this in class methods? And what would be the advantages or disadvantages on each example?

Cole Tobin
  • 9,206
  • 15
  • 49
  • 74
nomaxpi
  • 51
  • 1
  • 4
  • 1
    I got a pretty good answer regarding this over on [codereview.stackexchange](http://codereview.stackexchange.com/). Check it out [here](http://codereview.stackexchange.com/a/23857/20878) – jnthnjns Apr 02 '13 at 00:27
  • Thanks for the link ASOK! Now it makes sense as to why you should only use $this ... to reference properties within the class. I couldn't quite understand the relationship with accessing them outside of the class. – nomaxpi Apr 02 '13 at 00:35
  • Please do NEVER call the shell with unescaped values! `shell_exec("blah read $pin");` is wide open to code injection. Always use the escaping functions, in this case for shell commands: escapeshellarg() – Sven Apr 02 '13 at 00:48
  • hi Sven, yes I am guilty of not "sanitizing" the input. This is just a quick and dirty example of something I was trying to quickly get working. The rest of the logic for escaping is coming after I fully understand the OOP side of what I'm trying to accomplish. Thanks again! – nomaxpi Apr 02 '13 at 00:53
  • @user2233942 "WouterJ" was referring to accessing properties from outside the class being comparative to `$this->` within the class for instance `$Foo = new Foo()` then `$Foo->get_stat("Variable Passed")` vs `$this->get_stat("Variable Passed")` within the class. – jnthnjns Apr 02 '13 at 00:55
  • In other words `$this` within the class is an instance of that class where `$Foo` in my example above is an instance of the same class but from outside of the class. If that makes more sense. – jnthnjns Apr 02 '13 at 00:59

3 Answers3

6

You have to use $this when using any class member. You must not use it when using local variables. You should avoid using class members if they are not necessary, like $this->pin in your second example.

Sven
  • 69,403
  • 10
  • 107
  • 109
  • thanks Sven. So in short, the only time I _should_ use $this is when I need to work with a class member. It's simply not needed otherwise? – nomaxpi Apr 02 '13 at 00:26
  • I wouldn't call it "not needed". `$this` must not be used when dealing with local vars. – Sven Apr 02 '13 at 00:27
  • @ColeJohnson: This entirely depends on coding style. As long as it is supported by the language, it is a valid approach. – Sven Apr 02 '13 at 00:42
  • valid and sane may be 2 different things though. While I agree with your point, php will technically eval any string you pass it. That doesn't make it a sane choice to run everything through eval just cause the language supports it. So outright abuse of class or local vars may technically work but it going nuts in the wrong direction can be the difference between him getting a job that pays well or living under a bridge. – Kai Qing Apr 02 '13 at 00:45
  • But lets say I have a variable, `$var` that contains `word`. Using this coding style, I would type `"$vars"` which would result in an undefined variable when I really meant `$var + "s"`. – Cole Tobin Apr 02 '13 at 00:45
  • If you criticize something it should not be the coding style but the lack of proper context escaping for a SHELL EXECUTE! – Sven Apr 02 '13 at 00:46
  • Certainly, but if he assigned every little thing as a class variable for no good reason, I would likely in the end conclude the same thing as if he ran everything through eval: plain and simply that this guy isn't the right guy to hire. But I am being too extreme. It would require absolute and outright stupidity in both cases for me to base my decision entirely on that. I was just being extreme. – Kai Qing Apr 02 '13 at 00:51
  • And I just realized you may have been responding to another comment... sorry. – Kai Qing Apr 02 '13 at 00:53
1

"best practice" depends on your needs. In your example, it looks like pin is static. You could just set that initially and not even pass it to the method.

private $pin = 'abc123';

public function get_stat() {
    $this->stat = shell_exec("blah read $this->pin");
    return $this->stat;
}

Setting class variables only makes sense if you need them accessible by the methods within the class. In your example both key and stat may potentially be used in many methods so it makes sense to define them as class variables and accessing them by using $this->key and $this->stat is sane and logical. It wouldn't make sense if something like stat was only used in a particular method or changed depending on a specific data set making stat an attribute of many objects instead of a common attribute of the class.

As Sven pointed out, using $this->pin when $pin is passed to the class is not sane. It would be more logical to assign it as a class variable and use $this->pin if the pin does not change and is common to the instance, in which case you would not need to pass anything to the method. Like, for example, an API request where the key is not likely to change. Passing $key to the method makes sense if $key can be anything, like results from a database, user input, or anything else where the source is not specifically known.

I don't know if this will help much, but here is an example of using getters and setters if you intend to change the values of the pin or stat based on anything passed generically, or abstractly. Getter and Setter?

Community
  • 1
  • 1
Kai Qing
  • 18,793
  • 5
  • 39
  • 57
  • This was actually helpful Kai. As you and Sven pointed out, $this->pin isn't needed since it's passed to the class to begin with. My disconnect is the relationship between the var inside and outside of the class (which there doesn't appear to be any). – nomaxpi Apr 02 '13 at 00:47
-1

If you want to stick with the good practices of OOP then you should really have setters and getters for your instance variables. For example, here is a revision of your code:

class Foo {

    // common practice to begin private variables and methods with an underscore
    private $_pin = 0;
    private $_stat = 0;

    // this is called a setter because we are setting a value
    // note the lack of a return
    public function setStat($stat) {
        // we use $this-> because we are referencing THIS instance of THIS class/object
        // and in doing so we refer to our private $_stat instance variable.
        $this->_stat = $stat;
    }

    // this is called a getter because we are getting a value
    // not how we are NOT setting values here.
    public function getStat() {
        return $this->_stat;
    }

}

So overall, you use $this when you refer to this instance of the class (also called an object). The benefit of having a class is that you can have multiple objects that a class defines. For instance:

class Person {

    public $name, $age, $gender;

    public function setName($name) {
        $this->name = $name;
    }
    public function setAge($age) {
        $this->age = $age;
    }
    public function setGender($gender) {
        $this->gender = $gender;
    }
    public function getName() {
        return $this->name;
    }
    public function getAge() {
        return $this->age;
    }
    public function getGender() {
        return $this->gender;
    }

}

// outside the class
$john = new Person();
$john->setName('John Doe');
$john->setAge(22);
$john->setGender('male');
var_dump($john);

The var_dump will show:

object(Person)#1 (3) { 
    ["name"]=> string(8) "John Doe" // $this->name
    ["age"]=> int(22)               // $this->age
    ["gender"]=> string(4) "male"   // $this->gender
}

Hope this helps!

djthoms
  • 3,026
  • 2
  • 31
  • 56
  • No, sorry, I disagree. Your getter/setter example for "getStat" is wrong, completely. Your code is an infinite loop with getStat() calling getStat(), and the idea is wrong. It is a good thing to have an object with methods that do something with passed parameters and return a result. Your other example is a completely different example because you illustrate getters/setters with a value storage without any functionality inside. – Sven Apr 02 '13 at 00:45
  • Well, the infinite loop in `getStat()` is still there, no matter what my opinion on the rest of your code is. – Sven Apr 02 '13 at 15:19
  • Sevn, I finally got what you were saying lol. Sometimes it's like talking to a brick wall -___-. It's been corrected. Thanks for pointing it out – djthoms Apr 02 '13 at 21:38