Subscribe to PHP Freaks RSS

10 Signs of Crappy PHP Software

Print
by John Kleijn on Jul 2, 2009 7:10:56 AM - 38,116 views

Note: This post is not a tutorial! If you have any questions related to underlying concepts, I can recommend my tutorials and the Application Design board.

Like it or not, as a professional developer, sooner or later you are going to do some customising (if you are lucky, "extending") of existing software.

If you are not familiar with the software, it is good advice to look into it before accepting the job. I had to learn that the hard way. But how do you recognize crappy applications without getting knee deep into the code? 10 pointers to identify crappy PHP software quickly...

1. The software tries to reinvent the object model, or "fix" language features.

See if you can find a class called "Object". If you find it, it's a pretty clear indication that the author is in the business of trying to reinvent the object model (most commonly because of his own lacking understanding of OO). It is safe to assume that his "fixes" won't stop there. Unplug your phone and hide under your desk.

2. The code includes user defined global variables

A search in the code for "global" or "$GLOBALS" may reveal something like this:

global $database, $my, $mainframe;

The infamous global variable. If you can tell me what those last two variables contain you are either intimate with the software I pulled it from, or you're psychic. Unlimited bonus points if you can say what code has had it's claws on it before execution flow got to this point. In short, steer well clear.

3. Scattered HTML and SQL

Search for some common SQL and HTML strings. You should be able to determine very quickly whether these are appropriate places for HTML or SQL. If you find HTML and SQL in the same file, "crappyness" is most definitely confirmed.

4. Classes do too much

Find the 3 largest class files by bit size. Take a look at the class name. Does it indicate a distinct responsibility? Look at the methods. Are the tasks they perform closely related? If not, run away screaming.

5. Lots of properties are public or lots of properties are static

If lots of properties are declared "public static", triple your quote. If I have to explain why, maybe there's an open position on the development team of the software for you.

6. Multiple levels of inheritance

