To register for an Internet.com membership to receive newsletters and white papers, use the Register button ABOVE.
To participate in the message forums BELOW, click here
PHPBuilder.com  
 

 

Go Back   PHPBuilder.com > PHP Help > Code Critique

Code Critique Having someone critique your code is always a great way to hone the skills. Stop in and post your code to see what your peers may have done differently.

Reply
 
Thread Tools Rate Thread Display Modes
Old 09-30-2003, 03:10 PM   #1
Sxooter
Chamberlain
 
Sxooter's Avatar
 
Join Date: Aug 2002
Location: Denver, CO
Posts: 4,056
Common mistakes

Here's a couple of common mistakes you see a lot in older php code that can bite you in the butt with 4.3.x.

Unquoted identifiers in an array:

$string['a'] = "text";
print $string[a];

While this would work in older versions of PHP fine, lately, it generates this notice:

Notice: Use of undefined constant abc - assumed 'abc' in - on line x

Also, testing for a value instead of using isset:

if ($varname) {
do_something();
}

If the variable $varname doesn't exist, this will now throw a warning:

Notice: Undefined variable: var in - on line 2

So, if you are writing for PHP 4.3/PHP 5, you should pay attention to these things.
__________________
PostgreSQL version 8.4 is now in beta! New features in the works: Updateable Views, WITH queries including recursive, On-disk bitmap indexes, and improved partitioning. woot!
Sxooter is offline   Reply With Quote
Old 09-30-2003, 04:23 PM   #2
bliss322
Member
 
Join Date: Dec 2002
Posts: 85
since we're on this subject, my recent, big mistake was not having a config file.

when i took the code to another site, i had to change all the instances of database names, tables, session names and such.

session names are the big one for me, that's how i check for admin access and everything.

i had a $_session["admin_user"] on my main site, and then created a subdomain using the same code. because it was a subdomain, that "admin_user" had admin access to my site.

now i have a config file declaring the database info, the table names, and the session names, too.
__________________
ahhh, learning.
bliss322 is offline   Reply With Quote
Old 09-30-2003, 04:48 PM   #3
BuzzLY
2($infinity) && $beyond
 
BuzzLY's Avatar
 
Join Date: Nov 2002
Location: Star Command
Posts: 2,535
As an added security measure, you can put that config file outside your website file structure, so nobody has any access to it at all.
__________________
New to the board? Check out the guidelines
| Color Picker | Blogification |
¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤
With all its sham, drudgery, and broken dreams, it's still a beautiful world.
BuzzLY is offline   Reply With Quote
Old 09-30-2003, 05:44 PM   #4
Sxooter
Chamberlain
 
Sxooter's Avatar
 
Join Date: Aug 2002
Location: Denver, CO
Posts: 4,056
Followup to my mistakes post

Note that the proper test to see if a var is set, or if it is set AND has a value other than 0, would be something like:

// Is var set?
if (isset($varname)){

}

or

/ Is var set and has a non-zero value:
if (isset($varname)&&$varname){

}

Note that evaluation order means that if the first isset fails, the second $varname check will not be made, and therefore no error. This means that the above check is NOT the same as:

if ($varname&&isset($varname)){

}

since that will result in the first evaluation occuring on a possible non-existent variable.

Good programming practices might seem to demand that you even:

PHP Code:
if (isset($varname)){
  if (
$varname){
  }
}
But that's probably overkill.
__________________
PostgreSQL version 8.4 is now in beta! New features in the works: Updateable Views, WITH queries including recursive, On-disk bitmap indexes, and improved partitioning. woot!
Sxooter is offline   Reply With Quote
Old 09-30-2003, 08:14 PM   #5
drawmack
Computers can do that?
 
drawmack's Avatar
 
Join Date: Apr 2003
Location: Pocono Mtns PA
Posts: 3,268
1) Code to a standard, any standard even if it is your own but write it down and have it laying around for future programmers.

I use a variation of: http://utvikler.start.no/code/php_coding_standard.html

2) Document to a standard, and an automated documentation extraction tool is preferable. I use phpdoc - http://www.phpdoc.de/

