current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

The active column in the database only contains a boolean. The country column contains ISO 3166 A2 codes (2 char). Do you have any advice on improving this class? 

<?php
class WebShops {
    private $database;

    public function __construct() {
            $this->database = new Database();
    }

    public function fetchAll() {
            $this->database->query("SELECT * FROM web_shops");
            return $this->database->fetchAll();
    }

    public function fetchActive($option) {
            $this->database->query("SELECT * FROM web_shops WHERE active = :option");
            $this->database->bind(':option', $option);
            return $this->database->fetchAll();
    }

    public function fetchCountry($country) {
            $this->database->query("SELECT * FROM web_shops WHERE country = :country");
            $this->database->bind(':country', $country);
            return $this->database->fetchAll();
    }
}
share|improve this question

2 Answers 2

up vote 1 down vote

You could validate $option and $country before you put them in a query. One needs to be true or false, and the other should be two letters A-Z. You need some kind of error handling for that.

For the rest: This is such a simple class, and the context is missing, what would you want us to say?

share|improve this answer
up vote 1 down vote

I agree with @KIKO Sotfware that becuase the question lacks context, we cannot provide an extensive review. But there is one thing which caught my eye.

You ahould utilize dependency injection in your constructor. By hard coding your database class into the constructor you make it hard to swap the implementation if a another one is required in the future or you want to write a test.

I would build a generic interface which implements the different method the class uses.

interface GenericDatabaseInterface {

    public function query($sql);
    public function bind($key, $value);
    public function fetchAll();

}

Your database class should implement this interface. You would then type-hint this interface in your constructors argument list. If you in the future requires another implementation the new class should also implement the interface.

public function __construct(GenericDatabaseInterface $database) {

    $this->database = $database;

}

Hope this can guide you a little, happy coding!

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.