HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

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

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
arraysintowritinggettingbicyclesdatabasedifferenttwocarsand

Problem

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

Solution

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;
    }
    ...
}

Code Snippets

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');
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);
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;
    }
    ...
}

Context

StackExchange Code Review Q#54537, answer score: 3

Revisions (0)

No revisions yet.