My Mysql Class - Advice

Not for 'how-to' coding questions but PHP theory instead, this forum is here for those of us who wish to learn about design aspects of programming with PHP.

Moderator: General Moderators

User avatar
Luke
The Ninja Space Mod
Posts: 6424
Joined: Fri Aug 05, 2005 1:53 pm
Location: Paradise, CA

My Mysql Class - Advice

Post by Luke »

Every time somebody posts their mysql interaction class on this forum (and it seems often) the response is... keep it minimal. I am wondering if this is too minimal... am I missing something... missing the point? Please nitpick!

Code: Select all

<?php
class Mysql{
	private $host;
	private $user;
	private $pass;
	private $db;
	private $registry;
	private $config;
	function __construct($registry, $host=false, $user=false, $pass=false, $db=false, $persistant=true){
		
		$this->registry = $registry;
		
		$this->config = $this->registry->get('Config');
		
		$this->error = $this->registry->get('Error');
		//$this->error->set_source(get_class($this));
		
		$this->host = $host ? $host : $this->config->get('host');
		$this->user = $user ? $user : $this->config->get('user');
		$this->pass = $pass ? $pass : $this->config->get('pass');
		$this->db = $db ? $db : $this->config->get('db');
		
		$this->persistant = $persistant;
				
	}
	
	public function connect($select_db=true){
		$function = $this->persistant ? 'mysql_pconnect' : 'mysql_connect';
		
		$function($this->host, $this->user, $this->pass)
			or $this->error->push('Connection failed:\n' . mysql_error());
			
		if($select_db){
			$this->select_db();
		}
	}
	
	public function select_db($db=false){
		if($db) $this->db = $db;
		mysql_select_db($this->db);
	}
	
	public function execute($query){
		
		$this->connect();
		
		if($r = mysql_query($query)){
			return mysql_affected_rows();
		}
		else{
			$this->error->push('SQL Execution Failed:\n' . mysql_error());
		}
		
	}
	
	// TODO: get this working
	public function query($query, $array_type='default'){
		
		$this->connect();
		
		switch($array_type){
			case "MYSQL_ASSOC":
				$set_array_type = MYSQL_ASSOC;
				break;
			case "MYSQL_NUM":
				$set_array_type = MYSQL_NUM;
				break;
			default:
				$set_array_type = MYSQL_BOTH;
		}
		if($r = mysql_query($query)){
			return mysql_fetch_array($r, $set_array_type);
		}
                else{
                    $this->error->push('SQL Query Failed:\n' . mysql_error());
                }
	
	}
}
?>
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Post by Benjamin »

Looks ok. I've found that the database connection code should be in a seperate class, that way you can run more than one instance using the same database connection. The db class I wrote has a lot of bloat in it, but I like it because it saves sooo much time when I do long inserts and selects.
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Post by Benjamin »

If you use mysql_fetch_assoc, you won't have to specify whether you want associative or indexed values as both will be returned..

Code: Select all

public function query($query){
               
                $this->connect();
               
                if($r = mysql_query($query)){
                        return mysql_fetch_assoc($r);
                }
                else{
                    $this->error->push('SQL Query Failed:\n' . mysql_error());
                }
        }
That function doesn't make much sense though, because you'll have to query the db for every row..

You could just return every row in an array..

Code: Select all

<?php
        public function query($query){
               
                $this->connect();
               
                if($r = mysql_query($query)){
                    $rCount = mysql_num_rows($r);
                    $records = array();
                    
                    if ($rCount > 0)
                    {
                        for ($i = 0; $i < $rCount; $i++;)
                        {
                            $records[$i] = mysql_fetch_assoc($r);
                        }
                        
                        return $records;
                    } else {
                        return false;
                    }
                } else {
                    $this->error->push('SQL Query Failed:\n' . mysql_error());
                }
        }
?>
User avatar
Luke
The Ninja Space Mod
Posts: 6424
Joined: Fri Aug 05, 2005 1:53 pm
Location: Paradise, CA

Post by Luke »

:oops: thanks... I hadn't even tested out that query method before I posted it... and I like the idea of using a seperate class for connection... I am working on that now. Any suggestions for methods in the class other than the ones there though? (I am going to add a close method, but anything else?)
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Post by Benjamin »

The Ninja Space Goat wrote::Any suggestions for methods in the class other than the ones there though?
I'm probably the wrong person to ask, I would load the thing up lol..

viewtopic.php?t=51623
User avatar
Jenk
DevNet Master
Posts: 3587
Joined: Mon Sep 19, 2005 6:24 am
Location: London

Post by Jenk »

