Page 1 of 1

How can I optimize this code?

Posted: Wed Dec 17, 2008 6:57 am
by newbieprogger
~pickle | Please use [ code=html ], [ code=php ], etc tags where appropriate when posting code. Your post has been edited to reflect how we'd like it posted. Please read: :arrow: Posting Code in the Forums to learn how to do it too.


Hiya, how can I optimize this code, or is there a better one already out there?
Also, see anything wrong with it? Constructive criticism please :)

Code: Select all

class lol{
    var $dblink;
    var $dbarraycheck = array();
    var $dbinsertstr;
    var $dbdeletestr;
    var $dbupdatestr;
    var $dbselectstr;
        public function connect() {
            require('skyddad/condb.php');
            $this->dblink = $link;
            return $this;
        }
    //Note that $columns & $values is a string that define each value with a , e.g "myvalue ='value', newvalue='value''" and 
        public function dbinsert($tablename, $column , $values) {
            $this->dbinsertstr = "INSERT INTO $tablename (".$columns.") values (".$values.")";
            mysql_query($this->dbinsertstr);
            echo "Query inserted data into table ".$tablename." in column(s) ".$column." the value(s) ".$values.".";
            return $this;
        }
        public function dbdelete($tablename, $column, $values) {
            $this->dbdeletestr = "DELETE FROM $tablename WHERE $column = $values ";
            mysql_query($this->dbdeletestr);
            echo "Query deleted data from table ".$tablename." in column(s) ".$column." the value(s) ".$values.".";
            return $this;
        }
        public function dbupdate($tablename, $updates){
            $this->dbupdatestr = "UPDATE $tablename SET $updates";
            mysql_query($this->dbupdatestr);
            echo "Query updated data in table ".$tablename." updated were: ".$updates;
            return $this;
        }
        public function dbselect($column, $tablename,$condition, $rownr) {
            $this->dbselectstr = "SELECT ".$column." FROM ".$tablename;
            if( !empty($condition) ) {
                $this->dbselectstr = $this->dbselectstr." ".$condition;
            }
            $r = mysql_query($this->dbselectstr);
            for($i = 0; $i<$rownr; $i++){
            $this->dbarraycheck = mysql_fetch_array($r);
            }
            $this->dbselectstr = $this->dbarraycheck[$column];
            return $this;
        }
        public function disconnect(){
            mysql_close($this->dblink);
            if(!empty($this->dbselectstr) ) {
            return $this->dbselectstr;
            }
        }
    
}
 

~pickle | Please use [ code=html ], [ code=php ], etc tags where appropriate when posting code. Your post has been edited to reflect how we'd like it posted. Please read: :arrow: Posting Code in the Forums to learn how to do it too.

Re: How can I optimize this code?

Posted: Wed Dec 17, 2008 11:14 am
by jaoudestudios
var ...
Out of date!
return $this;
Why return this? defends the point of having class wide variables. Plus even if it was correct why return all of it?

Hope that helps :)

Re: How can I optimize this code?

Posted: Wed Dec 17, 2008 2:09 pm
by John Cartwright
I really do not see much room for optimization. However, there are plenty of improvements you can make.

1. why is your class named LOL? You should find something far more decriptive than that.
2. Limit the scope your attributes (var should be protected or private)
3. Do not echo anything within the class. An object should do one thing, and one thing only. Simply return a boolean or something similiar to indicate a success or failure, therefore it is more flexible.
4. No error reporting. You should always check that the query executed successfully by either using mysql_error() or simply checking that mysql_query() returned true.

If you wanted to keep your fluent interface I would add another function to get the result of the last operation. Something like

Code: Select all

 
 
public function affectedRows() 
{
   return mysql_affected_rows($this->_link);
}
 
public function getInsertId() 
{
   return mysql_insert_id();
}
 
5. Pass the connection resource to all the mysql_* functions, for those times that you are connection to more than one database.
6. I won't touch too much on your dbselect() function, as it is very simple and will not allow for many situations. One glaringly bad thing I see right now is you try to implement some sort of LIMIT without actually using the LIMIT clause. This will mean you will query your entire dataset, even though you only specified a certain amount. I'm not even sure exactly what you implemented when you iterate your resultset, but you were overwritting $dbarraycheck on every iteration without doing anything with it. Try something like..

