Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

i just wanted to confirm if my codes below is correct. Feel free to make corrections.

<?php
class createConnection
{
var $host="localhost";
var $username="root";
var $password="";
var $database="wilbert";
var $myconn;



function connectToDatabase()
{
$conn = mysql_connect($his->host,$this->username,$this->password);
if (!$conn)
{
    die ("Cannot connect to the database");
}

else
{
    $this->myconn = $conn;
    echo "Connection established!";
}
return $this->myconn;

}
function selectDatabase()
{
mysql_select_db($this->database);
if(mysql_error())
{

    echo "Cannot find the database ".$this->database;
}
else
{
echo "Database selected..";
}

}
function selectData()
{
mysql_select_db($this->database);

$result = mysql_query(" SELECT a.student_id, a.FirstName, b.First_Name FROM Student a        LEFT JOIN teacher b ON a.student_id = b.student_id");

echo "<table border='1'>
<tr>
<th>Student_ID</th>
<th>First_Name</th>
<th>First_Name</th>
</tr>";

while($row = mysql_fetch_array($result))
  {
  echo "<tr>";
  echo "<td>" . $row['student_id']. "</td>";
  echo "<td>" . $row['FirstName'] . "</td>";
  echo "<td>" . $row['First_Name'] . "</td>";
  //echo "<td>" . $row['student_id'] . "</td>";
  echo "</tr>";
  }
echo "</table>";
}

function closeConnection()
{
mysql_close($this->myconn);
echo "connection closed!";
}

}
?>

and here is my object.

<?php

include ('phpclass.php');

$connection = new createConnection(); //i created a new object

$connection->connectToDatabase(); // connected to the database

echo "<br />"; // putting a html break

$connection->selectDatabase();

echo "<br />";

$connection->selectData();
//$connection->closeConnection();// closed connection
?>
share|improve this question
4  
I'd probably avoid using a deprecated library (i.e. mysql_*) too. –  BenM Feb 27 '13 at 18:20
    
Either you should have cut and pasted a lot more carefully or shown us the error which occurred when you ran this ( mysql_connect($his->host,$t... ) –  symcbean Mar 5 '13 at 21:09
    
@symcbean - the above codes, my original codes are working but when I try the answers below, it doesnt work anymore :( –  lexter Mar 6 '13 at 0:04
    
@lexter: really? What is $his ? –  symcbean Mar 6 '13 at 9:30
    
@symcbean - it's a typo.. it must be $this –  lexter Mar 7 '13 at 3:45
add comment

2 Answers

up vote 7 down vote accepted

I am a fan of loose coupling so I would suggest that you take the values of host, username, password and database out of your class. Pass the values through the constructor and set them in the constructor. This makes it easier to reuse your code.

Second, in classes such as one that creates a connection to a database I would not "echo" stuff. You better throw exceptions on error or return meaningful values to the caller.

Last, I'd suggest other names. This might be nitpicking but this has more meaning to me:

$connection = new DatabaseConnection();
$connection->open();
$connection->connectDatabase();

So, wrapped up I would have something like:

<?php
class DatabaseConnection
{
    var $host;
    var $username;
    var $password;
    var $database;
    var $myconn;

    function __construct($dbhost, $user, $pass, $db)
    {
        $this->host = $dbhost;
        $this->username = $user;
        $this->password = $pass;
        $this->database = $db;
    }

    function open()
    {
        $conn = mysql_connect($his->host,$this->username,$this->password);
        if (!$conn)
            throw new Exception('Error establishing a connection.');
        else
            $this->myconn = $conn;

        return $this->myconn;
    }

    function connectDatabase()
    {
        mysql_select_db($this->database);
        if(mysql_error())
            throw new Exception("Cannot find the database " . $this->database);
    }

    function selectData($query)
    {
        mysql_select_db($this->database);
        $result = mysql_query($query);
        return $result;
    }

    function closeConnection()
    {
        mysql_close($this->myconn);
    }
}
?>

Usage:

$connection = new DatabaseConnection("localhost", "root", "", "wilbert");
$connection->open();
$connection->connectDatabase();
$connection->selectData("SELECT a.student_id, a.FirstName, b.First_Name FROM Student a LEFT JOIN teacher b ON a.student_id = b.student_id");

See how the other names make more sense? Try to avoid verbs as a classname. Methods can and most likely will be or contain verbs, but not classnames. Also, leaving the query out of the class leaves you the option to use the same code for other queries.

Note that this is no perfect code, I was only suggesting what might be changed. Hope this helps! :)

Edit:

You call the function selectData() and pass the query-string as a parameter. This changes nothing, it gives the same result as your code. Only in my case you can pass other queries to the function whereas your function is limited to the embedded query.

Now for the result. This has also to do with the decoupling of your connection-class and what will be shown to the user. I showed u the usage of the class, the code below is how you can process the result. Create a function (but not in the connection class!) that returns the result in a table:

function resultTable($sqlResult)
{
    $table = "<table border='1'>";
    $table .= "<tr><th>Student_ID</th><th>First_Name</th><th>First_Name</th></tr>";

    while($row = mysql_fetch_array($sqlResult))
    {
        $table .= "<tr><td>" . $row['student_id']. "</td>";
        $table .= "<td>" . $row['FirstName'] . "</td>";
        $table .= "<td>" . $row['First_Name'] . "</td></tr>";
    }
    $table .= "</table>";

    return $table;
}

And the usage:

$connection = new DatabaseConnection("localhost", "root", "", "wilbert");
$connection->open();
$connection->connectDatabase();
$result = $connection->selectData("SELECT a.student_id, a.FirstName, b.First_Name FROM Student a LEFT JOIN teacher b ON a.student_id = b.student_id");
echo resultTable($result);

You could create a new class in which you define all sorts of methods like these to process a result and return it to the view-file where you will show it on the screen. Hope this clarifies it. :)