My *sql classes contain an iterator for the results (results are stored in an array - which I use an object for as well, but essentially it's just the same as normal array as per below):

Code: Select all

public function fetchRow()
{
    if (isset($this->result[$this->index])) {
        return $this->result[$this->index++];
    } else {
        $this->index = 0;
        return false;
    }
}
I've also exapanded on that in the past to use optional arg $index to specificy which row to return. Can also store multiple results if need be too.

The above adds the ability to while() loop the results just as if using mysql_fetch_*:

Code: Select all

while ($row = $db->fetchRow()) {
    //do something.
}
User avatar
pickle
Briney Mod
Posts: 6445
Joined: Mon Jan 19, 2004 6:11 pm
Location: 53.01N x 112.48W
Contact:

Post by pickle »

I'd say get rid of the connect() and select_db() functions.

You're never going to be instantiating this object without wanting to connect to the database, so why not put that in the constructor?
The same applies to the select_db() function. You're passing along the DB to the constructor, why not just connect & select the DB there?
Real programmers don't comment their code. If it was hard to write, it should be hard to understand.
User avatar
jayshields
DevNet Resident
Posts: 1912
Joined: Mon Aug 22, 2005 12:11 pm
Location: Leeds/Manchester, England

Post by jayshields »

Put your \n's in double quotes!! :P
User avatar
Luke
The Ninja Space Mod
Posts: 6424
Joined: Fri Aug 05, 2005 1:53 pm
Location: Paradise, CA

Post by Luke »

jayshields wrote:Put your \n's in double quotes!! :P
d'oh! Thanks.
User avatar
Chris Corbyn
Breakbeat Nuttzer
Posts: 13098
Joined: Wed Mar 24, 2004 7:57 am
Location: Melbourne, Australia

Post by Chris Corbyn »

astions wrote:Looks ok. I've found that the database connection code should be in a seperate class, that way you can run more than one instance using the same database connection. The db class I wrote has a lot of bloat in it, but I like it because it saves sooo much time when I do long inserts and selects.
I think that may be a bit of an overkill. Perhaps provide a method of fetching a singleton instead.
User avatar
daedalus__
DevNet Resident
Posts: 1925
Joined: Thu Feb 09, 2006 4:52 pm

Post by daedalus__ »

i just make my db class return resources.
User avatar
Luke
The Ninja Space Mod
Posts: 6424
Joined: Fri Aug 05, 2005 1:53 pm
Location: Paradise, CA

Post by Luke »

this class is going to be used strictly by other (more specific) classes... so the point of it is to:

1. Connect to the database (but only if a query is ran... not auto-connect, which is why mysql_connect isn't in the constructor)
2. Run (select) queries and return the results in a neat format
3. Execute (update, delete, etc.) queries and return whether or not it was successful

That's basically it. I don't want it more complicated than that.
User avatar
John Cartwright
Site Admin
Posts: 11470
Joined: Tue Dec 23, 2003 2:10 am
Location: Toronto
Contact:

Post by John Cartwright »

The Ninja Space Goat wrote:this class is going to be used strictly by other (more specific) classes... so the point of it is to:

1. Connect to the database (but only if a query is ran... not auto-connect, which is why mysql_connect isn't in the constructor)
2. Run (select) queries and return the results in a neat format
3. Execute (update, delete, etc.) queries and return whether or not it was successful

That's basically it. I don't want it more complicated than that.
Why not establish the connection once, and use d11's suggestion of adding a method to return the instance of the class?
User avatar
Luke
The Ninja Space Mod
Posts: 6424
Joined: Fri Aug 05, 2005 1:53 pm
Location: Paradise, CA

Post by Luke »

I have tried to create a singleton before and failed (I am doing something wrong, and I Can't post my code right now because it's at home). I like the idea though... I will work on doing it that way. Then I need to somehow get my registry to accept singletons.
User avatar
Chris Corbyn
Breakbeat Nuttzer
Posts: 13098
Joined: Wed Mar 24, 2004 7:57 am
Location: Melbourne, Australia

Post by Chris Corbyn »

The Ninja Space Goat wrote:I have tried to create a singleton before and failed (I am doing something wrong, and I Can't post my code right now because it's at home). I like the idea though... I will work on doing it that way. Then I need to somehow get my registry to accept singletons.
Your registry shouldn't care if the object is a singleton.... it's still an object. Then again, I'm not sure I've seen how you've written your registry. Is it a factory as well as a registry or something?

As for allowing a singleton to be fetched it's just a case of adding a static property to the class and a static method to return that. Think of static properties as encapsulated globals (which is why some people don't like them!) in this scenario.

Code: Select all

class MySQL
{
    static private $instance = null;

/* snippety snip... */

    static public function getInstance($registry, $host=false, $user=false, $pass=false, $db=false, $persistant=true)
    {
        if (self::$instance === null)
        {
            self::$instance = new MySQL($registry, $host, $user, $pass, $db, $persistant);
        }
        return self::$instance;
    }
Then you can either use your class like it is now or you can use a singleton instance by doing this rather than instantiating it:

Code: Select all

$db = MySQL::getInstance($reg, 'localhost');
Post Reply