First foray into OOP: Correct implementation?

  • (8 Pages)
  • +
  • 1
  • 2
  • 3
  • Last »

113 Replies - 8604 Views - Last Post: 23 March 2013 - 11:52 PM Rate Topic: -----

#1 Darkranger85  Icon User is offline

  • D.I.C Head

Reputation: 1
  • View blog
  • Posts: 148
  • Joined: 31-August 12

First foray into OOP: Correct implementation?

Posted 22 February 2013 - 11:33 AM

Hey again everyone,

As most of you know I'm working on a login / Registration system for a learning project and I have gotten close to the end of where my tutorials take me and I decided that it was time for me to do a major overhaul and basically rewrite things from the ground up.

Over the course of the tutorial and from input from the awesome staff here, I learned so many things I can't count them all and so my code was pretty unorganized because at the start I didn't know much and by the end I had learned how to do a lot of things better.

The first thing I'm working on is overhauling my functions pages to be more organized and I grouped the functions into classes.

I just want to know, before I go any further, if I'm heading in the right direction with my implementation.

<?php
/*This script is for housing functions and classes having to do with general stuff and security*/
	
	//THE GENERAL CLASS: Used for general functions
	class general{
			
		function email($to, $subject, $body){
 			mail($to, $subject, $message, 'From: registration@planetofruins.com');
 		}
		
		function output_errors($errors){
 		//Calling scripts: login.php
 			 		
	 		$output = array();
		
			foreach ($errors as $error){
				$output[] = '<li>'.$error.'</li>';
			}
		return '<ul id="error_list">'.implode('', $output).'</ul>';
 		}
		
	}//END class
	$GENERAL = new general;
	//END General Class
	
	//THE GRAB CLASS: Used for 'Grabbing' information such as IPs and dates.
	class grab{
		
		public function get_ip(){
		//DESCRIPTION: Trys to return the best possible ip address//
	 	//CALLING SCRIPTS: register.php, login.php
		    if (!empty($_SERVER['HTTP_CLIENT_IP'])){	    	
		      $ip_address = $_SERVER['HTTP_CLIENT_IP'];			
		    } else if (!empty($_SERVER['HTTP_X_FORWARDED_FOR'])){	      	
		      $ip_address = $_SERVER['HTTP_X_FORWARDED_FOR'];		  
		    } else{	    	
		      $ip_address = $_SERVER['REMOTE_ADDR'];		  
		    }
			
		    return $ip_address;
	 	}
	 
	 	public function get_date(){
	 		//DESCRIPTION: Returns the date in Year Month Day format.
	 		//CALLING SCRIPTS: users.php (login and register functions)
	 		return date('Y-m-d');
	 	}		
	}//Class end	
	$GRAB = new grab;
	//END of Grab Class
	
	
	//THE SECURITY CLASS: Used to hold the methods related to site security. Locking pages, generating codes, and encrypting data
	class security{
			
		public function get_code($username){
	 		return md5($username + microtime());
	 	}
	
	  	public function get_salt(){
	 		return hash('sha256', uniqid(rand(), true) + microtime());
	 	}
		
		public function crypt_password($password, $salt){
 		//Calling scripts: users.php
 			return $password = crypt($password, $salt);
 		}
 
 		public function protect_page(){
 			
 			if(!logged_in()){
 			header('Location: protected.php');
			exit();
 			}
 		}
	}//Class end
	$SECURITY = new security;
	//END of Security Class
?>



This is pure speculation at this point but I was thinking of declaring the globals of these classes on the init.php script that I have included on most pages for easy use.

However since I know there are 'taboos' on the use of global stuff I wanted to make sure this was a good idea as well.

Thanks again! :)

Is This A Good Question/Topic? 0
  • +

Replies To: First foray into OOP: Correct implementation?

#2 laytonsdad  Icon User is offline

  • Cheese and Sprinkles
  • member icon

Reputation: 447
  • View blog
  • Posts: 1,930
  • Joined: 30-April 10

Re: First foray into OOP: Correct implementation?

Posted 22 February 2013 - 11:44 AM

Take a look at Javadoc. These comments help when using an IDE.
Was This Post Helpful? 0
  • +
  • -

#3 andrewsw  Icon User is online

  • It's just been revoked!
  • member icon

Reputation: 3809
  • View blog
  • Posts: 13,518
  • Joined: 12-December 12

