PHP OOP Login Script

I'm new to both PHP and OOP and would like some constructive feedback on a class I made.

I have a "main account" login system already setup and working; when the user logs in they're presented with a list of games. This code logs said user into a specific game.

I have a separate database for each game, plus one for the main account. Tables for the games include 'Players' 'Login History' and 'Player Inventory'

For security I choose to have both prepared statements and issue a unique token at login stored in the 'player' table; which will be checked anytime there is an interaction with the database.

Things I'm looking for:

-
Bad programming practices: I want to learn to do things the right way
before my bad code becomes a bad habit.

-
Readability: Goes along with the bullet above; but the easier it is
to read and maintain the better; and hints in this area are more than
welcome.

-
Security: Being a game I want this as secure as possible. Any
direction in this area is highly appreciated.

-
Better way of doing this: I often miss the obvious and with the
amount of experience I have there could easily be a better way I
haven't thought of.

```
game_name = $game;
$this->user_id = $user_id;

switch($game)
{
case 'game_001' :
//set up variables
$this->game_id = 001;
$this->host = 'host';
$this->database = 'game_001';
$this->username = 'dbUser';
$this->password = 'dbPassword';
break;
default :
die('No database exists.');
}

//delete this
echo 'Login started...';

//login to game
$this->login();
}

public function __destruct()
{
//close database connection
$this->db_connect->close();

//delete this
echo 'Login has been closed.';
}

private function login()
{
//connect to database
$t

Output of utility class

You are outputting error conditions and debugging statements to the output buffer on several occassions. This is an utility class, used by other code to log into a game. Error conditions should set a specific attribute that other code can use to detect an error and show an appropriate message such as "The server is currently under maintanance. Please try again later." when a database is not found, while you log useful information for developers only. Similarly, you can show a similar message when the player is, for example, already logged into the game.

Don't show sql errors to the user. They belong in a private error log somewhere. Give the user a user-friendly error message instead.

The ever-growing switch

In your constructor you have an ever-growing switch statement. These details do not belong in this class. You might choose to store them in their own table, or have a file with constants somewhere. You are setting yourself up for a world of hassle by duplicating login data, id's etc. over several files, then having to change them everywhere and potentially forgetting something.

Class naming

I don't think Login is a good name for this particular class. What you seem to try to emulate is a game session, so you probably should name it that instead.

Token generation

I am not sure what you are trying to accomplish with your token generation. uniqid() has two optional parameters, the first being a string containing a prefix, the second being a boolean to have more entropy. You seem desperate to get something unique, so you generate a random number between 0 and rand_max (which is then converted to a string for a prefix), and set "more entropy" to true, so you generate an even bigger token. Uniqueness is still not guaranteed though. Then you throw it through md5(), which is a hash function, which will thus increase the likelyhood that your token matches an other token.

You do not share why you are generating a token in the first place, or what problem you are trying to solve with it. If you just need a unique value to store game data with, why not use the player id? I assume a player has a saved game state per game, and can only play the same game one instance at a time. If you need to share a token with the client, why not use the session? The session token is already shared with the client, and you can just store your information in the session itself.

If you really need a token, try to use just uniqid("", TRUE); or uniqid("server-id", TRUE); if you are using some kind of load balancer set-up.

Orphaned sessions

Your login creates a game session, and stores it in the session variable. At no point you ever check if such a session already existed. You probably want to not automatically login in this class, but instead have both the login and logout procedures here. In the constructor retrieve any information you have on the current session, and let the login and logout procedure handle creation or destruction of the game session.

You can use this class in other operations too to check if the user is still logged in.

Use of comments

Don't use comments for single function calls. If your function names are to cryptic to understand what they are doing, you should rename the function. If a function has side-effects, you should rewrite it so those side-effects are eliminated. Otherwise, you are just stating the obvious. In other words: Don't do this.

//login to game
$this->login();

Whitespace

Your whitespace is mostly consistent. I would recommend adding a space after a comma, just like you would do in a normal sentence. The space makes argument lists a lot more readable.

date_default_timezone_set

I don't think this function call belongs in this class file. Instead set the default timezone in a config file. Consider using UTC instead of a random timezone somewhere in the world. You can convert this timestamp to a specific timezone if you need to display it to the user, hopefully based on some kind of preference.

StackExchange Code Review Q#146808, answer score: 2