Code: Select all

 
public function dbselect($column, $tablename,$condition = false, $rownr = false) {
   $this->dbselectstr = "SELECT ".$column." FROM ".$tablename;
   if ($rownr) {
      $this->dbselectstr .= " LIMIT ". (int)$rownr;
   }  
   
   ...
    
   while ($row = mysql_fetch_assoc($result)) {             
      $this->dbarraycheck[] = mysql_fetch_array($r);
   }
6. Notice above I gave the parameters default values. You can do this so you do not need to specifiy all the parameters if they are not always required.
jaoudestudios wrote:
return $this;
Why return this? defends the point of having class wide variables. Plus even if it was correct why return all of it?

Hope that helps :)
returning $this allows for a fluent interface (method chaining).

An (poor) example of a fluent interface:

Code: Select all

$foo = new Foobar();
$foo->bee()->bar()->boo();

Re: How can I optimize this code?

Posted: Wed Dec 17, 2008 3:39 pm
by newbieprogger
Thanks Jcart, let me explain sokme of those things you mentioned :P ,

1. I recently had a problem with my database and it couldnt find my class, so I renamed it a little quick and forgot to change it back :roll:
2. I was thinking of using the protected and private attributes, but I thought those only were used when you use several classes like extends class, a so called "child"?
3. Actually, I'll answer the fourth as well in this one; I already had error reporting on the page, and I just wanted to use echo as a quick result to see how many times a certain loop triggered by using echoes >.<
All things considered, I appreciate you correcting me
5. ------- Good point!
6. This one needs some explaining. The variables sent through the function are all strings except the last one, that is a row counter, I wanted it to search the found rows in a specific order, so if I say 2 it retrieves the second row that was found matching the $condition which could be for example "WHERE this = 'that'"
But this has got me thinking... Love you :wink:

I've seen in some scripts where the method chaining actually looked like this:
$var->something->else->lolnessaward;
instead of mine where I call functions:
$var->something()->else()->lolnessaward();

Is it because they're passing through variables or...?

Thanks for your help, all of you! :D

Re: How can I optimize this code?

Posted: Thu Dec 18, 2008 4:06 am
by jaoudestudios
Jcart wrote:I really do not see much room for optimization. However, there are plenty of improvements you can make.

1. why is your class named LOL? You should find something far more decriptive than that.
2. Limit the scope your attributes (var should be protected or private)
3. Do not echo anything within the class. An object should do one thing, and one thing only. Simply return a boolean or something similiar to indicate a success or failure, therefore it is more flexible.
4. No error reporting. You should always check that the query executed successfully by either using mysql_error() or simply checking that mysql_query() returned true.

If you wanted to keep your fluent interface I would add another function to get the result of the last operation. Something like

Code: Select all

 
 
public function affectedRows() 
{
   return mysql_affected_rows($this->_link);
}
 
public function getInsertId() 
{
   return mysql_insert_id();
}
 
5. Pass the connection resource to all the mysql_* functions, for those times that you are connection to more than one database.
6. I won't touch too much on your dbselect() function, as it is very simple and will not allow for many situations. One glaringly bad thing I see right now is you try to implement some sort of LIMIT without actually using the LIMIT clause. This will mean you will query your entire dataset, even though you only specified a certain amount. I'm not even sure exactly what you implemented when you iterate your resultset, but you were overwritting $dbarraycheck on every iteration without doing anything with it. Try something like..

Code: Select all

 
public function dbselect($column, $tablename,$condition = false, $rownr = false) {
   $this->dbselectstr = "SELECT ".$column." FROM ".$tablename;
   if ($rownr) {
      $this->dbselectstr .= " LIMIT ". (int)$rownr;
   }  
   
   ...
    
   while ($row = mysql_fetch_assoc($result)) {             
      $this->dbarraycheck[] = mysql_fetch_array($r);
   }
6. Notice above I gave the parameters default values. You can do this so you do not need to specifiy all the parameters if they are not always required.
jaoudestudios wrote:
return $this;
Why return this? defends the point of having class wide variables. Plus even if it was correct why return all of it?

Hope that helps :)
returning $this allows for a fluent interface (method chaining).

An (poor) example of a fluent interface:

Code: Select all

$foo = new Foobar();
$foo->bee()->bar()->boo();
Thanks Jcart. I did not realise that about method chaining - will research more :)