Getting cars and bicycles from a database and writing them into two different arrays

Question

I've written a simple class which should:

• get 10 top cars and 10 top bicycles from database
• write them into 2 different arrays

It works, but I'm trying to find a simpler or clearer solution.

class TopVehicles extends Connection {
    private $sql1 = 'SELECT model, price FROM vehicles WHERE type = \'car\' ORDER BY price DESC limit 10';
    private $sql2 = 'SELECT model, price FROM vehicles WHERE type = \'bicycle\' ORDER BY price DESC limit 10';

    function __construct() {
        parent::__construct();
    }

    public function getTopCars() {
        $stmt = $this->mysqli->prepare($this->sql1);
        $stmt->execute();
        $stmt->store_result();
        $row_cnt = $stmt->num_rows;    
        $stmt->bind_result($model, $price);

        while ( $stmt->fetch() ) {
            $_result[] = 
            array(
                'Model' => $model,
                'Price' => $price
                );
        }

        $stmt->free_result();
        $stmt->close();
        return $_result;
    }

    public function getTopBicycles() {
        $stmt = $this->mysqli->prepare($this->sql2); /*this is the only line which differs these 2 functions!*/
        $stmt->execute();
        $stmt->store_result();
        $row_cnt = $stmt->num_rows;    
        $stmt->bind_result($model, $price);

        while ( $stmt->fetch() ) {
            $_result[] = 
            array(
                'Model' => $model,
                'Price' => $price
                );
        }

        $stmt->free_result();
        $stmt->close();
        return $_result;
    }

    function __destruct() {
        parent::__destruct();
    }
}

$obj = new TopProducts();
$_TopCars = $obj->getTopCars();
$_TopVehicles = $obj->getTopBicycles();

It works as expected, but it's messy.

Is it possible to somehow rearrange these 2 methods?

• getTopBicycles()
• getTopCars()

To stick to the rule: DRY? I've tried to divide them, but I am not able to write proper code. Can you please give me a hint on what I should do?

Answer 1

As far as I see the only difference is the type inside the SQL query. So why not pass that as parameter.

You could also think of creating additional classes for car and bicycle which call the getTop method of another class with the right parameters or if you extend the vehicle class set the corresponding type inside the constructor. Just to give you some more examples what you might be able to do or at least consider.

The following code is probably one of the easiest way to accomplish the DRY principle. But be aware that it's untested code.

class TopVehicles extends Connection {
    private $sql = 'SELECT model, price FROM vehicles WHERE type = ? ORDER BY price DESC limit 10';

    function __construct() {
        parent::__construct();
    }

    public function getTop($type) {
        $stmt = $this->mysqli->prepare($this->sql);
        $stmt->bind_param("s", $type);
        $stmt->execute();
        $stmt->store_result();
        $row_cnt = $stmt->num_rows;    
        $stmt->bind_result($model, $price);

        while ( $stmt->fetch() ) {
            $_result[] = 
            array(
                'Model' => $model,
                'Price' => $price
                );
        }

        $stmt->free_result();
        $stmt->close();
        return $_result;
    }

$obj = new TopVehicles(); // should be TopVehicles, shouldn't it?
$_TopCars = $obj->getTop('car');
$_TopVehicles = $obj->getTop('bicycle');

UPDATE: Without seeing the final code a code review is rather hard. It's true that you can't bind column names in a prepared statement. But what you could do is to manipulate the SQL string first. So here is a way how to accomplish that:

private $sql = 'SELECT %s FROM %s WHERE type = ? ORDER BY %s limit ?';

public function getTop($table, array $columns, array $orderByColumns, $type, $amount) {
    $sql = sprintf($this->sql, implode($columns, ', '), $table, implode($orderByColumns, ', '));
    $stmt = $this->mysqli->prepare($sql);
    $stmt->bind_param("si", $type, $amount);
    ...

$_TopCars = $obj->getTop('vehicles', array('model', 'price'), array('price DESC', 'model ASC'), 'car', 10);

Note: This snippet is again untested and there is enough room for improvement, but it should provide you with an idea on how you could do it. But make sure that you don't add SQL Injection vulnerabilities by using user input directly. This snippet contains also @Alex L's suggestion to pass the amount as parameter, too.

Note: Your class names also indicate that your code violates the http://en.wikipedia.org/wiki/Liskov_substitution_principle. TopVehicles are no Connection. I'd say it's better to dependency inject the connection.

class TopVehicles {
    private $mysqli;
    private $sql = 'SELECT model, price FROM vehicles WHERE type = ? ORDER BY price DESC limit 10';

    function __construct($mysqli) {
        $this->mysqli = $mysqli;
    }
    ...
}

Answer 2

To expand on @amkoroew's answer:

TopVehicles really doesn't seem to be an appropriate name here. When I see that, it makes me want to believe that I'll be given the vehicle that is on top of a stack of other vehicles! That wouldn't make much sense, would it!

I suggest renaming to something more appropriate such as plain ol' class Vehicles. Then you can rename your main method there to something such as getVehiclePrices.

---

To open up your code more and have it be less strict, you could pass a parameter into that function called $amount or something, and then return that amount of results. That way you're not always limited to 10 rows. You'd end up having:

$stmt->bind_param("si", $type, $amount);