Any Tips?

[BrandonTheG]

Hello,

I am coding a signature chat script I will be distributing free over the internet for people to run on there own servers.

Also trying to consolidate all the functions into one main, nicely commented file. I just wanted to know if I am overcommenting or commenting enough, as I want a nice script people can edit themselves, so outlining what everybody does is key too me.


<?php

/**
* Main class for SigShout Website
* License: http://creativecommons.org/licenses/by-sa/3.0/us/
*/

if (!defined("SIGSHOUT"))
die("Do not access this file directly.");

class main {

/**
* Holds the MySQL resource link
*
* @var resource
*/
private $mysql;

/**
* Holds the MySQL Host
*
* @var string
*/
private $mysqlhost = 'localhost';

/**
* Holds the MySQL User
*
* @var string
*/
private $mysqluser = '';

/**
* Holds the MySQL Password
*
* @var string
*/
private $mysqlpass = '';

/**
* Holds the MySQL Database Name
*
* @var string
*/
private $mysqldb = '';

/**
* Holds the database to use when connection to the server (mysql or mysqli)
*
* @var string
*/
private $mysqlconn = 'mysqli';

/**
* Holds the query to be run next, removed after it's run
*
* @var string
*/
var $sql = '';

/**
* Holds an array of all the query's run, so if we need to dump them we can.
*
* @var array
*/
private $sqllog = array();

/**
* Holds the number of query's run
*
* @var int
*/
private $numquerys = 0;

/**
* File Versions of SigChat website
*
* @var string
*/
var $version = '0.021';

/**
* Connects to the MySQL database, checks for errors, and then will spit out true or an array.
*
* @return bool\array
*/
function __construct ( ) {

if ( ($this->mysqlconn == '') || ($this->mysqlconn != 'mysql') || ($this->mysqlconn != 'mysqli') ) {

$this->mysqlconn = 'mysqli';

}

if ( $this->mysqlconn == 'mysql' ) {

// Attempt to connect to the MySQL database
$this->mysql = mysql_connect( $this->mysqlhost , $this->mysqluser , $this->mysqlpass );

// We never got a connection?
if ( !$this->mysql ) {

// Let's give out script a nice array to work from
$return = array("status" => "false", "error" = "An error has occured while connecting to the MySQL database." , "mysql_error" = mysql_error($this->mysql) );

// Just in case, we'll run this in depricated error mode.
@mysql_close( $this->mysql );

// Then we will unset that variable incase something happened to it, to be on the safe side.
unset( $this->mysql );

// Finally give the calling script back an array.
return $return;

}

// Depricate this incase we don't get a connection to that database, but we'll check it with the same function as above.
@mysql_select_db( $this->mysqldb , $this->mysql );

// Did we get an error selecting the database <-- Should never happen might I add
if ( mysql_errno( $this->mysql ) ) {

// Let's give out script a nice array to work from
$return = array("status" => "false", "error" => "An error has occured while connecting to the MySQL database." , "mysql_error" => mysql_error($this->mysql) );

// Just in case, we'll run this in depricated error mode.
@mysql_close( $this->mysql );

// Then we will unset that variable incase something happened to it, to be on the safe side.
unset( $this->mysql );

// Finally give the calling script back an array.
return $return;

}

// We got a connection using the mysql database result, yipdedo!

} else {

// Attempt a MySQL database connection; using mysqli OOP we'll be gansta yeh!
$this->mysql = new mysqli( $this->mysqlhost , $this->mysqluser , $this->mysqlpass , $this->mysqldb );

if ( mysqli_connect_error( ) ) {

// Let's give out script a nice array to work from
$return = array("status" => "false", "error" => "An error has occured while connecting to the MySQL database." , "mysql_error" => mysqli_connect_error( ) );

// Just in case, saftey first
$this->mysql->close();

// Then we will unset that variable incase something happened to it, to be on the safe side.
unset( $this->mysql );

// Finally give the calling script back an array.
return $return;

}

// We have a MySQLi connection, double woo!

return true;
}

/**
* Takes the string that is inputed and real_escapes it.
*
* @return string
*/
public function cleansql( $sql ) {

// If we are using MySQL, use MySQL
if ( $this->mysqlconn == 'mysql' ) {

// Clean the string
$string = mysql_real_escape_string( $sql , $this->mysql );

// Now we need to do it for MySQLi people, thanks
} else {

// Am I repeating myself? Clean the string :]
$string = $this->mysql->real_escape_string( $sql );

}

// Be nice, return the string to the script
return $return;

}

/**
* Runs the MySQL query given to it.
*
* @return bool/array
*/
public function query ( ) {

// Well, we'll local set the SQL
$sql = $this->sql;

// Run the following
if ( $this->mysqlconn == 'mysql' ) {

// YES! We ran the query
$query = mysql_query( $sql , $this->mysql );

// We had an error folks, don't worry.
if ( mysql_errno( $this->mysql ) ) {

// This is the string, in fact, we will be returning.
$return = array( "success" => "false" , "error" => "An error has occured while querying out database" , "mysql_error" => mysql_error( $this->mysql ) );

// Let's log the data ... should be outputs in an e-mail (Will I code it?)
$this->sqllog[] = array( "success" => "false" , "sql" => $sql , "errorno" => mysql_errno( $this->mysql ) , "error" => mysql_error( $this->mysql ) );

// Unset the query
unset( $this->sql );

// Query was good, let's log the data and return a true.
} else {

// Return string
$return = $query;

// Log data
$this->sqllog[] = array( "success" => "false" , "sql" => $sql );

// We in fact ran a good query, add one to the counter
$this->numquerys++;

// Unset the query
unset( $this->sql );

// Everything is done, for MySQL at least
}
// Crossover
} else {

// Run the query
$query = $this->mysql->query( $sql );

if ( $this->mysql->errno ) {

// This is the string, in fact, we will be returning.
$return = array( "success" => "false" , "error" => "An error has occured while querying out database" , "mysql_error" => $this->mysql->error );

// Let's log the data ... should be outputs in an e-mail (Will I code it?)
$this->sqllog[] = array( "success" => "false" , "sql" => $sql , "errorno" => $this->mysql->errno , "error" => $this->mysql->error );

// Unset the query
unset( $this->sql );

// Free the result
$query->close();

// Query was good, let's log the data and return a true.
} else {

// Return string
$return = $query;

// Log data
$this->sqllog[] = array( "success" => "false" , "sql" => $sql );

// We in fact ran a good query, add one to the counter
$this->numquerys++;

// Unset the query
unset( $this->sql );

// Free the result
$query->close();

// Everything is done!!
}

// End the query crap,
}

return $return;

// Close the function
}

}

?>

Also, in the query() function, would returning the mysqli data into another variable then freeing the result work?

[krakjoe]

You're mixing php4 and php5 class notation. Its not var it's public, there's nothing wrong with that, but might want to fix it all the same.

If you're going to use @documentation syntax and private / protected you should probably use @access in the docblock comments.

If you're planning on distributing the class for people to integrate into their own code, you might think about naming the class a bit better, their application might have a main class already ...

You can't return an array from an object constructor, you'll need another mechanism, commonly setError and getError(s), with indicators such as hasErrors or isError

You shouldn't mysql_real_escape_string on whole queries, only input data in those queries, this might cause SQL syntax errors, you don't HAVE to change it if it's working for you, but you probably should ...

Copying a variable and freeing a result would work, however it might not be the best approach ...

Lastly, I think your approach to writing an object is somewhat confused. The idea of writing classes is reusing code, this technique is named DRY - don't repeat yourself, lemme explain some more ...

However you use your code, you're going to end up repeating yourself. Let's say I had a table in a database I wanted to abstract with php, I'd have a think about what methods I would need to manage the data in the table without needing to write complicated SQL statements in my code to do so. Let's imagine my table was a member table, instead of providing a way to query the database/table, you would abstract the data by providing a Create(), Get(), isUser(), Authenticate() and other methods, the only data passed between my object and the procedural code that executes it is the bear minimum, only what's needed, and once I wrote my SQL statements for those methods, I pretty much never have to think about it again !!

So there's my advice, take it or leave it ...

[BrandonTheG]

Thanks for the tips, I'll work on that later (This is going to be distributed in my application I am making).