13

Consider the following code, in which the setter is deliberately broken due to a mundane programming error that I have made for real a few times in the past:

<?php

    class TestClass {

        private $testField;

        function setField($newVal) {
            $testField = $newVal;
            // deliberately broken; should be `$this->testField = $newVal`
        }

        function getField() {
            return $this->testField;
        }

    }

    $testInstance = new TestClass();
    $testInstance->setField("Hello world!");

    // Actually prints nothing; getField() returns null
    echo $testInstance->getField(); 

?>

The fact that I declared $testField at the top of the class helps conceal that programming error from me. If I hadn't declared the field, then I would get something similar to the following warning printed to my error log upon calling this script, which would potentially be valuable to helping my debugging - especially if I were to make an error like this in a large and complicated real-world application:

PHP Notice: Undefined property: TestClass::$testField in /var/www/test.php on line 13

With the declaration, there is no warning.

Perhaps I'm missing something, but I'm aware of only two reasons to declare class fields in PHP: firstly, that the declarations act as documentation, and secondly, that without declarations one can't use the private and protected access modifiers, which are arguably useful. Since the latter argument doesn't apply to public fields - assigning to an undeclared field of an object makes it public - it seems to me that I ought to at least comment out all my public field declarations. The comments will provide the exact same documentation value, but I will benefit from warnings if I try to read an uninitialized field.

On further thought, though, it doesn't seem to make sense to stop there. Since in my experience trying to read an uninitialized field is a much more common cause of error than trying to inappropriately read or modify a private or protected field (I've done the former several times already in my short programming career, but never the latter), it looks to me like commenting out all field declarations - not just public ones - would be best practice.

What makes me hesitate is that I've never seen anybody else do it in their code. Why not? Is there a benefit to declaring class fields that I'm not aware of? Or can I modify PHP's configuration in some way to change the behavior of field declarations so that I can use real field declarations and still benefit from "Undefined property" warnings? Or is there anything else at all that I've missed in my analysis?

