Sloppy class please help

[Geo200k]

Hi, im new to php and im trying to learn by building a text based car game. The problem im having is i designed a class to list all the cars a player has or can purchase new or is in the scrapyard. The problem is i have really badly designed it and i found a huge flaw.

This is the class

class CarList
{
     
     //class attributes
     var $title;
     var $description;
     var $sql;
     var $nores;

     
     var    $i = '0';
    var $n = '4'; //starting row cars
    var $s = '4'; //cars per row
     
    public function __construct($title, $description, $sql, $nores = 'No Cars to Display!'){
        $this->title = $title;
        $this->sql = $sql;
        $this->description = $description;
        $this->nores = $nores;
        $this->DisplayCars();
    }
    
    
     protected function DisplayCars()
     {
        echo '<h1>'.$this->title."</h1>\n".$this->description."\n";
        echo "<table class=\"products\">\n";
        echo "<tr>\n";
                    
        require('inc/db/dbconnect.inc.php');
        
        @$query = $db_conn->query($this->sql)
            or die('Failed to connect to database and retrieve your Cars! Please report this problem.');
            
        while ($cars = $query->fetch_array())
        {        
    
            $tbl = $this->DrawTable($cars, $this->DisplayImage($cars, $this->ImagesExists($cars)));
        
            echo $tbl;
        
            $this->i++;

        }
        
        if ($this->i == '0')
        {
            echo '<td><b>'.$this->nores."</b></td>\n";
        }
        
                echo '</tr></table>';
        
    }
    
    
    protected function ImagesExists($cars)
    {
        if (file_exists('images/cars/'.$cars['imgsrc']) && !empty($cars['imgsrc']))
         {
              return true;
        }else{
             return false;
        }
    }
    
    
    protected function DisplayImage($cars, $exists = true, $sclass = 'gsel', $nclass = 'gnsel')
    {
        if ($exists == true)
         {
              if ( $cars['selected'] == '1')
              {
                $img = '<img src="images/cars/'.$cars['imgsrc'].'" '.ImageResize('images/cars/'.$cars['imgsrc'], 100).' alt="'.$cars['car'].'" class="'.$sclass.'" />';
            }else{
                $img = '<img src="images/cars/'.$cars['imgsrc'].'" '.ImageResize('images/cars/'.$cars['imgsrc'], 100).' alt="'.$cars['car'].'" class="'.$nclass.'" />';
            }
        }else{
         
            if ( $cars['selected'] == '1')
              {
                $img = '<img src="images/cars/none.gif" alt="'.$cars['car'].'" class="'.$sclass.'" />';
            }else{
                $img = '<img src="images/cars/none.gif" alt="'.$cars['car'].'" class="'.$nclass.'" />';
            }
        }
        
        return $img;
    }
            
    
    protected function DrawTable($cars, $img)
    {
        if ($this->i == $this->n)
         {
            $tbl = "\n</tr>\n<tr>\n<td><a href=\"index.php?p=cardet&amp;item=".$cars['carid'].'&amp;tid='.$_SESSION['tid'].'">'.$img.'</a><br />'.$cars['car'].' - $'.$cars['price'].'.</td>';
            $this->n += $this->s;
            
        }else{
         
            $tbl = "\n<td><a href=\"index.php?p=cardet&amp;item=".$cars['carid'].'&amp;tid='.$_SESSION['tid'].'">'.$img.'</a><br />'.$cars['car'].' - $'.$cars['price'].'.</td>';
        }
        
        return $tbl;
        
    }

} 

It is accessed on a page called garage, which is all the cars a player owns, like this. This will also work for cars in the player scrapyard.

          $sql = "SELECT
                cars.carid,
                cars.car,
                cars.description,
                cars.`type`,
                cars.imgsrc,
                garage.price,
                garage.bhp,
                garage.speed,
                garage.selected
            FROM
                garage
                Inner Join cars ON garage.carid = cars.carid
            WHERE
                garage.userid = '{$_SESSION['uid']}'";
                
    $garagelist = new CarList('Welcome to Your Grage', 'Below are your cars.', $sql, 'You need to Buy a Car!'); 

The huge problems is that, because i am using a ref table to hold the players cars and a car table to hold the cars, i cannot list all the cars a player does not have.

[cuberoot]

I only had enough time to take a cursory look over the source, so forgive me if this is a silly question, but is there anything preventing you from using a left join? For instance --

SELECT cars.car, NOT garage.userid IS NULL AS has_car, ... FROM cars LEFT JOIN garage ON cars.carid = garage.carid AND garage.userid = $userid

[Geo200k]

I may be wrong but surley that would still display the cars a player has.

Thanks anyway
George

[allspiritseve]

