09-17-2007, 03:10 PM
|
#6 (permalink)
|
|
Moderateur
Join Date: Apr 2007
Posts: 1,393
Thanks: 5
|
I've got a whole bunch of things to comment on, so bear with me because it might turn out to be quite a long post! Note that the line numbers mentioned are taken from a direct copy/paste of the code in your first post.
Database Class- Lines 23-25: Constants
It would be better to keep things relevant to the database class within the scope of said class. Define the host, user, password, etc as private member variables rather than as global constants. It avoids any potential problem with overlapping constant names, and also prevents any other class from getting access to those precious connection credentials.
- Lines 42, 53, 65: Method/function names
There's no need to append "db" to the end of the function names. We're within a class called "database", it should be obvious already what the functions do! I'd suggest using just connect/disconnect (merging the select function into connect).
- Line 42 & many other places: Constants not used properly
When connecting to the database and in many other places throughout both classes you try to make use of your constants, but incorrectly. If you do define('MY_CONSTANT', 'My Constant Rocks!') and attempt to echo it using: echo "MY_CONSTANT" what do you think will be echoed? The actual answer is MY_CONSTANT and not My Constant Rocks!. However if you were to write: echo MY_CONSTANT then the result would be as expected. All over your two classes you make this mistake.
- Line 102: database::define function
What's the point of this, really? It is just as easy to use PHP's define method and see the comments above about using global constants at all.
- Line 149: Session
Why are you mixing sessions and database activity? They should remain completely separate (as far as your classes are concerned). Also the comment is factually incorrect: it does not create a session at all.
Member Class- The whole concept here I think could do with a little improvement. I'd like to see a
Member object which simple contains that member's credentials (the $data variable in your class). This class is an interface between the database and a member -- what if you wanted to retrieve the member's details from an altogether different source instead of the MySQL database? You'd have to rewrite the whole class again, essentially. Anyway, on to specific points within the class as it stands at the moment:
- Line 20: Constructor
Perhaps it might be an idea in this case to add a type hint for the $db property: __construct(database $db, $tablename)
- Line 23: Define
More defining of global constants, this time the name (MEMBER_TABLE) doesn't even follow the DB_ convention.
- Line 38: Update query
I'm a fan of the UPDATE TABLE (...) VALUES (...) syntax and I feel it would be neater in this case.
- Line 47: Syntax error
You have a typo: $this-data['password'] is missing the >. Also note that the SQL query built up on this line would also contain a syntax error since there is a missing closing bracket at the end of the VALUES list.
- Line 61: parent::fetch();
The member class isn't a child of any other class so you cannot use parent
- Line 70: strip
What is the purpose of this function?
- SQL Queries
Many times you're online looking for one row to be returned from the database. Save your DBMS some time and effort and tell it to only return a single row using LIMIT 0,1.
- Fetching results
I don't see anywhere where you use mysql_fetch_* to bring back results from the database. Surely a logical place would be in your fetch function but alas, that's not the case.
Something I need to ask is, have you actually tried using your code on your own server (localhost?) at all? The first thing that I did was copy/paste your two classes into files and tried to execute them -- syntax errors show up right away (assuming you have error tracking/reporting turned on on your development machine).
As for the grey comment lines in PHP Designer, can you not go into the settings and change the colour to something more readable (like the orange)? On the topic of comments, it's nice to see that you've at least thought about adding comments to your code but to be honest they're really not all that useful. For example, commenting the database constructor with "Our constructor that auto-loads" is just stating the blindly obvious. More useful would be to comment on what the constructor actually does "Defines global constants for the class to use and attempts to connect to the database." Most of the other comments follow in a similar vein; basically restating the associated function's name.
P.S. Sorry for such a long post!
|
|
|
|