Mark Amery
  • 1,250
  • 1
  • 14
  • 27
  • 1
    Arguably useful? The benefits you stated are extremely important. I would probably flatly refuse to work on code that had no explicit property declarations. – Michael Feb 08 '13 at 19:01
  • 2
    Really, the way to protect yourself from these errors is with careful error checking and better, via unit testing. – Michael Feb 08 '13 at 19:02
  • Whatever you do, for getField to work you must first initialize the field. Might as well do that in the property declaration! –  Feb 08 '13 at 19:04
  • 1
    there is built in php error reporting (a notice level) that will tell you if you use a variable without first declaring it. – CrayonViolent Feb 08 '13 at 19:05
  • @MichaelBerkowski I wrote 'arguably' because I've never accidentally tried to access a private or protected field on an object in any language that had access modifiers. I would think this is quite rare? I view private/protected modifiers as being primarily useful as documentation. If avoiding accidental access of private or protected properties is a concern, perhaps I should combine commenting out my declarations with adopting Python-style leading underscores on private field names to reduce the chance of mishap? – Mark Amery Feb 08 '13 at 19:06
  • @MichaelBerkowski As for unit tests, I agree that these are the ideal.. However, I work in a frantically-paced startup and for us, finding the time to write proper unit tests is a fantasy. As such, finding programming practices that minimize the chance of errors being created in the first place without adding friction to our development process at the same time is valuable to us. So far as I can tell, commenting out declarations will help with this - but if I am wrong, please explain why! – Mark Amery Feb 08 '13 at 19:08
  • @CrayonViolent You mean a level at which assigning to an undeclared field triggers a warning? That sounds like an improvement on using the default notice level along with declarations. But is it an improvement on using the default notice level with commented-out declarations? If so, why? – Mark Amery Feb 08 '13 at 19:12
  • 3
    @MarkAmery I would think that a frantically-paced startup is among the most important places to adopt strict unit testing - since you're always changing code, you're likely always breaking it. That's really the exact purpose of strict testing and flat-out TDD. Yeah, I guess using underscores may help you remember you're accessing private stuff, but for me the idea of adopting specific coding standards to protect me from making errors I shouldn't be making is unsettling. Like doing TRUE === $variable to prevent yourself from accidentally assigning instead of comparing. – Michael Feb 08 '13 at 19:14
  • 1
    @MarkAmery Note also that the visibility keywords are parsed by automated documentation tools, so they give more "documentation value" than just a comment you would put in manually. – Michael Feb 08 '13 at 19:16
  • @MarkAmery - Unit testing is very important. Even for a start up. The cost of discovering a bug later on (or even in production) is a huge amount compared to finding it earlier. As to OOP and access modifiers it is more that for documentation - it is saying something about the contract between the object and the thing that is using the object. I would also say that and objects fields should be either private or protected. Then provide methods to get/set those fields. – Ed Heal Feb 08 '13 at 19:19
  • @FrancisAvila Sure - unless I don't know what a meaningful value will be until during or after instantiation, which is the case for most fields that aren't class constants. Just assigning a default value (probably of 0, or null, or "") at declaration time provokes exactly the same error-concealing behaviour as declaring without assignment. – Mark Amery Feb 08 '13 at 19:19
  • If you used TDD, this wouldn't be a problem. Even if you didn't forget to properly reference the variable, then entire method may be wrong, so you can't really say one way is more useful because it stops silly errors, because you're still in danger of being exposed to much bigger errors. –  Feb 08 '13 at 19:20
  • @MichaelBerkowski - I disagree with the TRUE == .. example. If you get into the habit, and as a human we all do make mistakes from time to time, the compiler is able to pick it up. Also as a human have you caught yourself thinking one thing when in fact you wrote something slightly different. – Ed Heal Feb 08 '13 at 19:23
  • @EdHeal I don't mean to be obtuse, but "saying something about the contract" === "documenting the contract", right? My exact point was that using commented out declarations preserves that benefit - unless you need the declarations to be parsable by tools, which as MichaelBerkowski points out is a scenario I hadn't considered. – Mark Amery Feb 08 '13 at 19:24
  • @PO'Conbhui Sure, there are an infinite variety of possible programming errors that you won't be protected from by PHP reporting on attempts to read an uninitialized variable. The same is true of any other kind of debugging aid. I don't see how your argument doesn't apply equally to, for example, syntax checking tools in IDEs. The purpose of such tools isn't to protect you from all errors; it's to protect you from a specific subset of possible kinds of errors. But that doesn't mean they're worthless. – Mark Amery Feb 08 '13 at 19:28
  • Mark - so having a contract with bits commented out is a different contract. Take for example your employment contract. Imagine that commented out the bit about holidays and your employer said - well that is a given. Where would you stand after booking that fortnight away? – Ed Heal Feb 08 '13 at 19:30
  • @MarkAmery, you should not even allow an object to be in an unusable or inconsistent state. If you must allow it, then your getField() should throw an exception if the field is uninitialized, not a mere notice. –  Feb 08 '13 at 19:37
  • @FrancisAvila Fair enough, but if you're using declarations and null is a valid value for the field then you can't even check if the field is uninitialized from the getter by any mechanism that I know of; without the declaration, you could throw an exception if property_exists returns false. – Mark Amery Feb 08 '13 at 19:43
  • @MarkAmery I guess leaving it out as an error check is better than no other error checking, but if it's a publicly accessible variable, it really should be declared for clarity, and you really should be using some sort of unit testing which would catch this sort of thing. So, answering your question, leaving them out is bad style, and leaving unit testing out will be more harmful in PHP than any particular language construct. – P O'Conbhui Feb 12 '13 at 01:22
  • 1
    Looks like the people on this thread are more into FUD and mantra and less into answering your question! – Dmitry Minkovsky Sep 22 '13 at 18:33
  • You could have gotten help from your IDE. PhpStorm warns you by changing the text color to gray if you set a local variable without using it. – Gogowitsch Feb 08 '17 at 15:24
  • One suggestion to reduce the tendency of assuming a declared variable name from what should be a field name (because it was seen previously without '$this->') is to clearly delineate the section of code containing field declarations with comments, possibly even to the extent of using comments to draw a big ASCII box around them. – Darren Ringer Oct 16 '20 at 18:01

2 Answers2

16