I'm not completely familiar with the syntax cuberoot used, but: a left join takes two tables, and joins them. Because it's a left join, and not an inner join, the rows in the left table (the first in the query) have to exist. The rows in the right table do not. So, if your left table is all cars, and your right table is player cars, and you join them using a common field (typically a car_id, or something similar) you will end up with some rows with player car information, and some cars with NULL as their values. These rows correspond to cars in the cars table that are not in the players table, which is what you want.

SELECT * FROM cars LEFT JOIN player_cars ON (cars.car_id = player_cars.car_id) WHERE player_cars.car_id IS NULL

[Geo200k]

Not quite, if i ran that sql it would list all the cars that are not owned by any player. Because more than one player can own the same car it wouldnt work. cuberoots code would work if i add an if statment to the loop like so

        while ($cars = $query->fetch_array())
        {        
            if ($cars['has_car'] != true)
            {
                $tbl = $this->DrawTable($cars, $this->DisplayImage($cars, $this->ImagesExists($cars)));
        
                echo $tbl;
                
                $this->i++;
            }
        } 

The problem with this, for me anyway, is what already seems like sloppy code gets worse as i would need to check the var is set. The way i see it is this is a very simple idea but when coded i can get it as simple as it should be in my mind.

Thanks

[allspiritseve]

Not quite, if i ran that sql it would list all the cars that are not owned by any player.

Ah, I thought the player_cars would just have a single player's cars in it. In that case:

SELECT * FROM cars LEFT JOIN player_cars ON (cars.car_id = player_cars.car_id AND player_cars.user_id = :user_id) WHERE player_cars.car_id IS NULL

(Note: I use PDO, so you could substitute :user_id with however you'd place a variable in a query)

[Geo200k]

Yep that does it. Any advise on my code as im very new to PHP.

Thanks alot
George

[Cups]

Are you new too OOP or PHP? What other languages have you learned then?

[cuberoot]

I'd break the database specific stuff out of the class, since it really hurts reusability. I'd also separate the structure of the view from the rendering process. Something like this --

class View {
    function accept($visitor) {
        $visitor->visit($this);
    }
}

class CarView extends View {
    protected $cars;
    function __construct($cars=null) {
        if ($cars) {
            foreach ($cars as $car) {
                $this->addCar($car);
            }
        }
    }
    function addCar(Car $car) {
        $this->cars[] = $car;
    }
    function getCars() {
        return $this->cars;
    }
}

abstract class HtmlTableRenderer {
    protected $tplTable;
    protected $tplRow;
    protected $html;
    function __construct($tplTable = '<table>%s</table>',
        $tplRow = '<tr><td>%s</td><td>%s</td></tr>') {
        $this->tplTable = $tplTable;
        $this->tplRow = $tplRow;
    }
    abstract function visit($view);
    abstract function render();
}

class CarViewTableRenderer extends HtmlTableRenderer {
    function visit($view) {
        if ($cars = $view->getCars()) {
            foreach ($cars as $car) {
                $this->html .= sprintf($this->tplRow,
                    $car->getName(), $car->getDescription());
            }
            $this->html = sprintf($this->tplTable, $this->html);
        }
    }
    function render() {
        return $this->html;
    }
}

class Car {
    private $name;
    private $description;
    function __construct($name, $description) {
        $this->name = $name;
        $this->description = $description;
    }
    function getName() {
        return $this->name;
    }
    function getDescription() {
        return $this->description;
    }
}

$cars = array();
$cars[] = new Car('Pinto', 'The finest automobile ever made');
$cars[] = new Car('Yugo', 'A huge commercial success');

/*
$cars could obviously be an iterator on a database query result,
or a collection of nodes parsed from a web service, etc.
*/

$carList = new CarView($cars);
$carList->accept($renderer = new CarViewTableRenderer());
echo $renderer->render(); 

This article offers further insight into this approach --
http://devzone.zend.com/node/view/id/9

[Geo200k]

cups - im new to php and oop, i have done a little in java but never fully grasped it and therefore my oop knowledge is very limited. I did a lot in vb as a student so am familiar with procedural code.

cuberoot - i see makes much more sence to do it like that.

Could anyone recomend any books on oop and design pattens.

Thanks George

[ServerStorm]

Hi Geo200k,

Could anyone recomend any books on oop and design pattens.

Before you dig yourself into the fowler books (i.e Patterns for Enterprise Architects) you should check out 'Guide to PHP Design Patterns' by Jason E. Sweat. This book is centered around patterns but will also help you understand a good way to code and test your code.

It's isbn# = 0-9735898-2-5 and it is published by php|architect so you will likely be able to order it right off their site; however I have not verified this.

Regards,
ServerStorm