More than 2 levels inheritance should be avoided like the plague. I stake my life on the resulting objects having too much unrelated behaviour (ok, maybe not my life, but if you find a proper use of more than 2 levels of inheritance, I'll buy you a beer).

7. The authors try to use Design Patterns

Whether or not the authors have a clue is easily determined by searching for some of the most common design patters. Search the code base and/or documentation for "factory", "decorator", "strategy" etc. If present, you can pretty quickly determine whether the authors know their stuff or are trying to look interesting. That is, if you know how the code should look. If not, refuse to take the project until you do.

8. The software messes with the error level

Well written applications run fine at any error level. Searching the files for /error_level\(.*\)/ should do the trick. In case of hits, try replacing the value with E_STRICT. That's little more than a formality though.

9. In the code base, there is a directory called "core"

This is usually used as an excuse to have the whole application dependent on whatever is in there. Despite the appeal of the term (it does make the contents sound pretty cool and important), defining a "core" is a sign of bad design.

10. The software uses it's own template language.

Be afraid. Very afraid. These guys are definitely in the business of reinventing the wheel. Ignore this warning and you will find yourself spending the better part of a day simulating a "for" loop.

Comments

rjp Jul 2, 2009 8:35:02 PM

Interesting article, and while I agree with you on a couple of points, you could have provided some alternative solutions to doing things better.

For example, instead of Globals, try using singletons.

Or what about using a 'core' directory. I use the Zend Framework in most of my applications and going by their standard, aside from the library files, the application code is all stored in the 'application' folder. Is this not the same thing?

Or number (5), "If I have to explain why, maybe there's an open position on the development team of the software for you."
How arrogant do you have to be to put in a quote like this?
The PHP community has always been about providing useful information for everyone to learn from and better themselves.

Its pretty easy to voice your opinion and criticize some of the coding habits out there, but in my opinion, if you can't offer solutions to some of those problems then you're just wasting everyone's time.

nico80 Jul 3, 2009 3:02:55 AM

I absolutely agree with rjp...
Also I don't really understand the point of #3. I can think of many legitimate cases when you can mix HTML and SQL queries.
I think the article would really benefit of some real-life examples, because as it is your points aren't very informative.

TriLLi Jul 3, 2009 3:16:42 AM

Hi, i don't agree with this,
5. Lots of properties are public or lots of properties are static
I agree with this only if "Hello World" script, but for every other project.

7. The authors try to use Design Patterns
10. The software uses it's own template language.

....

In 7 don't use already invented stuff and in 10 why don't you use already invented stuff

John Kleijn Jul 3, 2009 6:23:13 AM

"you could have provided some alternative solutions to doing things better."

I was expecting this critique. But this is not a tutorial about good design. It simply describes what to look for in applications that are new to you, to determine whether is wise to accept a project build on this project, or if you should look for alternative to propose. I'll admit that much of this advice is irrelevant if you are still coding procedural or don't really understand OO yet, maybe I should have mentioned that in the introduction.

"For example, instead of Globals, try using singletons."

This is bad advice. Singletons are not much better than global variables. This is exactly why I am not going to the "why" too much. All of this is based on theory proven in the field, as well as my personal experience. If you at any point question my advice don't take my word for it, do some research.

"aside from the library files, the application code is all stored in the 'application' folder. Is this not the same thing?"

No. What that point says, is that developers often define a core directory as an excuse to group unrelated code and justify use of that code throughout the application. The bigger the "core", the more code will be interwoven with the application. Your ZF "application" directory is not the "core" of your application. It contains different parts of your application, which are dependent on different parts of the library. Hardly the same.

"How arrogant do you have to be to put in a quote like this? "

Very arrogant. You should hear me on Friday evenings, I am known to proclaim mastership of the Universe. But seriously, this is meant to be provocative. I could have explained Encapsulation and prefering stateful over static in this post, but again this is not a tutorial. But, if I have your attention with this comment, chances are you haven't fully taken distance from this practice, and I got your attention just in time. You have two choices: do away with my comment thinking "you arrogant prick", or being a little more pragmatic about the situation and looking into why I am pointing this practice out so harshly.

"if you can't offer solutions to some of those problems then you're just wasting everyone's time."

I disagree. Again this is not a tutorial, and it's primary goal is to identify bad practice in existing applications. It's secondary goal is bring attention to these practices. They are still very widespread in the PHP language, more so than in any other language I believe. Again, if you disagree with anything I say here I urge you to look further into matter. If you then still disagree, I can only recommend you look a little further. If you still don't see it, your loss.

I can however offer you some explanation found in some of my tutorials on this site. Specifically my tutorials "Boring OO Principles" and "Design Patterns: Introduction". But for those too the advice "don't take my word for it, do your own research" is valid.

"Also I don't really understand the point of #3. I can think of many legitimate cases when you can mix HTML and SQL queries."

I am not going to debate this, as it has debated to death already, in it's general form as "separation of concerns". Maybe this is as good a time as any to start thinking about your coding practices?

"In 7 don't use already invented stuff and in 10 why don't you use already invented stuff"

You can hardly compare implementing Design Patterns with creating Yet Another PHP Templating Language. Putting aside the discussion of whether you should use a template language or straight PHP (I personally have a very strong preference for the latter), the fact is that successfully creating a flexible template language is not a small task, and aside from the fact that you will be submitted to learning a new language, there is a good chance the authors weren't as successful as you might have hoped. Hence, in general, try to steer clear. I can pretty much guarantee this advice will save you some headaches.

The bottom line is this: this post is not a tutorial. You can either resent me for not offering more explanation, or get your hands dirty and look into whatever it is you don't agree with or don't understand. Ask yourself which of these options would benefit you.

Daniel Jul 3, 2009 8:02:30 AM

"In 7 don't use already invented stuff and in 10 why don't you use already invented stuff" (TriLLi)

That's not what he said regarding #7. John said that many people use them just for buzz words, but otherwise implement them poorly.

Anyway, as I see it, this is written for people who already have some experience with application design. It's perfectly okay to set prerequisites for certain articles. I don't walk into a physics PhD course and start complaining that the professor doesn't explain the material in lay-man's terms either. If you for every single text need to explain things in a such detail that every possible reader can understand what's going on you would never have any books shorter than 1000 pages.

karczoh Jul 3, 2009 11:15:14 AM

"Singletons are not much better than global variables"? Can't really agree. Singletons are good solutions for some of the problems, they can obviously be overused - but that is other problem...

John Kleijn Jul 3, 2009 11:20:02 AM

Singletons are not much better than global variables. Take another look at #2. Now imagine instead of $mainframe, it says Mainframe::getInstance().

This will let you answer the first question, but the bonus points are still not yours. Singletons can easily create "tramp objects". Available for anyone to do with as they please.

nico80 Jul 3, 2009 11:59:35 AM

Bah... my point is that you can't generalise on these things. Singletons or global variables are bad? Well, they exist and they have applications. Maybe 90% of the times they're used in a bad way, that does not make them bad. You know, you can use a knife to kill someone or to cut a steak. The knife isn't bad by itself...

Provocative or not, what I get from this post is something like: oh, if you're using <xxx> you can't program and your code is crappy. That's very naive.

Daniel Jul 3, 2009 1:45:44 PM

I can make a non sequiteur as well: rape exists, therefore it it's (sometimes) good. Or: raping people is bad, therefore eating apples is bad.

You cannot conclude that something is good just because something exists. You also cannot conclude that X is A because Y is A when there is nothing connecting X with Y in respect to A. Such fallacies, when the conclusion does not follow from the premises, are called non sequiteur (literally meaning "it does not follow" in Latin).

The reason why globals are bad is because they break encapsulation so you cannot ensure data integrity very well and because it gives you tight coupling, which lowers the maintainability. Because a singleton is a global element it has those pitfalls as well.

Alejandro Zuleta Jul 3, 2009 11:04:57 PM

I disagree with #6, #9 and #7. Here is why:

#6. Inheritance is not the best solution for every problem, but some very well factored APIs have more than your "no more than 2 levels" rule of thumb. (I have enough examples to guarantee a long term supply of beers from you).

#9. Many extensible systems have a 'core' directory, containing the essential parts. If the only directory of the application is 'core', then something is wrong. But if there is a 'core' directory and many other directories, I don't see why this may raise a warning sing.

#7. When I have worked in projects with tons of crappy code, these guys never try to use any design pattern, because they don't even know design patterns exist, they just write the most awful code line after line without any planning. For this reason, if I find that someone tries to use a design pattern (even if is not absolutely correct) I consider this a good sign. At least this man tried.. really crappy programmer don't even try.

Finally, don't treat Singletons as if they were global variables, because they are not. It is true that they are global objects, but a good Singleton can have encapsulated properties and methods, protecting them totally from the outside, so they can't do any harm at all. This is why Singletons and global variables are very different beasts.

gerrywastaken Jul 4, 2009 3:52:11 AM

10 Signs of a Crappy Article

1. It's a list containing a nice round number such as 10 which suggests that the author had a few good ideas and then has had to make up bullshit for the rest of the list in order to reach their nice number.

2. The assumes they have though of every possible case of where and how something could be used and thus makes a bold statements that don't allow room for compromise.

3. It uses phrases like "If I have to explain why then <insert insult here>"

4. The author repeats themselves in a very short sentence when there is an obvious alternative which would be much better.
eg. "Lots of properties are public or lots of properties are static"
becomes..
"Lots of properties are public or static"

5. I was being facetious when I wrote "10 signs" in the title.

nico80 Jul 4, 2009 5:14:24 AM

> "I can make a non sequiteur as well: rape exists, therefore it it's (sometimes) good. Or: raping people is bad, therefore eating apples is bad."

Well, evidently you didn't get my knife example at all.
A knife is a TOOL, just like a singleton. You can use it in a good way or in a bad way.

To use your rape example... sex is something really good, but we shouldn't ban it because there are criminals out there who rape people.

That said, I will continue to use singletons in my code. And hopefully you won't have to put your hands on it, as it will be REALLY crappy code.

John Kleijn Jul 4, 2009 9:01:30 AM

It's amazing how many people defend their coding practices as if it were their firstborn.

I should probably leave it that, but I'll indulge.

"#6. Inheritance is not the best solution for every problem, but some very well factored APIs have more than your "no more than 2 levels" rule of thumb. (I have enough examples to guarantee a long term supply of beers from you)."

I am confident these can be better solved by favouring composition over inheritance. I'd be happy to discuss your examples. You make an interesting point though, you call it a "rule of thumb", and that is exactly what it is.

"#9. Many extensible systems have a 'core' directory, containing the essential parts.

Maybe so. This is why this is near the bottom of the list. These are "signs", a match on one of the lower items on the list doesn't necessarily mean you should run away screaming.

"If the only directory of the application is 'core', then something is wrong. But if there is a 'core' directory and many other directories, I don't see why this may raise a warning sing."

It warns that possibly the authors have created unnecessary dependencies on what's in this directory. Finding such a directory does not make that given fact, but in my experience there is definitely a good chance.

". really crappy programmer don't even try."

..but even crappier programmers do try, but without properly looking into the matter. I can provide you with an example of a CMS, released, promoted, with a company behind it, where at least two examples can be identified. Just because you don't share this experience, doesn't mean this doesn't happen.

"Finally, don't treat Singletons as if they were global variables, because they are not."

Nobody said Singletons and global variables are the same thing. In fact, I acknowledged that they aren't, and that Singletons have an advantage over global variables. But, that doesn't make using Singletons a good idea. You may have noticed though, Singletons are not this list. That's because while I discourage Singletons, and certainly don't want to promote them, finding a Singleton doesn't say enough about the design of the software to make it to this top 10. You won't hear me saying that Singletons are "bad". Usually a bad idea, with better alternatives available, yes.

"It is true that they are global objects, but a good Singleton can have encapsulated properties and methods, protecting them totally from the outside, so they can't do any harm at all. This is why Singletons and global variables are very different beasts."

This is a flawed argument. An object in a global variable has the same properties. The only real difference between an object in a global variable is that querying a singleton method, you know what to expect. Don't get me wrong, this is definitely a mayor advantage, but it doesn't change the global nature of a Singleton, nor does it provide any additional level of encapsulation.

"1. It's a list containing a nice round number such as 10 which suggests that the author had a few good ideas and then has had to make up bullshit for the rest of the list in order to reach their nice number."

Unfortunately this isn't true. I could have made this list a lot longer, but people would get more caught up in some of the lower numbers, as undoubtedly those accustomed to more "traditional" PHP practices would have disagreed.

"2. The assumes they have though of every possible case of where and how something could be used and thus makes a bold statements that don't allow room for compromise."

This is not what this post says. It merely identifies 10 warning signs. If you choose to ignore those warnings, your loss. But don't let your defence mechanism get the best of you, and try to assume, only long enough to make an objective, founded decision yourself, that I actually know what I'm talking about.

"3. It uses phrases like "If I have to explain why then <insert insult here>"

So you perceive this as an insult? Good, looks like I am talking to right person. Do yourself a favour and look into the concept of encapsulation. And of course the benefits of stateful vs static. I'd be happy to debate the issue with you afterwards. Unless you'd rather just be insulted.

"4. The author repeats themselves in a very short sentence when there is an obvious alternative which would be much better.
eg. "Lots of properties are public or lots of properties are static"

Yes that's much better. I suppose next you are going to fault my grammar (I'm sure you can)? This is just throwing stones.

