current community

your communities

more stack exchange communities

Stack Exchange
tour help
careers 2.0
Take the 2-minute tour ×
Stack Overflow is a question and answer site for professional and enthusiast programmers. It's 100% free, no registration required.
up vote 1 down vote favorite

My code is as follows:

class Database  
{  
    private $db_host;  
    private $db_user;  
    private $db_pass;  
    private $db_name;  
    private $con;

    public function __construct() {
        $this->db_host = "localhost";  
        $this->db_user = "admin";  
        $this->db_pass = 'password';  
        $this->db_name = 'test';    
        $this->con = '';
    }

    public function connect() {
        $db_name = "test";    
        $this->con = mysql_connect($this->db_host, $this->db_user, $this->db_pass);
    }  

    public function select(){
        $q = "SELECT name, city FROM customers;";
        mysql_select_db($this->db_name, $this->con);
        $result = mysql_query($q);
        return mysql_fetch_assoc($result);
    }  
} 


$db = new Database();
$db->connect();
$tempArray = Array();
$rs = $db->select('customers', 'name, suburb');
foreach ($rs as $row)
{
    echo $rs['name'] . "<br>";
}

And my table's data is

name | city
--------------
Anne | Sydney
Jane | London

The actual output is:

Anne 
Anne

The desired output is:

Anne
Jane

Can someone tell me what I am doing wrong. It seems like I have missed something basic. I have read over 50 articles and nothing seems to explain what I am doing wrong.

Note: This is a scaled down version of my code. I intend to use this to make a more general object that pulls information from my database.

Thanks,

Brett

share|improve this question
    
just to let you know, your "select" function makes no sense. database class have to be used to run custom queries, not hardcoded ones. –  Your Common Sense Mar 21 '12 at 4:12
    
@YourCommonSense, I'm sure he just put that line in to minimize the possibility a mistake in the parameters, just for debugging, right? -- Clearly planning on changing it to work sensibly after he figured out what the problem was. –  Ben Lee Mar 21 '12 at 4:28
    
@Ben Oh. I am not that good in telepathy :) –  Your Common Sense Mar 21 '12 at 4:30
    
@YourCommonSense, just common sense, eh? ;) –  Ben Lee Mar 21 '12 at 4:31
    
But in all seriousness, I can't bring myself to believe the OP would miss something of that magnitude. Seems like a case of debugging to me. –  Ben Lee Mar 21 '12 at 4:32

5 Answers 5

up vote 1 down vote accepted

You need to call mysql_fetch_assoc for each row. It only returns one row of data, not the full set. For example, you could move it out into the loop:

class Database
{  
    /* ... */

    public function select(){
        $q = "SELECT name, city FROM customers;";
        mysql_select_db($this->db_name, $this->con);
        return mysql_query($q);
        /* Remove your line here, returning the query result, not the first row */
    }  
} 

$db = new Database();
$db->connect();
$tempArray = Array();
$result = $db->select('customers', 'name, suburb');
/* Note that I'm now using mysql_fetch_assoc to get each row from the result */
while ($row = mysql_fetch_assoc($result));
    echo $row['name'] . "<br>";
}

You can use a while loop there because after the last row has been retrieved, mysql_fetch_assoc will return FALSE and exit the loop.

share|improve this answer
    
What's wrong with this answer? Why the downvote? –  Ben Lee Mar 21 '12 at 4:25
    
Thanks Ben, that worked a charm! Re @Sudhir and other helps. I am sorry but I won't be trying your solutions because this is elegant and it works. Thanks to everyone. :-) –  Brett Mar 21 '12 at 4:33
1  
I don't entirely get the down-vote, but even your suggested change doesn't reflect the fact that the select() function is being called differently than it's defined, which is one reason the OP doesn't get the answer he expects. –  Tieson T. Mar 21 '12 at 4:34
    
@Brett it is not elegant by any means, because it's just a replica of your ugly code. But I do understand a desire to have not the good code but the code that at least works. Good luck :) –  Your Common Sense Mar 21 '12 at 4:36
1  
My code was odd because I wanted to keep it simple so people would want to solve it. The real code was about five times as long. –  Brett Mar 21 '12 at 5:05
up vote 0 down vote

In this section of your code:

return mysql_fetch_assoc($result);

You are merely returning the first row. I suggest you to create an array.

public function select(){
    $q = "SELECT name, city FROM customers;";
    mysql_select_db($this->db_name, $this->con);
    $result = mysql_query($q);
    $toReturn = array();
    while($row = mysql_fetch_assoc($result) )
        $toReturn[] = $row;
    return $toReturn;
}
share|improve this answer
1  
I wouldn't recommend doing it this way. Why load the entire result set into memory at once? In many cases, that's insane. –  Ben Lee Mar 21 '12 at 4:26
    
(Having an option to load the entire result set like in @YourCommonSense's answer is okay, so you can use it in places you know it will be okay this instance; but don't make it the only option). –  Ben Lee Mar 21 '12 at 4:33
up vote 0 down vote

Should'nt it be :


foreach ($rs as $row)
{
    echo $row['name'] . "<br>";
}

And:


$resArr = array();
while($res = mysql_fetch_assoc($result)) {
  $resArr[] = $res;
}
return $resArr;
share|improve this answer
up vote 0 down vote

A sane version of your class. It lacks vital parts of course, to be used in the real life, but just out of your sketch:

class Database  
{  
    private $con;

    public function __construct() {
        $this->con = mysql_connect("localhost", "admin", 'password');
        mysql_select_db("test",$this->con);
    }
    public function query($sql,$this->con){
        return mysql_query($sql);
    }  
    public function get_all($sql,$this->con){
        $ret = array();
        $res = mysql_query($sql);
        while  ($row = mysql_fetch_array($row)) {
          $ret[] = $row;
        }
        return $ret;
    }  
} 

$db = new Database();
$rs = $db->get_all("SELECT name, city FROM customers");
foreach ($rs as $row)
{
    echo $rs['name'] . "<br>";
}
share|improve this answer
up vote 0 down vote

Seems kind of odd that no one has picked up on a bit of a gaff in the code here.

You define select() like so:

public function select(){
    $q = "SELECT name, city FROM customers;";
    mysql_select_db($this->db_name, $this->con);
    $result = mysql_query($q);
    return mysql_fetch_assoc($result);
}

But, then you call it like this:

$rs = $db->select('customers', 'name, suburb');

I assume your intention was to be able to specify the table and the fields to select from the database. If it was, your select function should look more like this:

public function select($table, $fields){
    $q = "SELECT $fields FROM $table;";
    mysql_select_db($this->db_name, $this->con);

    return mysql_query($q);
}

From there you would follow @BenLee's example in his answer, since you need to iterate over a resultset. Each field becomes a key in an associative array.

I wouldn't recommend actually doing string insertion like this in production code, but I think it's closer to what you intended.

HTH.

share|improve this answer
    
I'd +1 this answer if such a method was viable in the real life. but selecting whole table contents hardly can be used in the environment other than educational... –  Your Common Sense Mar 21 '12 at 4:47
    
Heh. I also wouldn't ever use the mysql library, but everyone seems to refuse to stop, so what can you do? –  Tieson T. Mar 21 '12 at 4:49
    
that's another matter. there are no reasons to stop other than some laziness of the some PHP team. –  Your Common Sense Mar 21 '12 at 4:52

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.