Signs of Crappy PHP Software

http://www.phpfreaks.com/blog/10-signs-of-crappy-php-software

by lox • 16 years ago

16 40 16 years ago

phpfreaks.com

petervandijck2• 16 years ago

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

jacksoncarter• 16 years ago

if they escape variables: danger!

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

http://bobby-tables.com/

chime4• 16 years ago

> 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... ?

code_duck2• 16 years ago

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

petervandijck2• 16 years ago

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.

arnorhs2• 16 years ago

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

neovive• 16 years ago

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

phoenix24• 16 years ago

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

jaxn3• 16 years ago

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

crikli• 16 years ago

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.)

ojbyrne• 16 years ago

"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.

arnorhs2• 16 years ago

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.

chime4• 16 years ago

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.

_Lemon_2• 16 years ago

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.

chime4• 16 years ago

$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.

DjDarkman3• 16 years ago

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.

chime4• 16 years ago

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?

Chris_Newton• 16 years ago

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.

arghnoname• 16 years ago

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.

lox2• 16 years ago

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?

afhof• 16 years ago

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

subwindow• 16 years ago

#1. It is written in PHP.

(sorry, I couldn't resist)

alttab• 16 years ago

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.

snorkel• 16 years ago

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.

robryan3• 16 years ago

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.

DanHulton• 16 years ago

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.

pacemkr• 16 years ago

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.

wanderr• 16 years ago

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...

Signs of Crappy PHP Software

Related Stories