You should always declare your class properties ahead of time. While PHP is a dynamic language and will happily go right along with you creating your properties at runtime, there are several downsides to this path.

  • The compiler can optimize declared properties. When you dynamically declare a property this is a performance hit as the compiler has to create a dynamic hash table to store your dynamic properties.
  • I'm not 100% on this one but I believe that bytecode optimizers like APC won't be as useful since they won't have a full picture of your class. (And optimizers like APC are a must)
  • Makes your code 1000x harder to read. People will hate you.
  • Makes it 1000x more likely that you will make a mistake. (Was it getId or getID? Wait, you used both. Fail.)
  • No IDE autocompleting or type hinting.
  • Documentation generators won't see your property.

The problem you described is actually a minor one when compared to the problem of not declaring your properties in your class definition. Here's a good solution.

Get used to declaring defaults for your properties. 

private $testField = null;
private $testField = '';
private $testField = 0;
private $testField = []; //array()
private $testField = false;

Except for properties that will be storing objects, this will cover the majority of base types that you will be storing. The properties that will be storing objects can be set in your constructor.

A good rule for class design is that after your object has been created and your constructor has run you shouldn't have any properties out there "undefined".

Jarrod Nettles
  • 6,145
  • In response to your 5 downsides: 1 (Performance): Do you have a source for this? 2 (Readability): Not sure I understand; the proposal was for commenting out the declarations, not just removing them, so what readability is lost? Slightly less friendly syntax highlighting and potential difficulty distinguishing from neighboring comments, I guess? 3: This one I don't understand at all; can you clarify? 4 (IDEs): Fair enough - I don't have experience of coding PHP with an IDE and didn't know about Eclipse's type hinting. 5 (doc generators): Fair enough - never used one of these. – Mark Amery Feb 08 '13 at 23:01
  • I didn't see your line about commenting them out. Regardless, the above points still apply - how do you know which ones are active or legitimately commented out?? – Jarrod Nettles Feb 08 '13 at 23:08
  • In response to your proposed solution: this is precisely what I want to avoid - the assignment of defaults even when they are meaningless and the default values should never be used. The problem with these defaults (and with declaration without assignment, which just assigns null) is that if, due to a programming mistake, I don't assign a value to something in the constructor when I ought to, then rather than PHP giving a warning when I try to use that value later - helping me spot my error - everything will seem to work fine even though the class is broken. – Mark Amery Feb 08 '13 at 23:11
  • 2
    @MarkAmery Your programming mistake, though, essentially amounts to a typo. We've all had them - but you're trying to detonate a bomb to kill a fly here. – Jarrod Nettles Feb 08 '13 at 23:13
  • You may be right. I spent a couple of hours tracking down a bug today that turned out to be caused entirely by such a typo, and was frustrated afterwards by the sudden realization that if I'd only not used field declarations, I would've got a notice and known immediately where my error was (which I'd always thought would happen in these circumstances anyway; I didn't realize declarations initialized fields to null). The immediate accessibility of that experience is probably skewing my judgement of how likely it is to recur, or how worthwhile it is to defend against. – Mark Amery Feb 08 '13 at 23:24
  • @MarkAmery I agree with Jarrod here, your mistake is basically a typo. I've spent many hours debugging code because I've misplaced a semicolon or connected to the wrong database, etc. That doesn't mean I stop using Java or Hibernate just because I've wasted my time by typing in the wrong thing before – Deco Feb 09 '13 at 02:36
  • @Deco Not comparable because abandoning an entire language is a much larger change than just commenting out declarations in PHP. Jarrod's answer is useful because - while I query or challenge parts of it - it points out some actual downsides, none of which I knew about when I posted the question. A better comparison - were it not for the tool-related downsides - would be something like using "use strict" in Javascript. Yes, you can always say it's pointless because you'd rather just not make the errors it protects you from by being a flawless human, but I'll take the help if it's free. – Mark Amery Feb 09 '13 at 10:56
2

I worked on some code that used dynamically created object properties. I thought that using dynamically created object properties was pretty cool (true, in my opinion). However, my program took 7 seconds to run. I removed the dynamic object properties and replaced them object properties declared as part of each class (public in this case). CPU time went from over 7 seconds to 0.177 seconds. That's pretty substantial.

It is possible that I was doing something wrong in the way I was using dynamic object properties. It is also possible that my configuration is broken in some way. Of course, I should say that I have a very plain vanilla PHP configuration on my machine.

user328304
  • 21
  • 1