Quick MySql Database class.

[amPAR]

I use this to quickly run mysql functions needed in my scripts. I figured before I start a new script using this class I would see if there is anything I should change or improve. Any ideas?

class DB_sql
  {

//You'll never believe what this does
function connect()
{
  global $dbname, $servername, $dbusername, $dbpassword;

  //connect to database
  $this->id_link = @mysql_connect($servername, $dbusername, $dbpassword) or die;
  mysql_select_db($dbname, $this->id_link);
  //check connection
  if (!$this->id_link) {
      $this->stop('Failed to connect to Mysql Database');
      }
}

function query($string, $fetch = '0')
{
  // run query, get id
  $qid = mysql_query($string, $this->id_link);

  //go ahead and retrieve data?
  if ($fetch == 0) { return $qid; } elseif ($fetch == 1){
      return $this->fetch($qid);}
      else { return $this->fetch_array($qid);    }
}

function fetch_array($qid) {
    while($row = mysql_fetch_array($qid)){
foreach( $row AS $key => $val ){
    $record[$key] = stripslashes( $val );
    }
  }
  return $record;
}


function fetch($qid)
{
  //I found this piece difficult to write
  return @mysql_fetch_object($qid);
}

function num_rows($qid)
{
  //Line up soldiers, we need a headcount
  return @mysql_num_rows($qid);
}

function close()
{
  //You're not useful to us anymore, good night
  return mysql_close($this->id_link);
}

function stop($statement)
{
  echo $statement;
}

[thorpe]

one thing i noticed straight away is the use of global vars. you would be much better off setting these up as properties and instantiating them inside a construct. eg;

class DB_sql {
  var $dbname
  var $servername
  var $dbusername
  var $dbpassword;
  function DB_sql($dbname, $servername, $dbusername, $dbpassword) {
    $this->dbname = $dbname;
    $this->servername = $servername;
    $this->dbusername = $dbusername;
    $this->dbpassword = $dbpassword;
}

also, im not sure if this is just a typo, but shouldn't this...

function query($string, $fetch = '0')

be...

function query($string, $fetch = 0)

?

you might also want to atleast consider some error checking. your fetch_array() method for instance never even checks to see if the query was succesfull or not.

[Shrike]

I think the class could be simplified.

First of all your Db_sql::query() method doesn't just query, but fetches too. To keep things simple have the query method just do queries and move the fetching to another method.

Also having this class traverse a recordset (Db_sql::fetch_array() ) seems like unnecessary duplication. The client can already traverse the recordset with a combination of query(), num_rows() and fetch().

Why is there a method for fetching objects one a time but not one for fetching arrays?

[amPAR]

Thorpe, thanks for the stuff you pointed out there. All of that makes a lot of sense thanks.

Shrike, There is one for arrays and...
I kind of like using query for fetching too. If I don't want it to fetch I just don't add the 1 or 2 to the end. That way it just runs the query. If I need it to bring back data I just add a number depending on whether or not I need one piece of data or an array of it. That in itself works pretty well I've tested it. However, if there is a practical reason that this could mess with my code let me know.

    function fetch_array($qid) {
        while($row = mysql_fetch_array($qid)){
    foreach( $row AS $key => $val ){
        $record[$key] = stripslashes( $val );
        }
      }
      return $record;
    } 

[thorpe]

if there is a practical reason that this could mess with my code let me know

my main concern with the said method is that your creating an extra loop. it loops to create they array itself, and then your client code would need to loop again to display the results. its just unwanted overhead for no real benifit.

[Shrike]

thorpe wrote:

and then your client code would need to loop again to display the results. its just unwanted overhead for no real benifit.

Exactly.

Also if you elect to return a resource from the query() method how do you then call the fetch_array() / fetch_assoc() method from within your class. It's limited to *_fetch_object().

One more thing that's struck me - using stripslashes() on the data should be the responsibility of the client code, after all the class doesn't know what the data is intended for. In any case does the data actually need slashes removing? I would use addslashes() (or preferably *_real_escape_string() ) on incoming data - there shouldn't then be a need to remove slashes when pulling the data back.

[amPAR]

Okay guys I see what your saying.. I fixed everything you said... I think.

I'm having a new problem now.

First of all thorpe.

What's bad about globals? I've used them quite a bit in past scripts, of course, I'm still learning so nothing major.

Anyway here's how the code looks now

class DB_sql
  {
  var $dbname
  var $servername
  var $dbusername
  var $dbpassword;

  function DB_sql($dbname, $servername, $dbusername, $dbpassword) {
    $this->dbname = $dbname;
    $this->servername = $servername;
    $this->dbusername = $dbusername;
    $this->dbpassword = $dbpassword;
}
    //You'll never believe what this does
    function connect()
    {
      // global $dbname, $servername, $dbusername, $dbpassword;

  //connect to database
  $this->id_link = @mysql_connect($this->servername, $this->dbusername, $this->dbpassword);
  //check connection
  if (!$this->id_link) {
      $this->stop('Failed to connect to Mysql Database');
  }
  //pick database
  if (!mysql_select_db($this->dbname, $this->id_link))
        {$this->stop('DBconnection failed');
  }
}

function query($string)
{
  // run query, get id
  return mysql_query($string, $this->id_link);

   }

function fetch_arr($string)
{
    $qid = mysql_query($string, $this->id_link);
    while($row = mysql_fetch_array($qid)){
            foreach( $row AS $key => $val ){
            $record[$key] = $val;
            }
    }
  return $record;
}


function fetch_obj($string)
{
  $qid = mysql_query($string, $this->id_link);
  return @mysql_fetch_object($qid);
}

function num_rows($qid)
{
  //Line up soldiers, we need a haircut I mean headcount
  return @mysql_num_rows($qid);
}

function close()
{
  //Your not useful to us anymore, good night
  return mysql_close($this->id_link);
}

function stop($statement)
{
  echo $statement;
}


}

Alright... Now when I run it...

$DB->connect();
$record = $DB->fetch_arr("SELECT * FROM `users` WHERE id = 1");

for ($i = 0; $i <= count($record); $i++) {
   echo $i.") ";
   echo $record[$i];
   echo "<br>------<br>";
}

I get this...

0) 1

1) Par