"That said, I will continue to use singletons in my code. And hopefully you won't have to put your hands on it, as it will be REALLY crappy code."

Again with the defensiveness. I'd like to point out a flaw in your logic though. A knife can be used to cut someone, yes. But it's use isn't ill-advised by it's nature, it has peaceful uses. A Singleton however is by it's very nature globally accessible. I think it is a little short-sighted to dismiss global variables for this same reason, but justify using Singletons. I am certainly not saying that a Singleton or two warrants an application to be labeled as "crappy code". I am saying that as far as alternatives go, there is usually a "better" one. I use Singletons myself, sometimes, when I don't see an alternative. But always with the realisation that it is not the ideal solution, and it is most definitely not my first resort.

I would suggest though, that any discussion regarding OO concepts underlying this list, with the addition of the Singleton discussion, be directed to the Application Design discussion board. I'd be happy to explain in further detail.

gerrywastaken Jul 4, 2009 11:44:45 PM

I didn't attack any particular point you raised, just the article itself. For instance...

> So you perceive this as an insult? Good, looks like I am talking to right person. Do yourself a favour and look into the concept of encapsulation. And of course the benefits of stateful vs static. I'd be happy to debate the issue with you afterwards. Unless you'd rather just be insulted.

I was just point out a "sign" of a bad article, I didn't say I disagreed with that point. :P You didn't provide reasoning and instead just said "If I have to explain why then <insert insult here>". If you're trying to teach something then teach it. If you can justify your "signs" then do so. If you can't, then you should at least know that the cheap clichéd alternative that you used is very transparent.

