TalkPHP - A little bit of code that needs critiquing

TalkPHP (http://www.talkphp.com/forums.php)

- Advanced PHP Programming (http://www.talkphp.com/advanced-php-programming/)

- - A little bit of code that needs critiquing (http://www.talkphp.com/advanced-php-programming/3480-little-bit-code-needs-critiquing.html)

A little bit of code that needs critiquing

what do you guys think:

PHP Code:


		
			
class sql {
   var $host   =   'host';
   var $user   =   'username';
   var $pass   =   'password';
   var $db     =   'database';
   var $link;
   var $sql;
   var $result;
   var $nl = '
';
   function __construct($sql, $format='', $ele1, $ele2=null, $ele3=null, $ele4=null, $ele5=null, $ele6=null, $ele7=null, $ele8=null, $ele9=null, $ele10=null) {
      if (strchr($sql,'auto-')) {$newsql='SELECT * FROM '.str_replace('auto-','',$sql);}
      else {$newsql=$sql;}
      if (!strchr($format,'::1')) {$newformat='<div class="'.$format.'">::1</div>';}
      else {$newformat=$format;}
      self::li($newsql, $newformat, $ele1, $ele2, $ele3, $ele4, $ele5, $ele6, $ele7, $ele8, $ele9, $ele10);
   }
   function co() {# Connect to database
      $this->link = mySQLi_Connect($this->host, $this->user, $this->pass, $this->db);
      if (!$this->link) {die();}
   }
   function cl() {mySQLi_Close($this->link);} #close database connection
   function q() { #preform query on database
      self::co();
      $this->result=mySQLi_Query($this->link,$this->sql);
      self::cl();
   }
   function li($sql, $format, $ele1, $ele2=null, $ele3=null, $ele4=null, $ele5=null, $ele6=null, $ele7=null, $ele8=null, $ele9=null, $ele10=null) {
      $max=1;
      for ($i=2;$i<=10;$i++) {
         if (${'ele'.$i}==null){break;}
         else{$max=$i;}
      }
      $this->sql = $sql;
      self::q();
      while ($row = mySQLi_Fetch_Assoc($this->result)) {
         $fin = $format;
         for ($i=1;$i<=$max;$i++) {
            $fin=Str_Replace('::'.$i,$row[${'ele'.$i}],$fin);
         } 
         echo $fin.$this->nl;
      }
   }
}

function sql($e1, $e2, $e3, $e4=null, $e5=null, $e6=null, $e7=null, $e8=null, $e9=null, $e10=null, $e11=null, $e12=null) {
   new sql ($e1,$e2,$e3,$e4,$e5,$e6,$e7,$e8,$e9,$e10,$e11,$e12);
}

description here: Sql.Class

The first thing that strikes me is that code is awfully difficult to read. I dislike the actual layout of the code, and there's a lack of commenting. I fear it would take me a while to understand what's going on there, and whilst you may think that's not such a bad thing, because perhaps you're the only person who's working on it. What happens if you neglect it and then come back to it in 6 month's time? Do you think you'll still understand what is going on?

I think the best approach is probably to separate out the difficult tasks to other classes. That way one class only has one responsibility, and that responsibility can be clearly detailed by yourself. So when you do come back to the code in 6 month's time, you'll be able to read the comments in which you wrote down, and begin working on the classes again within, hopefully, 5 minutes or so.

In addition to that, I think placing the curly brackets on new lines -- lines of their own, really does help in readability, but that's merely a personal preference.

@up- Totally agree. This code looks really messy and, as you said, very hard to read.

I do have to agree, although its certainly not the worst code block i have ever seen, it certainly could do with a bit of a tidy up.

Why if you are using a class that has PHP5 style syntax have you used 'var' instead of the visibility modifiers, if you want the class to be PHP4 compliant, you will need to remove your construct and replace it with the C++ style constructor (a method with the same name as the class), if not then use either 'public', 'private' or 'protected' instead of var (it used to call a E_STRICT warning, but i cant seem to replicate it).

Dont call non-static methods as static:

PHP Code:


		
			
self::co();

you call them using $this (unless declared as static, which isn't the case here):

PHP Code:


		
			
$this->co();

You need to also name your methods so that they are descriptive, I.E.

PHP Code:


		
			
public function connect() {}
public function disconnect() {}
public function execQuery() {}

as opposed to:

PHP Code:


		
			
public function co() {}
public function cl() {}
public function q() {}

Looking at the first example, i am immediately able to see what the method does because the name describes it well, therefore i don't need to actually read the code to find out what it does.

Apart from that its all personal preference. I personally would have the code block delimiters {} on a new line and I wouldn't use the bash comment style.

@ All the above, I couldn't agree more.

What they said, comment your code, use white-space to your advantage, be clear with variable names and so forth.

As opposed to:

PHP Code:


		
			
$fin
// Use
$finishedString
// or
$strFinished
// or
$vFinished

The first I would be most likely to use, but the str and v are a bit more clear if you're writing complete, long, clear code. str stands for string, obviously, and v stands for variable. You can also work with sFinished which the s would declare it to be a string.

Also, you should form up your comments with newlines and explain what you're doing. It doesn't have to be pretty, you wouldn't need to great a peice of ASCII art of sorts, but just explain the function. Therefor the name, description, static function, an example (how it could be used in your script) and so forth.

Don't be shy of using a couple of enters while you're at it.

PHP Code:


		
			
if($this->variable){
      function($var1,$var2,$var3);
}

is a bit too tight if you ask me. I would rather use this as a template.

PHP Code:


		
			
if ($this->variable) { 
      function ($var1, $var2, $var3 = false);
}

// or

if( $this->variable ) {

      function( $var1, $var2, $var3 = false ) {

}

The type of coding doesn't really matter. A way that ALWAYS works, if you get a couple of beers in, then check out the code in the early hours and THEN completlely, down to the letter, understand what it is about, you've written self-explandetory code.

One more for the road. Try to comment your code as wisely as you can.

PHP Code:


		
			
/* Name        : name
 * Values      : $var1( integer), var2( string )
 * Example     : name( $var1, $var2 );
 * Returns     : float
 * Build:      : 1.0
 */
function name( $var1, $var2, $var3 = false, $var4 = 0 ) {
      
      return someClass::someFunction();

}

Good luck and I hope this helped.

Don't worry about long function names:

sdba() or setDatabaseAdapter()... which do you understand better? I can't make sense of any of your functions except construct

Kinda what the complete topic answers are about. :-P