Signs of Crappy PHP Software | Hacker News Reader

Signs of Crappy PHP Software

phpfreaks.com

16 pointslox40 comments

I seriously don't understand why people hate any use of globals? I've written a 20k line MRP system that spans over 100 different PHP files. I have a global "$look_ahead = 90; // days". Every once in a while, someone from production planning comes up to me and asks if they can look ahead 45 - 120 days from now on. I simply change that variable and all is good immediately. Every single PHP file uses the global $look_ahead variable.

Sure, I could make a function look_ahead. Or I could pass the look_ahead value everywhere. Or I could have a DB setting look_ahead. But why would I make it any more complicated than I have to? Globals work very well when used as application preferences that rarely change but could.

Also, $company_address is a global. We're moving to a new building. That's a single variable I need to change. It'll change across every single report/printout/auto-generated PDF. And $db is always a global in my code. I've been coding for 10 years and not once have I had any issues because of that.

edit: Why the downvotes? I thought the proper etiquette here was to post a reply. It's not like I said something offensive.

Globals can make sense. In C it isn't unusual at all to see a variable that is global in file scope only, which is similar in effect to how class variables are treated within the class definition in a variety of OOP languages.

The problem is PHP's scoping rules don't lend themselves well to this kind of thing. If you were to use a PHP library, for instance, that used the same practices you are using, say a parser, you might find that depending on the include order $look_ahead now means something different.

