Bad practice!

Not for 'how-to' coding questions but PHP theory instead, this forum is here for those of us who wish to learn about design aspects of programming with PHP.

Moderator: General Moderators

User avatar
patrikG
DevNet Master
Posts: 4235
Joined: Thu Aug 15, 2002 5:53 am
Location: Sussex, UK

Bad practice!

Post by patrikG »

What would you consider to be bad practice when coding PHP?

Rant all you want against something you really, really sneer at, but at the end of your post, say how it should be done.

Personally, what I find very bad practice is mixing PHP with an HTML-page. Even worse, assigning each line of HTML to one PHP variable. Not only does it look ugly, it makes life so much more difficult in that logic and presentation are so entwined that you just see escaped quotes here there and everywhere, have to search for every little snippet of code that has anything to do with logic and then, once you've made the changes required, someone decides that it should actually look entirely different, but have the same, yet somewhat advanced, functionality.
Mixing PHP and HTML is like landfills. You dig a hole, pour concrete in and dump everything into it. Once it's full, you move on to the next.

Why not think about recycling. Think about re-using your code. Think how to abstract from what you have. There are so many excellent examples on the internet (and so many very bad ones) - go have a look at them, think about it the code first, think what you want to achieve, how to best structure it, then implement it. PHP can be so much more than a simplistic templating engine.
User avatar
redhair
Forum Contributor
Posts: 300
Joined: Fri May 30, 2003 4:36 pm
Location: 53.23N-6.57E
Contact:

Post by redhair »

What template engine do you use?
I find myself using Yapter lately. http://yapter.sf.net
User avatar
patrikG
DevNet Master
Posts: 4235
Joined: Thu Aug 15, 2002 5:53 am
Location: Sussex, UK

Post by patrikG »

Heh, I wasn't talking about templates, but I am using my own template engine - if Manuel Lemos accepts it, it'll be on phpclasses.org soon.
User avatar
redhair
Forum Contributor
Posts: 300
Joined: Fri May 30, 2003 4:36 pm
Location: 53.23N-6.57E
Contact:

Post by redhair »

You did say "PHP can be so much more than a simplistic templating engine.".

And the problem you sketch out, can best be solved using templates.

With "you" I meant "All of you reading this".

So what do "You" use?

I'm just curious about the result.
User avatar
patrikG
DevNet Master
Posts: 4235
Joined: Thu Aug 15, 2002 5:53 am
Location: Sussex, UK

Post by patrikG »

At one level, I agree, redhair - the problem could easily be solved with a templating engine. But what I was getting at was that when you start programming, you should
a) have a very clear idea of what you want
b) think about how to make the code re-usable
c) use object oriented programming
d) at some point in the distant future start reading up on design patterns

Actually, onle a) and b) were in my post above, but why the kind of "bad practice" coding I mentioned above annoys me, is
a) I've done it
b) I've done it too many times
c) It cost me hours/days/weeks to modify things
d) seeing others repeat the same mistakes over and over and over again
e) at work I am the one who has to maintain and fix the code.
McGruff
DevNet Master
Posts: 2893
Joined: Thu Jan 30, 2003 8:26 pm
Location: Glasgow, Scotland

Re: Bad practice!

Post by McGruff »

patrikG wrote:Personally, what I find very bad practice is mixing PHP with an HTML-page.
That would be the first thing on my list.

Also, some basic refactoring techniques:

(1) wrap code in short functions which do just one thing; 6-7 lines is a good size
(2) anything which is repeated should (usually) be resolved into a single source
User avatar
m3mn0n
PHP Evangelist
Posts: 3548
Joined: Tue Aug 13, 2002 3:35 pm
Location: Calgary, Canada

Post by m3mn0n »

My list:

1. Lack of script structure
2. Lack of spacing
3. Ambundance of HTML within PHP (or vice versa)
4. Unorganized application structure
5. Classes that do too much
6. Unnessesary use of functions
7. Too much comments
8. Not enough comments

To explain those points would require me writing a book, sorry. ;)
McGruff
DevNet Master
Posts: 2893
Joined: Thu Jan 30, 2003 8:26 pm
Location: Glasgow, Scotland

Post by McGruff »

Sami wrote: 6. Unecessary use of functions
Not sure about this one: in fact I'd almost say you can't have too many.

I hardly ever have code that isn't wrapped up in a fn (or rather a class method) and you never regret refactoring big functions down into smaller parts.

Of course, I don't know exactly what you have in mind in mentioning this.
timvw
DevNet Master
Posts: 4897
Joined: Mon Jan 19, 2004 11:11 pm
Location: Leuven, Belgium

Post by timvw »

If love scripts that make use of global variables and that start like this

<?
if ($is_logged_in) {
// send lotz of money to user
}
?>

I don't really like short tags either, but then again, this is for the style police.;)
User avatar
patrikG
DevNet Master
Posts: 4235
Joined: Thu Aug 15, 2002 5:53 am
Location: Sussex, UK

