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
5  
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
1  
@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

1 Answer 1

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
    
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
    
include('DatabaseConnection.php'); and then the lines of code from the section usage. For simplicity you can place the resultTable-function in the view-file. Otherwise place it in a separate file and include that file as wel with an include(). Then simply call the function with the SQL-result. –  Abbas Mar 2 '13 at 0:26
    
echo resultTable($result); $result doesnt have any values. am i correct? –  lexter Mar 4 '13 at 3:09

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.