Avatar

We use this class in 1000's of places. Its the most reused code on the php pages on my site. I'm considering making a ruby version as well. But I'm pretty sure this class could use some work. Also, for some reasons transactions aren't working using the Command method.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
class DB {
        /*
        $c=new DB();


        */
	var $properties=array(
			'die_on_errors'=>true,
			'complain_about_errors'=>false
		);
        var $database='web2_db1';
        var $user=/*woops*/;
        var $password=/*woops*/;
        var $connected=false;
        var $connection="";
        var $count=-1;
        var $id=-1;
        var $result=false;
        var $error;
        var $results=false;
        var $last_ql;
        function __construct($db='web2_db1') {
                        $this->database=$db;
			$this->connect();
        }
	function connect() {
		$this->connection=mysql_connect('localhost',$this->user,$this->password);
		if (!$this->connection) {
			die("Cannot connect to the database!");
		}
		$select=mysql_select_db($this->database);
		if (!$select) {
			die("Could not read $deatabase database");
		}
		$this->connected=true;
	}
	function __destruct() {
		@mysql_close($this->connection);
	}
	function SetProperty($property,$value) {
		$this->properties[$property]=$value;
	}
        function Query($ql,$args=false,$json=false) {
                if ($args)
                        $this->last_ql=str_replace(array_keys($args),array_values($args),$ql);
                else
                        $this->last_ql=$ql;
               if (!mysql_ping($this->connection)) {
                        $this->connect();
                }

		try {
	                $this->result=mysql_query($this->last_ql,$this->connection);
		} catch (Exception $e) {
			$this->connect();
			return $this->Query($ql,$args,$json);
		}
                if (!$this->result) {
                        $this->error=mysql_error();
			if ($this->properties['die_on_errors'])
				die(var_dump(array("error"=>$this->error,"last_ql"=>$last_ql)));
			if ($this->properties['complain_about_errors'])
				throw new Exception($this->error." IN SQL ".$this->last_ql);
			
                        return false;
                }
                $this->results=array();
                $i=0;
                try {
                while ($row=mysql_fetch_assoc($this->result)) {
                        $this->results[$i]=array();
                        foreach ($row as $key=>$value) {
                                $this->results[$i][$key]=$value;
                        }
                        $i++;
                }
                } catch (Exception $e) {
                        var_dump($e);
                        var_dump(debug_backtrace());
                        die(var_dump($this->result));
                }

                return ((!$json)?$this->results:Utilities::JSENC($this->results));
        }
        function Command($ql,$args=false,$json=false) {
                if ($args) {
                        $this->last_ql=str_replace(array_keys($args),array_values($args),$ql);

                 }else
                        $this->last_ql=$ql;
		if (!mysql_ping($this->connection)) {
			$this->connect();
		}	
                try {
                        $this->result=mysql_query($this->last_ql,$this->connection);
                } catch (Exception $e) {
                        $this->connect();
                        return $this->Query($ql,$args,$json);
                }

                if (substr($ql,0,2)=='IN'&&$this->result)
                        $this->id=mysql_insert_id($this->connection);
                if (!$this->result) {
                        $this->error=mysql_error();
                        $this->results=false;
                } else {
                        $this->result=true;
                        $this->results=true;
                }
                return ((!$json)?$this->results:Utilities::JSENC($this->results));
        }

}
}

Refactorings

No refactoring yet !

02f0d41896b1c37c537c33e77aa31a7b

twar59

July 9, 2008, July 09, 2008 19:26, permalink

No rating. Login to rate!

Some pretty good coders have already refactored this, I recommend looking at PDO, propel and doctrine (PHP), and Active Record on the Ruby side. These tools may look a little over the top at first, but once you get the hang of it, you will be really happy.

F288a8afe5302a16a366d5e9d34f2fec

Joe Grossberg

July 9, 2008, July 09, 2008 22:03, permalink

1 rating. Login to rate!

"We use this class in 1000's of places"

Then you're probably not reusing code properly.

In any case, if you have that many apps that depend on this file, I would do the following:
* add a comment to the top, indicating the version number
* write a lot of tests before you change anything, to make sure that any improvements don't break legacy code

Avatar

Amenthes

July 10, 2008, July 10, 2008 09:15, permalink

No rating. Login to rate!

I have two questions:

1. Why do you have try/catch blocks in your code?
2. Why is there no (at least) mysql_real_escape_string() function?

6dc0e9a07bcff97ac9b111f36e12f1f6

Ishkur

July 11, 2008, July 11, 2008 00:46, permalink

No rating. Login to rate!

I've got a sourceforge project dealing with this same sort of thing. And from my experience in coding that I might be able to offer a few tips on how to make it better.

1.)Since you're clearly using PHP5 for this, why not go ahead and add access control to the global vars and methods. You dont want just anyone to be able to call out your data anywhere they want. you need a defined entry point and a defined access point.

2.) going along with that PHP5 thing I was talking about, you can also use the __construct() method to define some of your global vars like the username, the password, the host, and the database name. Doing that will save you a lot of coding work and processing time later.

3.) A lot of your global vars dont look like they need to be initiated as a global var. try to make them as narrow in scope as possible.

4.) and just a little (and by little, i mean little) advice on making your loops faster. ++$i is faster than $i++.

For reference on another way of going about this task, you can look at my DB class: http://jsandlin.org/null_dbw/source.php

Your refactoring





Format Copy from initial code

or Cancel