3) Always, always, always write if statments in this formate if('constant' == $variable) then the parser catches it when you type = instead of ==

4) Use the ! operator in if statments sparingly, pretend it is a contagous disease. It confuses the logic and makes the program harder to read.

5) Never assume register globals is on.
drawmack is offline   Reply With Quote
Old 09-30-2003, 08:43 PM   #6
Merve
black sheep with red wool
 
Merve's Avatar
 
Join Date: Jul 2003
Location: North of the 49th parallel
Posts: 2,579
Use other functions in place of complex array_splice() calls, as in using them in 'if' statements. wordwrap() or other functions will usually do in 2 or 3 lines what array_splice() does in 13 or 14 lines. I know this because I once wrote a wordwrap()-like function using array_splice that took 15 lines...It think. wordwrap() did the same thing in one.

As a general rule, shorten your code! Sift through the manual to find the function you want. If it doesn't exist, make your user-defined function as short as you possibly can.

Also, look through your code and make sure you have closed ALL brackets (brace or curved) this will save you headaches later.

Finally, don't use the delete() function as it doesn't exist. If you think it exists, you're a dumbass.
__________________
"A proof is a proof. What kind of a proof? It's a proof. A proof is a proof. And when you have a good proof, it's because it's proven." -- Jean Chrétien

The Three C's
Merve is offline   Reply With Quote
Old 10-01-2003, 02:31 AM   #7
Moonglobe
Better fan than rebelo!
 
Moonglobe's Avatar
 
Join Date: Apr 2003
Location: brain://localhost:left-side
Posts: 2,381
Quote:
Originally posted by Merve
Finally, don't use the delete() function as it doesn't exist. If you think it exists, you're a dumbass.
well then what's this:

delete



yes i know it's just a dummy entry.... but still if it's in the manual....
__________________
there's no place i can be, since i found serenity.
Moonglobe is offline   Reply With Quote
Old 10-01-2003, 03:28 AM   #8
drawmack
Computers can do that?
 
drawmack's Avatar
 
Join Date: Apr 2003
Location: Pocono Mtns PA
Posts: 3,268
Quote:
Originally posted by Merve
As a general rule, shorten your code! Sift through the manual to find the function you want. If it doesn't exist, make your user-defined function as short as you possibly can.
in theory this is excellent advice but if you were paying me by the hour I think you'd be a lot happier if I took 10 minutes to code something then if I took an hour to find just the right function.

There are a lot of ''needless'' functions in php. If I can code it in 3 or 4 lines I'm probably not even going to consider that it's already part of the language unless I'm using those 3 or 4 lines all the time.
drawmack is offline   Reply With Quote
Old 10-01-2003, 06:05 AM   #9
Jeb.
Titles are overrated.
 
Jeb.'s Avatar
 
Join Date: Jul 2003
Posts: 150
Don't re-invent the wheel. If there's a class on the Internet that'll do what you need (and there probably is), find it and use it. Of course, pay attention to licensing and what-not.

Modify the class if you have to, but don't write the whole thing from scratch if you can avoid it. This is the biggest problem with programmers - we are architects, and our first instinct is to rip it down and start again. Be careful - this could cost you a lot of development time!

The only time I rewrite something from scratch is if the license it's under prevents me from doing what I want with it. EG, I had to rewrite portions of the Eclipse library because of the license it was under (LGPL) - I couldn't allow the source of it to be viewable to the general public.

While I'm on the subject of classes, get a class library together. Have a whole bunch of classes doing different (but unique) things. That way when you need them, you just pull up your library, grab the class, whack it in and there you go.
__________________
Once you eliminate the impossible, whatever remains, however improbable, must be the truth. - Sir Arthur Conan Doyle
Jeb. is offline   Reply With Quote
Old 10-01-2003, 09:53 PM   #10
Merve
black sheep with red wool
 
Merve's Avatar
 
Join Date: Jul 2003
Location: North of the 49th parallel
Posts: 2,579
Excellent point drawmack. I didn't think it up.