John Kleijn Jul 5, 2009 12:37:42 AM

"I was just point out a "sign" of a bad article, I didn't say I disagreed with that point."

Then you have a very sensitive idea of what constitutes an "insult".

"If you're trying to teach something then teach it. If you can justify your "signs" then do so."

I don't know how many times I have to repeat that this is NOT a tutorial. I'll also repeat that if anyone requires additional explanation, I can offer my tutorials (this one does cover encapsulation), and the forums. If that's not sufficient for you, though luck. I am not changing this to a tutorial.

"If you can't, then you should at least know that the cheap clichéd alternative that you used is very transparent."

Transparent you say? Would you care to share what, according to you, shines through?

John Kleijn Jul 5, 2009 12:57:15 AM

I have added a note to the top of the post. Hopefully that will stop lazy people from nagging about not getting enough explanation. I would urge anyone to buy some good OO books though.

Radical Jul 5, 2009 3:13:44 PM

I can give you the 11th... as I said on my blog at http://gabriel.e-radical.ro/blog1.php/2009/07/05/10-signs-of-crappy-php-software

11. Rewriting in PHP what can be provided by an already written (PECL) extension.

John Kleijn Jul 5, 2009 3:43:15 PM

I can't say I would put that as high as 11. Not reinventing the wheel is definitely good advice, but reusing an external component especially when that component effectively creates a system requirement, is not a trade off everybody is willing to make. Even though I would definitely like to advocate the use of managed VPSes, I don't think this is something you can expect from the average public application. An author that creates userland code that mimics the behaviour of an extension would seem to be aware of the existence of the extension, and have made a concious choice to recreate it to avoid the system requirement. Can't say I approve of that, but I wouldn't say it is a sign of "crappyness" (unless of course we are talking about a standard extension).

phifty Jul 5, 2009 5:33:29 PM

If you're suggesting that the use of singletons is somehow bad practice, you are partially right. If you look at singleton objects as ordinary globals and nothing more, than shame on you. That's not what the singleton paradigm is for. It's meant to act as a chokepoint to a resource that for whatever reason doesnt perform well if there's more than one of it running around in your code. Thats it.

I think #5 is complete BS. There are many reasons why you would want a class to contain nothing but static method calls. For example, I wrote a class called FBML that simply acts as a wrapper for Facebook's FBML markup. It makes much more sense to call FMBM::getPersonInfo($person); than to instantiate and then call. Of course, this all depends on a properly configured autoload, but that's trivial.

That said, I think #3 and #10 are dead on. Mixing php with markup is just plain sloppy. CRUD is sloppy. You're just begging for weekend-long debug sessions if you do this.
As far as templating, If the code you're looking at is using a home-brewed system to handle its already shaky business logic, then the whole thing is bust.
If you're going to implement templating in your app, you should make it pluggable with something already out there and widely used (i.e. Smarty), or even better, use a format that is readable in multiple platforms (XSL). It irks me that people underuse the DOM tools that ship with PHP. They are easy to use, and render very fast and compliant xml.

John Kleijn Jul 5, 2009 6:28:53 PM