Re: First foray into OOP: Correct implementation?

Posted 22 February 2013 - 02:00 PM

I define a number of constants in my main include file:

<?php
date_default_timezone_set('Europe/London');
if (!defined('SALT')) define('SALT', 'SOMESALT');

if (!defined('LOCAL')) {
    define('LOCAL', true);
    if (LOCAL) {
        define('BASE_URL', 'http://localhost:81/AndysProject2/');
        define('EMAIL', 'someemail@host.com'); 
    } else {
        define('BASE_URL', 'http://mysite.com/');
        define('EMAIL', 'otheremail@host2.com');
    }
}
// Set the database access information as constants:
if (!defined('DB_USER')) {
    if (LOCAL) {
        DEFINE ('DB_USER', 'Andrew');
        DEFINE ('DB_PASSWORD', 'Password1');
        DEFINE ('DB_HOST', 'localhost');
        DEFINE ('DB_NAME', 'Andy2');
    } else {
        DEFINE ('DB_USER', 'andy_user');
        DEFINE ('DB_PASSWORD', 'Password2');
        DEFINE ('DB_HOST', 'some.host.com');
        DEFINE ('DB_NAME', 'somedb');
    }
}
?>

Then all I need to do is change the word true to false (for LOCAL) when I'm switching between local testing and my live site. I know there are more technical ways to do this, which wouldn't even require this minor change, but this approach suits me :)

If this seems useful to you then you could replace registration@planetofruins.com in your email() method with EMAIL.

Of course, you are free to ignore this suggestion!

You might also decide to use optional parameters with functions like email() and get_date(), so that you can use them as they are, but also supply a different 'From:' address and date-format. However, I personally don't tend to create methods like that, as they are not doing much and they require additional function-calls. I think some would disagree with me though.

But your approach seems fine to me and you might ignore my comments :)
Was This Post Helpful? 2
  • +
  • -

#4 andrewsw  Icon User is online

  • It's just been revoked!
  • member icon

Reputation: 3809
  • View blog
  • Posts: 13,518
  • Joined: 12-December 12

Re: First foray into OOP: Correct implementation?

Posted 22 February 2013 - 02:06 PM

But actually, to continue with your OOP approach, you might add some properties to your classes, perhaps in place of my constants-suggestion.

Personally, I'm happy to use my constants as I consider them as essential globals to the whole site.