Post by patrikG »

I agree Sami & McGruff, also what I find "highly improvable", i.e. annoying, is when you read code and it's all in those one-character variables $a to $z. At best maybe even two characters.

Jeez, the machine understands the code anyway, but us humans have to maintain it. Make the code as readable as possible and if you return to a piece of code written 10 months ago, you'll find your way around much more easily.
McGruff
DevNet Master
Posts: 2893
Joined: Thu Jan 30, 2003 8:26 pm
Location: Glasgow, Scotland

Post by McGruff »

Exactly: I'd say about two thirds of the effort goes into writing easily understandable & maintainable code - good names, clear design, etc.
User avatar
Bill H
DevNet Resident
Posts: 1136
Joined: Sat Jun 01, 2002 10:16 am
Location: San Diego CA
Contact:

Post by Bill H »

1. Unnecessary use of functions

I agree with Sami that functions can be overused. Any function that performs a highly specialized routine and is only called one time in an entire routine (is so specialized that it can only be called one time) and is no more than a few lines of code merely serves to obscure what is going on. I see that done more often than you might think.

2. Over-use of object oriented programming.

Using classes purely for the sake of using classes does not help the routines. For small, simple routines (sites) it merely makes things more unwieldy and obscure.

I have discarded third-party scripts for both of the above reasons, because they were so obscured by the overuse of classes and functions that they were virtaully impossible to decipher and modify.

Remember the KISS principle. (Keep It Simple Stupid)
McGruff
DevNet Master
Posts: 2893
Joined: Thu Jan 30, 2003 8:26 pm
Location: Glasgow, Scotland

Post by McGruff »

Bill H wrote:1. Unnecessary use of functions

I agree with Sami that functions can be overused. Any function that performs a highly specialized routine and is only called one time in an entire routine (is so specialized that it can only be called one time) and is no more than a few lines of code merely serves to obscure what is going on. I see that done more often than you might think.
I respectfully but strongly disagree with this. The purpose of functions is to isolate discrete tasks. It doesn't matter how many times each task is performed in a script: stick 'em in their own function. This creates chapters and titles in the code greatly increasing readability imo.

For example, management tasks suddenly become clearer for what they are:

Code: Select all

<?php
function selectAction($parameter)
{
    if(isValid($parameter))
    {
         doThis();
    
    } else {
    
        doThat();
    }
}
?>
Notice that the test has been factored into its own isValid function, as well as the actions for each case. If these were very simple, agreed there isn't much point but anything more than a few lines would trigger a rewrite imo.

You don't even have to understand any php to figure out selectAction. It's immediately clear it doesn't actually do anything itself: it decides what should be done. That's all you want to know about it. A script workflow decision has been made that much clearer and easier to track down.

I've seen (and written) a million fns where the whole caboodle gets lumped into one, indecipherable function with dozens of lines or more. The workflow decision gets obscured by the actions being performed and vice versa. You shouldn't have to take 10minutes to figure out what a fn does. If they are short, single-shot fns you don't have to.
Bill H wrote:2. Over-use of object oriented programming.

Using classes purely for the sake of using classes does not help the routines. For small, simple routines (sites) it merely makes things more unwieldy and obscure.
Personally I find fns very unwieldy for anything much more complicated than "hello world". I honestly wouldn't know where to start if I had to try to work with fns only.

There's good OOP and bad OOP. If procedural scripts can be written badly, then I guess that goes double for OOP. But, the good stuff is very good. It's not slow or unwieldy or verbose or any of the usual criticisms levelled at object oriented code. As with fns, ruthlessly refactor classes to do one thing only.

I have a forum with a reasonably big index page - about 20 or so separate boards. There are about 30 separate classes in the script (and I haven't finished refactoring or added a db layer yet) but the whole page takes about 3 or so tenths-of-a-second (on an ancient Duron 700).

Don't be afraid of splashing out on fns or classes - just make sure they're good ones :)
Last edited by McGruff on Tue Aug 09, 2005 1:32 pm, edited 2 times in total.
McGruff
DevNet Master
Posts: 2893
Joined: Thu Jan 30, 2003 8:26 pm
Location: Glasgow, Scotland

Post by McGruff »

PS: hope you don't mind patrick but I thought this would be good for the theory & design board.
User avatar
Bill H
DevNet Resident
Posts: 1136
Joined: Sat Jun 01, 2002 10:16 am
Location: San Diego CA
Contact:

Post by Bill H »

Code: Select all

<?php
function printhdr($Param)
{
     echo "Whatever " .  $Param . " rest of line<br><br>";
}
?>
This function was in a separate file, in a separate directory, and it was used once in the entire project. Sorry, McGruff, I cannot see that as being in any way useful. (It was one of quite a few similar cases in the project.)
2. Over-use of object oriented programming.
I probably should have added
3. Not using object oriented programming when needed.
Post Reply