TalkPHP - View Single Post

Tanax · 09-17-2007, 04:32 PM

Quote:

Originally Posted by Salathe

Database Class

Lines 23-25: Constants DONE
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 DONE
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 DONE
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 DONE
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 DONE
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 DONE Why? :P
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 DONE
More defining of global constants, this time the name (MEMBER_TABLE) doesn't even follow the DB_ convention.
Line 38: Update query DONE I think..
I'm a fan of the UPDATE TABLE (...) VALUES (...) syntax and I feel it would be neater in this case.
Line 47: Syntax error DONE
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(); NOT DONE
The member class isn't a child of any other class so you cannot use parent
Line 70: strip NOT DONE
What is the purpose of this function?
SQL Queries NOT DONE
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 NOT DONE
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.

About the fetch, how would that function look? If I were to use it without a variable in it?

PHP Code:


			
public function fetch() {



}

with that structure.

About LIMIT 0,1... why? :S I gather the row where `u_id` = $u_id so I only get 1 row anyways..