This kind of "all I did is include a file and everything broke" error is really annoying to find. Constants are useful for this, because at least it will flag an error/warning. Now that PHP has (is getting? It's been a while) namespaces, you can limit its scope and make it less likely to collide.

For the case of databases, it is fairly common to use PHP's builtin functionality to reuse the database connection without having to rely on a global handle. Again, if someone shares your practices the variable could end up getting stepped on. Throw in autoloading and maybe it only occurs when some confluence of events leads to some specific file being included that usually isn't.

So there's a reason for concerns about globals. Putting everything in a singleton with static variables is not really different, but in my mind mostly just namespaces it so your globals don't get in trouble with someone else's globals. I've found in practice that I've only rarely needed global variables or singleton classes housing global variables. This probably largely depends on your style of coding.

I haven't written in PHP for years now so if I've gotten any details wrong please forgive me. Hopefully though you can see some ways that frequent use of globals can lead to hard to find bugs. A lot of programming techniques (OOP, closures, for example) are designed to help pass around state in ways that don't lead to these kinds of problems.

Food for thought:

1. As another responder mentioned, some of your examples sound more like named constants than global variables.

2. Not using global variables is just an example of a more general rule: variables should generally have as small a scope as possible. The wider the scope, the harder it is for someone reading the code to understand how the variable is used, and the riskier it becomes to change that code.

3. 20K lines and 100 files is a small project, in the sense that one person can be familiar with all of it. While limiting scopes is still beneficial for readability on that scale, it really starts to pay off when there are multiple people working on a project and no one person knows the whole thing.

I see parallels with modular design generally here: although it is helpful even on quite small projects, you can get away without it up to a certain point. Beyond that point, you really start to suffer from having different parts of your system tied together in ad-hoc, uncontrolled ways.

You're specifically using it for configuration/preferences though. You should consider changing it to a constant instead as that's what they really are.

The problem is passing state around in global variables. Any code can (and readily does) modify the state which is the problem.

$db wouldn't work as a constant.

You're right that look_ahead/company_address would work as a define("CONSTANT") better but honestly, I hate not putting a $ sign in front of a value (constant or variable). I am more prone to missing a $ than redefining a global. I don't remember ever having the problem of redefining a global unintentionally. I can give 20 examples of when I missed a $ symbol and PHP gave me an error/notice. If PHP constants had a symbol akin to $ (say #), I would definitely use them. But I just feel dirty typing "echo LOOK_AHEAD;" in one place and "echo $look_ahead_partial;" in another because I don't want to get in the habit of putting $ in some places and not putting them in others.

On the other hand, when someone reads "echo LOOK_AHEAD" they immediately know it's a constant and thus (a) global and (b)(more importantly) defined once and never modified by the code.

With "echo $look_ahead" there's no indication of that, and in the future another dev might add something that modifies $look_ahead's value and break other parts of the code, making for sneaky bug (which incidentally is why non-constant globals are so disliked ;))

$db is something different altogether, in my code I have a cheap Database class and each model creates a new one -- but it relies on the MySQL functions it uses to re-use the database connection. So in reality it's the same thing.

The problem with not using a constant where it's necessary is that you're communicating the wrong thing to someone else or even yourself 6 months down the line. You might want to re-evaluate what you see as "ugly code" especially if it changes how you write the code.

Not only that but depending what kind of db wrapper class you have you probably want it to be modified by the code that touches it. Sure you could pass it around, I guess it depends on how many people touch the code.

Personally I global the database wrapper, the user session class and config stuff (which yes I could use constants). Although if the code base I'm referring to wasn't 95% me coding I would probably consider using global less.

Store them in static private variables of a singleton object.

An example http://pastebin.com/Gf4fzHn2

This way you could lookup the parts of the code that use your global stuff, because they have to get it though the static method call.

This is exactly what my argument was against. Why make it complex when a simple global would do? Why say GlobalSingleton::var when global $unique_var would do, as long as the app does not abuse these global vars?

It's a few lines of code, I would not call that complex. And the usage is.

GlobslSingleton::set('Something Meaningful',$unique_var);

and

GlobslSingleton::get('Something Meaningful');

I don't think this is complex at all.

This doesn't force you to use $unique_var as a variable name(I know it can be worked around). One of the biggest problem with globals is that they can crash and burn if someone else's code purges it by accident.

Also my pattern allows 'load on demand', I can incorporate it in if I'd want to, your does not.

Might I suggest something like Config::get('look_ahead') rather than GlobalSingleton? The design pattern used to implement something is generally far less useful than the purpose of the thing in terms of a name.

Of course.

Don't you get sick of all the "global $look_ahead;" cruft at the top of your functions?

Also, what happens when one of your new developers comes along and modifies the value of that in some unexpected way?

I'm going to disagree with #9 that says using a core framework is bad. Now, if the core framework isn't a pre-built web framework like CakePHP, Symphony, there's a good chance there is some ... interesting things in there. But honestly having a "core" at least tries to reuse code.

Also disagreeing with #9. Plenty of popular frameworks have "core". core provides a clear delineation between extensible parts of a framework and its inner foundation. A well designed framework allows you to extend its functionality and upgrade the core at any time without disruption. Frameworks have core for the same reason objects have private members: interface opaqueness provides change protection.

I think that if you're using a well-developed framework, you can safely ignore #9. However, his advice is SPOT-FREAKING-ON for any home-grown stuff.

The last time I ran across a "core" directory, there was all kinds of application-specific code in there, all rolled up in switches and stuff - if you wanted to develop something using the "core", you had to go through about fifty files and add your own cases for your site. It was awful.

Lots of things though I think could be a bit more careful about a core, wordpress as an example includes a large amount of code on each and every page, when in reality a lot of the time much of it doesn't get touched every load.

Another sign (and that goes for any software): bad commenting and bad naming. If the naming and commenting is good, you most likely have a programmer who cares, which is half of the battle.

So true. In fact, I think this argument outweighs any or even all of the author's arguments.

I doubt there are many who would argue that Drupal is crappy software. And here's the shocker: it uses globals and most of it is procedural PHP! Let that sink in... Signs alone do not prove guilt. =P

But -- just to be a hypocrite -- my number one sign of crappy software is deeply nested if statements. The more you branch the more loose ends you have to juggle up in the air as your solution is formulating.

Just to be productive, allow me to share a tool that has evaded me for many years: assertions. No really!

  function GoodFunction
     assert(good_state)
     do_one_thing_very_well

Reads better, and leads to better design than:

  function GoodFunction?
    if( oh_right )
      do_one_thing_very_well_slightly_differently
    else
      do_one_thing_very_well

In the first the oh_right condition just doesn't fit in and you start thinking about what caused this logical branch in the first place. In the second we feel little hesitation to add an elseif if need be, further increasing complexity.

To give credit where its due, "Coders at Work" has actually changed my coding style. I highly recommend the book.

Another sign: check the mysql queries. If they don't properly escape variables, danger.

if they escape variables: danger!

Escaping is not sufficient to prevent SQL Injection Attacks. You must used Parameterized Queries.

http://bobby-tables.com/

> Escaping is not sufficient to prevent SQL Injection Attacks.

Do you have an example or idea of how a SQL injection could occur despite using http://php.net/manual/en/function.mysql-real-escape-string.p... ?

I'm pretty sure the audience on HN understands sql injection without having to refer to a cartoon.

Yes, but Little Bobby Tables is an inside joke for the HN audience. I love that comic!

It's a good one, no doubt. And it's true that the resources of the linked site go beyond the world of drawn panes.

Also, using data from $_GET, $_POST directly in Views.

We are still suffering from the effects of a developer who employed #1 throughout the codebase. We have both Object and Collection, and bypassing his classes results in reduuced memory usage by a factor of 10 and improved performance by a factor of 100. Unforunately he also employed #7 so it's very tricky to bypass that in some places without doing a major rewrite...

"8. The software messes with the error level"

I did this one recently, because I needed to use some PEAR modules that produced notices under E_STRICT. I somehow think all of these have similar exceptions.

Yes, you often want to do this just to make sure you're overriding the server's defaults.

If you want all your notices logged into an error file as well as the fatal errors, and the server defines the default error level as E_ERROR, then you'll have to override the default error level.

Horrible article.

These signs would be present in almost any crappy software, not just PHP.

also, the ones that don't use an ORM, instead go at lengths to invent it.

What about Wordpress? They don't use an ORM do they?

I would posit that Wordpress is the wordwide leader in 'crappy PHP software.'

(And I'm not a PHP hater, my company writes 90% of our stuff in PHP, and WP works great for blogs and brochure sites. But it's nasty underneath that pretty UI.)

Yeah, I've found it a bit of a nightmare at times trying to integrate it into a website, in future I plan to just use a separate user system for the site and just link them.

I guess it depends how you measure good software. Wordpress works. Really well.

Wordpress has such a long history of catastrophic security flaws that it's become notorious among people (like me) who don't even use it.

I am probably not all the different from you. But I have been running Wordpress on my own server for years and have never had a problem.

#1. It is written in PHP.

(sorry, I couldn't resist)

Reply on news.ycombinator.com