2) Jonathan

3) Parsons

4) 14f91873bc0cbecf98c0ba7606b6c6b7

5) 4

6) [e-mail]

7) http://amp3d.net

8) ParXBuster

9) 67899177

10) EW

11) none

12) 1985-12-12

13) ../images/user/Par_sym.gif

14) ../images/user/Par_char.jpg

15) 18

16) 9

17) 1986

18) USA

19) Abstract Artist

20) None. at all

asdf

k

21) Administrator : Founder

22)

23)

24)

25)

26)

27)

28)

29)

30)

31)

32)

33)

34)

35)

36)

37)

38)

39)

40)

41)

42)

43)

44)

This is just from an old Database I had that I used to test a userscript. Is it still looping as you guys said and therefore adding all this extra information in the array or am I doing something else wrong? And if it's still looping... Why?

I'm lost on this.

[Weedpacket]

thorpe wrote:

my main concern with the said method is that your creating an extra loop. it loops to create they array itself, and then your client code would need to loop again to display the results. its just unwanted overhead for no real benifit.

If that's a problem you could have a method that takes a callback function to be run on each row as it's fetched. But there is one benefit in fetching the entire resultset into an array once and that's if you're going to be traversing it more than once; otherwise you're either going to have to re-fetch the result set again (very wasteful), or stick calls to mysql_data_seek() in whenever you want to reuse it (which can get in the way if you want to use the same code on something that didn't come from a MySQL db).

But the loop itself in that code could be eliminated:

function fetch_array($qid) {
  $recordset = array();
        while($row = mysql_fetch_array($qid)){
        $recordset[] = array_map('stripslashes', $row);
      }
      return $recordset;
    } 

amPAR wrote:

If I don't want it to fetch I just don't add the 1 or 2 to the end

Hey, it's good of you to [man]define[/man] those for me, or I'd never have known what the numbers meant 😃 But I think thorpe was asking why you had the 0 in quotes.

amPAR wrote:

What's bad about globals?

Well in this instance, for one thing it means you'll only be able to have precisely one database object. Which means if you want to connect to two databases, you can't have both at once. They're also an indication that your code isn't properly self-contained: do they really need to be visible for the whole world to mess with as it sees fit? If not, why are they? If they are, how do you prevent one part of the program overwriting the contents of a global variable that another part of the program is relying on to remain unchanged? Does the function need them at all? If not, why are they there? If they are, why can't you pass them explicitly as part of the function call. Then at least there'll be no argument about what information the function needs in order to operate. If a function uses a global variable, what will be the value of that variable when you call the function? If a global variable turns out to have the wrong value, how will you go about finding where it got that value from?

Anyway, try Googling for "Global variables considered harmful".

[amPAR]

Thanks a lot weedpacket. I haven't tried the code you gave me but I'm sure it will work. Any more problems I'll be sure to post them but thanks a lot guys for your help.

[Plasma]

Not sure if it was already mentioned, but the use of global variables in the class defeats one of the features of objects (being able to have an object own properties / be self contained).

For example, now you've fixed up the globals you can use two database objects of yours at the same time (with different variables), previously they would crosslink between one another.