View Single Post
Old 08-19-2008, 10:07 PM   #3 (permalink)
Village Idiot
Wizard
Top Contributor 
 
Village Idiot's Avatar
 
Join Date: Sep 2007
Posts: 1,299
Thanks: 17
Village Idiot is on a distinguished road
Default

From a quick run though:

-You have that extensive documentation comment block on the top, but almost nothing as you go.

-Your user function should use an existing connection. Not only is a connection not its intended purpose, it makes it harder to port.

-Why both validlogin and loggedin? Wouldn't one imply the other?

-Don't track users by username, use an auto-incrementing primary key int. This way refferences to that user (such as in a URL) will not break. Lets say UserMan changes his name to ManUser. The link User.php?name=UserMan that the person gave to someone last week will no longer work. However, if you use a unique ID that will never change, it can always be User.php?id=1 no matter what changes. It is also faster for a database to track by primary key, not to mention you can index.

-Instead of setting all these variables to false if the query fails, just return false. The code you would use would be something like:
PHP Code:
if($user->getuser($username,$password) == true)
{
code
}
else
{
echo 
"Login failed";

-Don't assign things to a variable then put that variable to another variable (eg. line 109). Just assign directly to the variable.

-Why assign a class method to a variable and run it in a context where you could just call the member itself (line 119)? Just run the method.

-Line 121, your query would be better suited as something like (I didnt debug it due to lack of your db schema):
Code:
SELECT username, email, COUNT(username) AS nameCount,COUNT(useremail) AS emailCount FROM userdata WHERE username='$username'
You then just check the count of result nameCount and emailCount.

-Don't track sessions by IP, they change too often and are inconsistent..
__________________

Village Idiot is offline  
Reply With Quote
The Following User Says Thank You to Village Idiot For This Useful Post:
adamsargant (08-21-2008)