"If you're suggesting that the use of singletons is somehow bad practice, you are partially right. If you look at singleton objects as ordinary globals and nothing more, than shame on you."

If you want to know how I think about Singletons, all you have to do is read my preceding comments. You would have been able to determine the answer to second "question" is "no, I don't". Read before you post or don't post. Shame on you.

"That's not what the singleton paradigm is for. It's meant to act as a chokepoint to a resource that for whatever reason doesnt perform well if there's more than one of it running around in your code. Thats it."

Hardly.

1) You can hardly call a Singleton a paradigm (I bet you call a portable TV a Home Cinema Set).
2) This is your own (flawed) interpretation. The GoF describe the intent of Singleton differently: "ensure a class has only one instance, and provide a global point of access to it" (GoF:127). This has nothing to do with "performance" (very little common Design Patterns do, Flyweight perhaps being the most prominent exception). It says Singleton is meant to ensure one instance and to be globally available. That's it.

"I think #5 is complete BS. There are many reasons why you would want a class to contain nothing but static method calls. "

Read #5 again. You'll notice it says nothing about methods. Regardless, all static classes are a very poor design choice. You might as well go back to procedural, because essentially you are just wrapping your functions in a class. You lose all flexibility that comes with a stateful interface. I would like to hear your "many reasons". Come on, don't be shy.

Or is "It makes much more sense" the best argument you can provide?

Radical Jul 6, 2009 2:19:27 AM

About a possible #11:

My favorite example as you might have seen is GETTEXT... which is a standard extension...

I do suspect that the reason for doing this is "fear of unknown"...

"I do not know how to work with that extension... I'm going to build my own... it's as simple as CSV..."

John Kleijn Jul 6, 2009 6:23:58 AM

Gettext needs to be available on the system though and/or the extension enabled . Zend_Translate reimplements a gettext adapter with the following arguments: 1) having gettext available (--with-gettext[=/path/to/gettext] or php_gettext.dll) is not a system requirement and 2) gettext is not thread safe.

I don't think those arguments are completely without merit.

Alejandro Zuleta Jul 6, 2009 7:33:40 AM

First, thanks for taking the time to respond.

"I am confident these can be better solved by favouring composition over inheritance. I'd be happy to discuss your examples."

Yes.. I have read Steve McConnel or the GOF too. But while favoring composition over inheritance may be a good practice, it is definitely not the best practice in every case. In fact, in many situations the sign of a good design is that you follow the most natural path.

A natural example would be the inheritance chain Being > Animal > Mammal > Primate > Human. Before you say this is not a real programming example, I will point you to one that is: The HtmlTextWriter class in the .net framework, which is part of a chain of 5 parents. Yes, I know it is not php, but I just want to point out that your rule isn't very universal, and that it may tell you absolutely nothing about the crappiness of someone's code, even if it is php. I concede, however, that your rule of thumb makes sense if the system is not very complex. But in a complex system it does not really apply.

"I can provide you with an example of a CMS, released, promoted, with a company behind it, where at least two examples can be identified"

Ok. Have you seen this in any other place? I think this is not as frequent as you try to make it seem.

"This is a flawed argument. An object in a global variable has the same properties. "

Totally wrong. Why are global variables considered bad? There's a clear reason: side effects. An object in a global variable, however, is able to protect itself from the outside, making side effects inexistent, and the problem is gone.

A quick example:

class Database{
private $connection;
private __construct(){ $this->connection = mysql_connect(blah);}
private static $instance;
static function getInstance(){
if(self::$instance)return self::$instance;
return self::$instance = new Database();
}
public query($sql){
return mysql_query($sql, $this->connection);
}

}

Usage: $db = Database::getInstance();
$db->query($sql);

You can't do any harm with this Singleton, and it is of course just an example (in which you see how I actually used encapsulation, to prove you wrong :) ). In a big application, it is quite possible that there are some natural global objects.. To be global is not bad per se, you shouldn't try to avoid global objects as far as it is natural that they exist, and they don't have side effects. To be fair, however, most objects are not naturally global.

On the positive side,I think your other points are right. Classes that do too much, sql and html in the same code (or also arbitrarily mixing javascript with php ), or too many public properties are a sign that something is wrong (or that you are using php 4 :)

"It's amazing how many people defend their coding practices as if it were their firstborn."

With you heading the list.

John Kleijn Jul 6, 2009 8:54:18 AM

"In fact, in many situations the sign of a good design is that you follow the most natural path. "

What comes "natural" is heavily influenced by your own conventions and experience. That in no way makes it a better idea.

"The HtmlTextWriter class in the .net framework, which is part of a chain of 5 parents."

I see. So because it is in .NET, it's a good idea? Regardless, looking at the class hierarchy, I think 5 levels is an exaggeration. The top two are language (platform) features, I am not willing to count those as there is no equivalent in PHP. I'm am sure that in Java, or other languages with classes as language or platform features a rule of thumb of 2 levels is too low a number, because you already start out with at least one. But what I see is the HtmlTextWriter has a supertype and some children. Corrected for the difference in platforms, that's 2 levels of inheritance, not 5.