[I'm awaiting criticism of my simple approach :) ]

This post has been edited by andrewsw: 22 February 2013 - 02:11 PM

Was This Post Helpful? 0
  • +
  • -

#5 JackOfAllTrades  Icon User is offline

  • Saucy!
  • member icon

Reputation: 6107
  • View blog
  • Posts: 23,661
  • Joined: 23-August 08

Re: First foray into OOP: Correct implementation?

Posted 22 February 2013 - 03:48 PM

If you're creating classes holding useful functions, but these functions do not use any variables or functions of the class, consider making these function static. So your grab class would be

class Grab
{
    public static function getIp()
    {
        //DESCRIPTION: Trys to return the best possible ip address//
	//CALLING SCRIPTS: register.php, login.php
	if (!empty($_SERVER['HTTP_CLIENT_IP'])){	    	
	    $ip_address = $_SERVER['HTTP_CLIENT_IP'];			
	} else if (!empty($_SERVER['HTTP_X_FORWARDED_FOR'])){	      	
	    $ip_address = $_SERVER['HTTP_X_FORWARDED_FOR'];		  
	} else{	    	
	    $ip_address = $_SERVER['REMOTE_ADDR'];		  
	}
			
	return $ip_address;
    }
	 
    public static function getDate($format = 'Y-m-d'){
        //DESCRIPTION: Returns the date in Year Month Day format.
        //CALLING SCRIPTS: users.php (login and register functions)
        return date($format);
    }		   
}


Now you don't need to instantiate these, but just call them directly:

$ip = Grab::getIp();
$date = Grab::getDate();


With the defaulted format argument on getDate, you can now change the format if you'd like by passing in a different format.

Also note the variable casing. This is fairly standard camelCase style, which is (to my knowledge) widely accepted as the norm. cf. PSR-1
Was This Post Helpful? 3
  • +
  • -

#6 Darkranger85  Icon User is offline

  • D.I.C Head

Reputation: 1
  • View blog
  • Posts: 148
  • Joined: 31-August 12

Re: First foray into OOP: Correct implementation?

Posted 22 February 2013 - 05:00 PM

Thanks guys!

@andrew: I appreciate the input and I would never 'ignore' the suggestions of a knowledgeable person such as yourself. I will certain look into your suggestions and see if it works for me.

As far as adding properties, I'm not 100% sure what you mean. Could you give an example?

@JackOfAllTrades: Thank you for the suggestions! That helps me out a lot! I'll get to work implementing that right away!

One question: When you say a function that doesn't use a variables of functions of the class do you mean that the functions are just self contained and do their own thing? Or do you also mean that they don't take outside input?

Cause if it's the former would that mean all of my functions listed could be static?

And on a side note, since I'm redoing all this I'd really like to get to the bottom of why I'm getting a multidimensional array back from fetchAll. Cause it makes my code just that much harder to keep straight lol.

This post has been edited by Darkranger85: 22 February 2013 - 05:17 PM

Was This Post Helpful? 0
  • +
  • -

#7 Darkranger85  Icon User is offline

  • D.I.C Head

Reputation: 1
  • View blog
  • Posts: 148
  • Joined: 31-August 12

Re: First foray into OOP: Correct implementation?

Posted 22 February 2013 - 06:11 PM

Alright,

I might have bitten off a bit much.

I put all the functions in classes and then when around changing function calls to account for the classes.

But now I'm getting errors such as:

Notice: A session had already been started - ignoring session_start() in C:\wamp\www\Projects\Planet Ruin v3\core\init.php on line 3

Fatal error: Cannot redeclare class users in C:\wamp\www\Projects\Planet Ruin v3\core\functions\users.php on line 5

I'm afraid that my eyes glazed over a bit and I'm not sure where to begin.

Note: Please don't be offended if all the suggestions I've gotten haven't been implemented yet. I haven't gotten that far yet.

init.php

<?php
//Starts session and requires connect and functions scripts for use around the site
session_start();
require 'core/connection.php';
require 'core/functions/users.php';
require 'core/functions/general.php';

//general.php classes
global $GENERAL;
global $GRAB;
global $SECURITY;
//users.php classes
global $USERS;
global $CHECK;
global $RESOURCES;

if ($CHECK->loggedIn()){
	$session_id = $_SESSION['user_id'];	
	//Gathers user data from the user_data table for use anywhere.
	//LOOK INTO: Possibly extract() the $user_data array for easier use. Fix miultidimentional array problem first.
	$user_data = $RESOURCES->userData($session_id, 'id', 'username', 'password', 'salt', 'email', 'secret', 'active', 'date_reg', 'last_logged', 
					  	  'ip_address', 'secret', 'secret_hint');
	
	//Watches for users to be 'Deactivated' and logs them out.
	if (!$CHECK->userActive($user_data[0]['username'])){
		session_destroy();
		header('Location: index.php');
		exit();
	}
}

//Global errors array
$errors[] = array();
?>



class users{
	public $PDO;
	
	function login($username, $password){
	//Calling scripts: login.php
	//WORKAROUND: If user supplies incorrect username query returns empty array. Added if statement to check for empty array before returning.
	//LOOK INTO: Fix the multidimentional array problem.
	
		//Query returns the salt and password of the user.	
		$query = $PDO->prepare("SELECT `salt`, `password`, `user_id` FROM `user_data` WHERE `username` = ?");
		$query->execute(array($username));
		$result = $query->fetchAll(PDO::FETCH_ASSOC);
		
		//Salt is used to crypt the password to be matched with the password and validate the user.
		if (!empty($result)){
			$db_password = $result[0]['password'];
			$salt = $result[0]['salt'];
			$user_id = $result[0]['user_id'];
			$password_crypt = $SECURITY->cryptPassword($password, $salt);
			
			//Insert last logged and ip_address
			$last_logged = $GRAB->getDate();
			$ip_address = $GRAB->getIp();
			$query = $PDO->prepare("UPDATE `user_data` SET `last_logged` = ?, `ip_address` = ? WHERE `username` = ?");
			$query->execute(array($last_logged, $ip_address, $username));
			
			//If passwords match, return user_id to make a session.
			return ($password_crypt === $db_password) ? $user_id : false;
		} else{
			return false;
		}		
	}

	function registerUser ($register_user_data, $register_player_data) {
		//Calling scripts: register.php
		//LOOK INTO: Make empire_name_exists function and integrate into registration system.	
	    $salt = $register_user_data['salt'];
	    $register_user_data['password'] = $SECURITY->cryptPassword($register_user_data['password'], $salt);
	    
		//Query for user data
		$sql = "INSERT INTO `user_data` SET ";
		$sets = array();
		
		foreach (array_keys($register_user_data) as $key) {
	    	$sets[] = "`{$key}` = :{$key}";
		}
		$sql .= implode(",\n", $sets);
		
		$query = $PDO->prepare($sql);
		
		foreach ($register_user_data as $key => $value) {
	    	$query->bindValue(":{$key}", $value);
	    }
		
		$query->execute();
		
		//Query for player data
	    $sql = "INSERT INTO `player_data` SET ";
		$sets = array();
		
		foreach (array_keys($register_player_data) as $key) {
	    	$sets[] = "`{$key}` = :{$key}";
		}
		$sql .= implode(",\n", $sets);
		
		$query = $PDO->prepare($sql);
		
		foreach ($register_player_data as $key => $value) {
	    	$query->bindValue(":{$key}", $value);
	    }
		
		$query->execute();
		
		$GENERAL->email($register_user_data['email'], 'Account Activation', "Hello, ".$register_player_data['ruler_name']."\n\nThank you for registering for 
		Planet of Ruins!\nYour empire has been founded, however before you can lead your people you must activate your account.\n Activation Link:".
		"http://www.planetofruins.com/activate?email=".$register_user_data['email']."&code=".$register_user_data['code'].
		"\n\n-Planet of Ruins Admin Team");
	}

	function changePassword($user_id, $password){	
		$user_id = (int)$user_id;
		
		$query = $PDO->prepare("UPDATE `user_data` SET `password` = ? WHERE `id` = ?");
		$query->execute(array($password, $user_id));
	}
	
	function userCount (){
		//Calling scripts: login.mod.php	
		$query = $PDO->prepare("SELECT COUNT(`id`) FROM `user_data` WHERE `active` = 1");
		$query->execute();
		
		$result = (int)$query->fetchColumn();
		
		return $result;
	}
}//END class
$USERS = new users;
//END USERS class


This post has been edited by Darkranger85: 22 February 2013 - 06:13 PM

Was This Post Helpful? 0
  • +
  • -

#8 andrewsw  Icon User is online

  • It's just been revoked!
  • member icon

Reputation: 3809
  • View blog
  • Posts: 13,518
  • Joined: 12-December 12

Re: First foray into OOP: Correct implementation?

Posted 23 February 2013 - 03:07 AM

session_start() must be called before ANY output is sent to the browser; not a DOCTYPE, not even a space.

It sounds like you are including pages more than once. Consider include_once() or require_once().

Yes, if some functions are completely independent then they could be static members. I would consider a single class such as General or Utils to place all my generally available functions. But if you have a lot of them then, of course, you might create more than one class for these, like your General and Security (notice the case-ing). I would ensure that this class only contains static members. It is even possible (sort of..) to make the class itself static - discussed at SO.

Sounds like you need to read about properties.

Hint: It would be useful to look through the classes available to you in PHP, to see how they work and what members they make available.

If you don't want to fetchAll then you could consider fetch() or fetchColumn(). the docs (again)

My config.inc.php file that I include on every page starts like this:

<?php
// Start output buffering:
ob_start();
session_start();

date_default_timezone_set('Europe/London');
// etc..

Was This Post Helpful? 0
  • +
  • -

#9 Darkranger85  Icon User is offline

  • D.I.C Head

Reputation: 1
  • View blog
  • Posts: 148
  • Joined: 31-August 12

Re: First foray into OOP: Correct implementation?

Posted 23 February 2013 - 06:50 AM

I'm looking over my pages but I don't see any instance of calling a session after output.

I call the session in my init file and it's included at the top before anything.

Of course at the moment the biggest thing holding me up is figuring out how to access my pdo file from my classes. Cause it's not giving me any messages at the moment beyond telling me my pdo variable is undefined.

I googled it and it seems like most of them include a new pdo object in every class in the constructor.

And it's not including my connection script. It says that no such directory exists.

I have the name spelled right, and it's one directory back from the calling file so I have '../connection.php'.

This post has been edited by Darkranger85: 23 February 2013 - 07:01 AM

Was This Post Helpful? 0
  • +
  • -

#10 Dormilich  Icon User is offline

  • 痛覚残留
  • member icon

Reputation: 3576
  • View blog
  • Posts: 10,442
  • Joined: 08-June 10

Re: First foray into OOP: Correct implementation?

Posted 23 February 2013 - 06:56 AM

View PostDarkranger85, on 23 February 2013 - 02:50 PM, said:

I googled it and it seems like most of them include a new pdo object in every class in the constructor.

it would be enough, if you pass the PDO object to the constructor.

<?php
class Query implements iQuery
{
    protected $pdo;

    // this is called Dependency Injection
    public function __construct(PDO $db)
    {
        $this->pdo = $db;
    }

    // etc.
}

<?php
$pdo = new PDO();

$query = new Query($pdo);
$query->exec($sql);


Was This Post Helpful? 0
  • +
  • -

#11 Darkranger85  Icon User is offline

  • D.I.C Head

Reputation: 1
  • View blog
  • Posts: 148
  • Joined: 31-August 12

Re: First foray into OOP: Correct implementation?

Posted 23 February 2013 - 07:22 AM

Ok, here is the start of my class

class userActions{
	protected $pdo;
	
	function __CONSTRUCT(PDO $db){
		$this->pdo = $db;
	}



And my connection

$username = 'root';
$password = '';
	
$db = new PDO('mysql:host=localhost;dbname=game_db', $username, $password);



I thought I put together the class like in your example but it doesn't like it.

Argument 1 passed to userActions::__CONSTRUCT() must be an instance of PDO, none given

This post has been edited by Darkranger85: 23 February 2013 - 07:23 AM

Was This Post Helpful? 0
  • +
  • -

#12 Dormilich  Icon User is offline

  • 痛覚残留
  • member icon

Reputation: 3576
  • View blog
  • Posts: 10,442
  • Joined: 08-June 10

Re: First foray into OOP: Correct implementation?

Posted 23 February 2013 - 07:29 AM

how do you instantiate the userActions class?
Was This Post Helpful? 0
  • +
  • -

#13 Darkranger85  Icon User is offline

  • D.I.C Head

Reputation: 1
  • View blog
  • Posts: 148
  • Joined: 31-August 12

Re: First foray into OOP: Correct implementation?

Posted 23 February 2013 - 07:33 AM

$Actions = new userActions;


Was This Post Helpful? 0
  • +
  • -

#14 JackOfAllTrades  Icon User is offline

  • Saucy!
  • member icon

Reputation: 6107
  • View blog
  • Posts: 23,661
  • Joined: 23-August 08

Re: First foray into OOP: Correct implementation?

Posted 23 February 2013 - 11:11 AM

Here
function __CONSTRUCT(PDO $db){


you're making your class's constructor require a PDO object as an argument. You have not passed an argument. This is, of course EXACTLY what the error message is telling you:

Quote

Argument 1 passed to userActions::__CONSTRUCT() must be an instance of PDO, none given


So you really need to learn to read and try to grasp your error messages.

I would change __CONSTRUCT to __construct by the way, as noted in my previous post.

So the answer is pass the $db object you created as an argument.

$db = new PDO('mysql:host=localhost;dbname=game_db', $username, $password);
$actions = new UserActions($db);


or, all in one line (if you're not using the $db object outside of the class)

$actions = new UserActions(new PDO('mysql:host=localhost;dbname=game_db', $username, $password));

Was This Post Helpful? 1
  • +
  • -

#15 Dormilich  Icon User is offline

  • 痛覚残留
  • member icon

Reputation: 3576
  • View blog
  • Posts: 10,442
  • Joined: 08-June 10

Re: First foray into OOP: Correct implementation?

Posted 24 February 2013 - 01:25 AM

additionally, I might quote my use-case example from before:

View PostDormilich, on 23 February 2013 - 02:56 PM, said:

<?php
$pdo = new PDO(…);

$query = new Query($pdo);


This post has been edited by Dormilich: 24 February 2013 - 01:26 AM

Was This Post Helpful? 0
  • +
  • -

  • (8 Pages)
  • +
  • 1
  • 2
  • 3
  • Last »