share|improve this answer
    
thank you! I learned a lot from your comments. thanks so much :) –  lexter Feb 27 '13 at 8:52
    
Glad I could be of help! :) –  Abbas Feb 27 '13 at 9:39
    
in the part of function selectData($query) { mysql_select_db($this->database); $result = mysql_query($query); return $result; } where will the $query get its values? is it possible to show the result in a table, because in my codes above. the results of the LEFT JOIN were embedded in an html table :) –  lexter Feb 28 '13 at 1:22
    
Check my edited answer! ;) –  Abbas Feb 28 '13 at 8:10
    
how will the class and object be connected in your codes?where is the "include();" line? –  lexter Mar 1 '13 at 8:03
show 4 more comments

here is my code update for my class

<?php
class DatabaseConnection
{
    var $host;
    var $username;
    var $password;
    var $database;
    var $myconn;

    function __construct($dbhost, $user, $pass, $db)
    {
        $this->host = $dbhost;
        $this->username = $user;
        $this->password = $pass;
        $this->database = $db;
    }

    function open()
    {
        $conn = mysql_connect($his->host,$this->username,$this->password);
        if (!$conn)
            throw new Exception('Error establishing a connection.');
        else
            $this->myconn = $conn;

        return $this->myconn;
    }

    function connectDatabase()
    {
        mysql_select_db($this->database, $this->myconn);
        if(mysql_error())
            throw new Exception("Cannot find the database " . $this->database);
    }

    function selectData($query)
    {
        mysql_select_db($this->database);
        $result = mysql_query($query);
        return $result;
    }

    function resultTable($sqlResult)
    {
        $table = "<table border='1'>";
        $table = "<tr><th>Student_ID</th><th>First_Name</th><th>First_Name</th></tr>";

        while($row = mysql_fetch_array($sqlResult))
        {
            $table .= "<tr><td>" . $row['student_id']. "</td>";
            $table .= "<td>" . $row['FirstName'] . "</td>";
            $table .= "<td>" . $row['First_Name'] . "</td></tr>";
        }

        $table .= "</table>";
        return $table
    }

    function closeConnection()
    {
        mysql_close($this->myconn);
    }
}
?>

And for my object:

<?php
include ('class.php');
$connection = new DatabaseConnection("localhost", "root", "", "wilbert");
$connection->open();
$connection->connectDatabase();
$result = $connection->selectData("SELECT a.student_id, a.FirstName, b.First_Name FROM    Student a LEFT JOIN teacher b ON a.student_id = b.student_id");
echo resultTable($result);
?>

Here is the error message:

 Parse error: syntax error, unexpected '}' in C:\wamp\www\class.php on line 57
 Call Stack
 #  Time    Memory  Function    Location
 1  0.0008  367112  {main}( )   ..\object1.php:0
share|improve this answer
    
@abbas - kindly check this –  lexter Mar 4 '13 at 8:54
    
Did you try: $connection = new DatabaseConnection("localhost", "root", "", "wilbert"); and in the function connectDatabase try adding the connection as a parameter: mysql_select_db($this->database, $this->myconn); Do you get any error messages? –  Abbas Mar 4 '13 at 9:38
    
@Abbas - everytime i run my php file in the browser, I get this error message. error: syntax error, unexpected '}' in C:\wamp\www\class.php on line 57 Call Stack # Time Memory Function Location 1 0.0008 367112 {main}( ) ..\object1.php:0 –  lexter Mar 5 '13 at 0:16
    
Which line is 57? Formatting your code with correct indentation would make it a lot easier to read and help you. –  David Harkness Mar 5 '13 at 1:36
    
@DavidHarkness - ahmm... Line 57 is only a } –  lexter Mar 5 '13 at 2:00
show 7 more comments

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.