"I concede, however, that your rule of thumb makes sense if the system is not very complex. But in a complex system it does not really apply."

Especially in a complex system you should avoid multiple levels of inheritance. Composition is way better at handling complexity without losing flexibility than inheritance.

"I think this is not as frequent as you try to make it seem."

Maybe it is, maybe it isn't. It's irrelevant as this list is not ordered by frequency of occurrence, but by impact.

"Totally wrong. Why are global variables considered bad? There's a clear reason: side effects. An object in a global variable, however, is able to protect itself from the outside, making side effects inexistent, and the problem is gone."

Since you mention "side effects" without sharing the nature of those side effects, I'll lend you a hand. At the top of the list is coupling to the global space. Object encapsulation does not solve this.The object maybe able to protect itself from the outside, but who is protecting the outside from the object?

Aside from that, you where argumenting the difference between internal encapsulation of globals and Singletons. I say there is none, assuming the global contains an object. Your new argumentation seems however to have switched from defending Singletons to defending globally available objects. I am pleased that you at least see that there is no difference.

"There are some objects ,such as a database connection, that are unique in their nature. If you create a Database singleton, make the connection in the private constructor and put a public method on the object to make a query, with the sql passed to the object, how are the chances that this object will be wrongly used? I have the answer: none. The connection resource is encapsulated within this (naturally) unique object and a singleton pattern makes total sense."

A database connection is probably the poorest example you could have possibly chosen. What if I need two simultaneous connections? Goes to show, the assumption that only a single instance is required is not always correct. As an added "bonus", when you discover this after littering your app with static calls, changing it requires a considerable amount of changes to your system.

I'm afraid you fail to see the full scope of coupling. Globally available objects are not ill-advised because they may be "used wrong". Encapsulation is not just about controlling access to an object (structure). It is about isolating different "components" of an application. In this context a component is a distinct part (big or small) of your application, whether it be a subsytem, package, class, object, or even just a method.

I realise that in a language without package visibly enforcing the higher levels of encapsulation requires a little more discipline, but that doesn't mean you should just couple to the global space without carefully evaluating the alternatives.

astromac Jul 6, 2009 5:47:27 PM