Also, if you have an if, and an else, but no elseif, consider using the ternary operator...shortens your code a lot...and if you have a huge if-elseif-else structure with only one variable in the condition, consider using the switch statement. It is much better for debugging purposes...well the ternary operator is not always better for debugging purposes...but you get the picture. Short code is good. Long code with lots of loops is bad. Only use loops if you absolutely have to and it is the shortest way to execute the code.
__________________
"A proof is a proof. What kind of a proof? It's a proof. A proof is a proof. And when you have a good proof, it's because it's proven." -- Jean Chrétien

The Three C's
Merve is offline   Reply With Quote
Old 10-02-2003, 10:00 AM   #11
drawmack
Computers can do that?
 
drawmack's Avatar
 
Join Date: Apr 2003
Location: Pocono Mtns PA
Posts: 3,268
Tom like loops

I am the loop god
the loop to the dog
or the pool to the god
drawmack is offline   Reply With Quote
Old 10-03-2003, 02:15 AM   #12
Weedpacket
Custom User Title™
 
Weedpacket's Avatar
 
Join Date: Aug 2002
Location: Rapid Offensive Unit "Foreign Object Damage"
Posts: 19,128
Quote:
Originally posted by drawmack
in theory this is excellent advice but if you were paying me by the hour I think you'd be a lot happier if I took 10 minutes to code something then if I took an hour to find just the right function.

There are a lot of ''needless'' functions in php. If I can code it in 3 or 4 lines I'm probably not even going to consider that it's already part of the language unless I'm using those 3 or 4 lines all the time.
You know, that sounds not too dissimilar to my some-time code early, code often approach.

[PS; As for taking ten minutes instead of an hour, my rates tend to be in increments of 30 minutes. So taking ten minutes saves me fifty but the customer only thirty; I might as well spend those other twenty minutes on something useful.... I need a coffee.]
__________________
On two occasions I have been asked [by Members of Parliament], "Pray, Mr. Babbage, if you put into the machine wrong figures, will the right answers come out?" I am not able rightly to apprehend the kind of confusion of ideas that could provoke such a question.

Last edited by Weedpacket; 10-03-2003 at 02:18 AM.
Weedpacket is offline   Reply With Quote
Old 10-03-2003, 07:55 AM   #13
drawmack
Computers can do that?
 
drawmack's Avatar
 
Join Date: Apr 2003
Location: Pocono Mtns PA
Posts: 3,268
Weed, that's exactly what I was getting at and your article reminded me of another point.

place every sql statement for the web-site into a named array and have the function use them from here. This way a database change is easy to propogate through the site.

Actually what I've done is better then that. Write a generic database class that uses the information the database will give you and a table in the database to do all database stuff then you just make the change to the database and change the control table and you've updated the access methods as well.
drawmack is offline   Reply With Quote
Old 10-03-2003, 10:43 AM   #14
BuzzLY
2($infinity) && $beyond
 
BuzzLY's Avatar
 
Join Date: Nov 2002
Location: Star Command
Posts: 2,535
What a great idea. What would you call that? I think you should call it "PEAR."
__________________
New to the board? Check out the guidelines
| Color Picker | Blogification |
¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤¤
With all its sham, drudgery, and broken dreams, it's still a beautiful world.
BuzzLY is offline   Reply With Quote
Old 10-03-2003, 06:18 PM   #15
drawmack
Computers can do that?
 
drawmack's Avatar
 
Join Date: Apr 2003
Location: Pocono Mtns PA
Posts: 3,268
Buzz, nah mine draws the input screens and stuff too. I like mine better then PEAR but a lot of people might disagree.
drawmack is offline   Reply With Quote
Reply

Bookmarks


Currently Active Users Viewing This Thread: 1 (0 members and 1 guests)
 
Thread Tools
Display Modes Rate This Thread
Rate This Thread:

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is Off
HTML code is Off
Forum Jump


All times are GMT -4. The time now is 07:10 AM.






Acceptable Use Policy

internet.comMediabistrojusttechjobs.comGraphics.com

WebMediaBrands Corporate Info


Advertise | Newsletters | Feedback | Submit News

Legal Notices | Licensing | Permissions | Privacy Policy


Powered by vBulletin® Version 3.7.2
Copyright ©2000 - 2009, Jelsoft Enterprises Ltd.