Are there cases where user-generated $GLOBALS variables are okay? I am a bit concerened as I have just written a procedural PHP script (I don't yet know OOP) that reads the contents of a csv text file, sanitizes the contents and stores it into a global array for reference throughout the script. Is this bad and if so, what can be done to improve it?

John Kleijn Jul 6, 2009 5:59:48 PM

I can really only give one advice: learn OOP.

Alejandro Zuleta Jul 6, 2009 7:38:15 PM

"Composition is way better at handling complexity without losing flexibility than inheritance."

As I said before, this depends on what you are doing. If you blindly give the same solution regardless of the problem at hand, then maybe you are the crappy programmer of your team.

"So because it is in .NET, it's a good idea?"

Believe it or not Microsoft has one of the best programming teams I have seen. .net framework was architected by people who really know their stuff very well. And I'm not saying this because I blindly believe in authority, I actually know who this guys are.

"Especially in a complex system you should avoid multiple levels of inheritance. "

This is not always true. In a complex system is way more probable to find the need for many levels of inheritance than in a simpler one. If you apply inheritance well, oods are good that your system will be better architected. Sometimes composition is not an option (if you need polymorphic behavior, for example).

"who is protecting the outside from the object?"

If your code makes a bad use of any object, then your code is crap, and its not the pattern's fault. This will happen with any object, global or not.

"Since you mention "side effects" without sharing the nature of those side effects"

I thought this should be obvious for a person like you. The problem with being global is that in any place, at any time, the state of the object can be changed. A global variable can be deleted at any instant, and whoever tries to use it afterwards will find problems.

"Your new argumentation seems however to have switched from defending Singletons to defending globally available objects."

I don't defend global objects, you were the one who mentioned this and I'm simply using your own words. Global objects, however, is a feature of singletons.

"A database connection is probably the poorest example you could have possibly chosen."

You are attacking an example, not my point. If you have something to say about my actual point and not about my example (which was put together in 1 minute), then I'm willing to hear it. If you fail to recognize the point, then shame on you.

"It is about isolating different "components" of an application. "

This is called loose coupling, not encapsulation.

John Kleijn Jul 6, 2009 9:26:01 PM

"If you blindly give the same solution regardless of the problem at hand, then maybe you are the crappy programmer of your team."

I can't disagree with that, but this in no way refutes that object composition usually offers a more appropriate solution in the face of complexity than inheritance. If you have any argument against this, I have yet to hear it.

"Believe it or not Microsoft has one of the best programming teams I have seen. .net framework was architected by people who really know their stuff very well. And I'm not saying this because I blindly believe in authority, I actually know who this guys are."

Who cares if you "know who this guys are", that adds zero value to your arguments, or this discussion in general. I don't consider the .NET framework as an authoritative work in the field of OOD, but even if I did, I refuted your example. As far as I can tell, .NET does not advocate the use of many levels of inheritance. But if it does, I would certainly be disappointed in the developers.

"In a complex system is way more probable to find the need for many levels of inheritance than in a simpler one."

Maybe in a more complex and/or larger system you will be tempted to many levels of inheritance more often (simply because you have to make more design decisions), but I certainly don't see how complexity, nor size, increases the "need" for multiple levels of inheritance. And I still don't see any argument.

"If you apply inheritance well, oods are good that your system will be better architected."

I couldn't agree more. What we apparently don't agree on is the interpretation of "well" in this context.

"Sometimes composition is not an option (if you need polymorphic behavior, for example)."

You only need one level of inheritance to exploit polymorphism.

"I don't defend global objects, you were the one who mentioned this and I'm simply using your own words. Global objects, however, is a feature of singletons."

You didn't orginally, I said "you where argumenting the difference between internal encapsulation of globals and Singletons". This statement was based on this quote:

"It is true that they are global objects, but a good Singleton can have encapsulated properties and methods, protecting them totally from the outside, so they can't do any harm at all. This is why Singletons and global variables are very different beasts."

Where you essentially say that a Singleton is better internally encapsulated than a global object, which I dispute. I responded by making clear that there is no difference in encapsulation: "This is a flawed argument. An object in a global variable has the same properties ".

Then you responded with this:

"An object in a global variable, however, is able to protect itself from the outside, making side effects inexistent, and the problem is gone."

So to recap, you say " a Singleton is better than a global object, because it is able to protect itself from the outside", but "an object in a global variable, is able to protect itself from the outside". With that last statement you are defending global objects.

Anyway, sidetracking, moving on.

"You are attacking an example, not my point. If you have something to say about my actual point and not about my example (which was put togheter in 1 minute), then I'm willing to hear it. If you fail to recognize the point, then shame on you."

I refuted your "point" in the paragraph directly below that, by pointing out that a Singleton causes coupling to the global space, no matter how you write it or when you use it. On a side note, I prefer to decide myself whether I should feel ashamed, saying "shame on you" addressed directly at me makes me feel annoyed rather than ashamed.

"This is called loose coupling, not encapsulation."

No, coupling is the degree in which change in one component influences another. Encapsulation can certainly decrease this factor, resulting in loose coupling (and usually higher cohesion), but encapsulation refers to breaking down your application and isolating the parts, big or small (Seperation of Concerns). This is used for Information Hiding, protecting parts of a program from modification if other parts change, in effect, protecting against coupling. Encapsulation is a means for Information Hiding, (loose) coupling is an effect.

Alejandro Zuleta Jul 6, 2009 10:23:32 PM

"but this in no way refutes that object composition usually offers a more appropriate solution in the face of complexity than inheritance"

When did I say that inheritance is better than composition? I just said that it depends on your problem, and that certain problems need more than 2 levels of inheritance, that's all. Thats why, returning to your article, your rule of thumb doesn't add much unless you stop and consider why this design decision was made. An thus, it is not a valid sign of crappy code.

"As far as I can tell, .NET does not advocate the use of many levels of inheritance."

Of course not, but when it is the right choice, they go on and do it, don't just try to avoid it because it breaks some so-called "best practice" . A great system designer knows when to "break" the rules as well as when to follow them.

"And I still don't see any argument. "

I still don't see any counter-argument.

"What we apparently don't agree on is the interpretation of "well" in this context."

"Well" just means the best choice, whether is using inheritance or not. Do you disagree with that? Again, I'm not favoring inheritance over composition, where did I say that?

"...That a Singleton causes coupling to the global space"

Maybe you are a purist, in which case this discussion makes no sense. Purists think they rules of thumb are something almost religious. I'm a pragmatic programmer, who applies best practices most of the time but don't follow them blindly and, if necessary, will not apply them. I'm not saying your style is wrong or mine is good, I'm simply saying we have different styles and probably we will not convice each other.

"encapsulation refers to breaking down your application and isolating the parts, big or small"

This is certainly true in a system that offers _real_ encapsulation like java or C#, where you can hide objects to the outside by making them internal. But in php you haven't a real way of encapsulating components, so, no matter what you do, the global space will be always cluttered (it is already is with tons of isolated functions). So, if your argument is that Singletons are better avoided because they clutter the global space, then you have chosen the wrong language. And don't get me wrong, I am not saying that we should make everything global and convert every class in a singleton, I am saying that to try to force tight rules in a language like php is almost contradicting your first point. In php, you will never be able to isolate components (which, again, doesn't mean that you should make badly designed code).

John Kleijn Jul 7, 2009 8:40:04 AM

"I just said that it depends on your problem, and that certain problems need more than 2 levels of inheritance, that's all. Thats why, returning to your article, your rule of thumb doesn't add much unless you stop and consider why this design decision was made. An thus, it is not a valid sign of crappy code."

A rule of thumb adds value if it holds true in MOST cases, that's what makes it a rule of thumb and not a fact. I think it holds true in most cases, but given your inability to provide me with a proper exception to the rule, maybe I wasn't strict enough and it is more realistic to say it holds true in ALMOST ALL cases. Just maybe. You see? I'm perfectly able to adjust my views in a discussion ;)

"A great system designer knows when to "break" the rules as well as when to follow them."

Agreed. Especially when the "rule" is a heuristic and not a design principle. But a great designer also knows the effects of his design choices and carefully evaluates them. This usually does lead to following established design principles, and more often than not are in compliance with well know heuristics, which is what keeps them alive. Principles and heuristics without merit slowly die out, because believe it or not these "purists" do have a mind of their own. This begs the question, why hasn't the artefact Singleton died out? The answer is simple: it is convenient. People are continuously looking for a quick fix, perhaps because of laziness, but probably more often because of outside pressure for quick resolution (deadlines). That this is a poor design choice with considerable effects for in the long term likely to cause more delays, is conveniently overlooked.

Like I have said previously in this discussion, I make myself guilty of this as well, especially for the reason above, quick resolution. But that doesn't mean I am not aware of the fact that I am making a design decision which has negative repercussions for future evolution of the software, and because of it try it avoid it whenever possible.

""Well" just means the best choice, whether is using inheritance or not. "

"well" means "best choice". How insightful. Thanks for that gem.

"Maybe you are a purist, in which case this discussion makes no sense. Purists think they rules of thumb are something almost religious. I'm a pragmatic programmer, who applies best practices most of the time but don't follow them blindly and, if necessary, will not apply them. I'm not saying your style is wrong or mine is good, I'm simply saying we have different styles and probably we will not convice each other."

I don't consider myself a purist, nor do want to label a Singleton or two or applying 3 levels of inheritance as "bad", I prefer the nuance "poor", which indicates that it could be adequate but probably could be improved.

I think it is kind of convenient to label me a purist. Prefixing it with "maybe" doesn't help, uttering just the possibility is using it as argument.

It's certainly a proven way to dismiss anything somebody says, without having to evaluate their arguments. Applause. Labelling me a purist because I do not share your views is certainly a very pragmatic approach of situation, more applause.

"This is certainly true in a system that offers _real_ encapsulation like java or C#, where you can hide objects to the outside by making them internal. But in php you haven't a real way of encapsulating components, so, no matter what you do, the global space will be always cluttered (it is already is with tons of isolated functions). So, if your argument is that Singletons are better avoided because they clutter the global space, then you have chosen the wrong language."

I see. So because php has no package private, encapsulation only applies to a class level and you shouldn't care about coupling to the global space? Encapsulation is a little more than "declaring stuff private". It's about isolation, this also involves hiding objects structures behind stateful interfaces. If two object structures have a stateful interface, but no reference to each other, they are isolated from each other, and there is encapsulation.

John Kleijn Jul 7, 2009 9:16:53 AM

Just for laughs, here's a nice analogy for some of your arguments: driving doesn't always work

Alejandro Zuleta Jul 7, 2009 10:53:52 AM

Hmm well, as I think we have both said enough I will not continue this thread. It was an interesting discussion though. However, I won't go without saying that you have failed to convince me that #6, #7 and #9 are real signs of crappy code. We could go on forever but this is not funny anymore. Thank you for your time.

Alejandro Zuleta Jul 7, 2009 10:59:31 AM

And, by the way, as a huge example, I invite you to see a php framework I wrote from scratch.. you can find a pre-release code here: http://code.google.com/p/orange-php/ . It is not complete yet, and some refactoring is needed here and there, as well as documentation that I will write when it is time. And yes, it uses some singletons, in a specific component it uses a deep inheritance chain, and it has a directory called "core" :) Maybe this directory will vanish in the future, however. And, well, if you take a look at it, it would be good if you say it is crap, provided you let me know why. I'm willing to get it better.

John Kleijn Jul 8, 2009 6:13:20 AM

When I have a little time, I will take a look at it. Maybe this weekend.

paralyzah Aug 8, 2009 8:56:08 PM

I cannot believe that so many people don't see the point of this article!? I found it very useful. I guess the point is as follows: Rules can be bent or even broken as long as you know that you do so, but in most cases, the programmer didn't even know the rule was there. That's why these are SIGNS of poor code. I have intentionally broken #3 about having SQL and HTML in the same file, but that has absolutely nothing to do with me mixing the different layers of logic, which this in most cases would be warning signs of. We humans usually tries to justify things we do, but to look for examples to justify our own code (flaws) is not very constructive. If you did anything on this list unintentionally, chances are, it's bad code.

playaz Aug 19, 2009 10:47:39 AM

To be fair that fact that so many people have criticised the article shows people are certainly reading the whole article!

I don't agree with everything John has mentioned but would definately agree with the bulk of it, especially the first few..

At the end of the day John is giving his views in the article which is clearly not agreed by everyone but at least get's people thinking so Kudos to John for that!

Thanks again John!

Samuel Marchant Oct 11, 2009 3:58:56 AM

9 and 10 i don't fuly agree with but thats reasonably negligable to argue.

jotrys Jul 4, 2010 5:44:50 PM

At first I too thought this was a bit on the light side even though it is a useful list. But John, your explanations and diligence more than make up my initial doubts.
5 stars out of 5!

Add Comment

Login or register to post a comment.