Simple VisitorCounter class

I am trying to learn the absolute best way to program/design classes in PHP. I am hoping that you would be able to review and critique this simple class.

Note: I know that there aren't any comments. Please don't critique me on that.

db = $dbh;
    }

    public function updateNumVisits()
    {
        if(!$this->getNumVisits() || !$this->firstTimeHere()){ return false; }        

        $_SESSION['new_visitor'] = 1;

        $sql = "UPDATE stats SET visits=". ++$this->numVisits ." WHERE id=2";
        if(!$this->db->query($sql)){
            $sql = "INSERT INTO stats (visits) VALUES (". ++$this->numVisits .")";
            return $this->db->query($sql)? true : false;
        }
        return true;
    }

    protected function firstTimeHere()
    {
        if(isset($_SESSION['new_visitor'])){
            return false;
        }
        return true;
    }

    protected function getNumVisits()
    {
        $result = $this->db->query("SELECT * FROM stats");
        if($row = $this->db->fetch_array($result)){
            return $this->numVisits = $row['visits'];
        }
        return false;
    }

}

Implementation

$Counter = new VisitorCounter($db);
$Counter->updateNumVisits();

Questions

-
Is it ever ok to "for example", to move the $Counter->updateNumVisits() directly into the constructor? Or is that NEVER good to do, even in the simplest of scenarios? I'm reading that it is best only to do simple assignments in the constructor.

-
Inside the function updateNumVisits(), I have a conditional that runs a function getNumVisits(). If this is not advisable, when should I call this function so that updateNumVisits() will have access to the latest visit count?

-
I read many Code Review posts about not using globals, and so I got rid of my $db global out of the class, and injected it in the constructor. What about $_SESSION and $_POST values? Is it ok to have them inside your class?

-
How would you write this class?

-
I was also

First of all, I started rewriting your class but found many implementation problems design wise that stopped me from rewriting it (because it would be to much for you to learn in one go and would take me a long time to design, code and explain). So I have done a small portion.

Problems Are

• No type casting so ANY object could be passed to the constructor

• What if the database object changes? The method calls on the db object inside the VisitorCounter class will break

• You are doing two roles inside the VisitorCounter class, you are both keeping a global visitor count using the database AND trying to keep track of the users visits. Also, you are hard coding it to the $_SESSION! What if you want to use APC or Memcache for example!? Code changes would occur.

• You are not programming to interfaces but instead to concrete classes (and globals i.e. $_SESSION)

Solution

This is a semi pseudo (not complete but will show for demonstration purposes) PHP example just to demonstrate many things for you to learn but i have only taken out the site counter part of your visitorcounter class. You can implement thevisitorcounter in exactly the same way I have done for the DB & Site Counter, but instead for Session & Visitor Counter which would be brilliant practice for you! ;)

Read the comments in the comment.

/**
* by programming to this db interface, you can CHANGE the db class to ANY class you want at any time. As long as the new class
* implements this interface, the rest of the code will no how to handle the class :) this demonstrates programming to implementations
* and not to (concrete) classes
*/
interface db_connection {
    public function connected();
    public function query($sql);
}

/**
* this will be where the database specific database calls will be made. I have named this one mysql, but I only
* did this because it means I could have db_oracle (and others), and if I were to pass a db_oracle instace to the visitor counter construct
* it would still work :D as long as the db_oracle class implemented db_connection. You can then interchange db_mysql and db_oracle as you wish
*/
class db_mysql implements db_connection {
    public function connected() {
        //returns true or false depending on if connected
    }
    public function query($sql) {
        //actually makes the query, returns the result resource if ok, false if it failed
    }
    public function escape($string) {
        //escapes the string to protect from sql injection attacks
    }
    public function result($sql_resource) {
        //if the resource contains results, will return the first field from the first row of the result, FALSE otherwise
    }
    //... and all the other methods you want such as connect() disconnect() ping() etc...
}
/**
* site counter
*/
class site_counter {

    private $db = null;
    private $total = 0;

    public function __construct(db_connection $db) {
        $this->db = $db;
    }

    /**
    * increments the site counter
    */ 

public function increment($step = 1) {

    //ensure the step is numeric
    if ( ! is_numeric($step)) {
        throw new Exception("Expected a number, got something else!");
        return false; //didn't work
    }

    //notice the limit clause, use this to indicate to SQL that it no longer has to keep searching for more to update
    //you can increment existing fields by using SQL directly, don't do things the long way around as you did as you will also have concurrency issues (another poster I believe explained it)
    $step = $this->db->escape($step); //just to be safe ;)
    if ($this->db->query("UPDATE stats SET visits=visits+{$step} LIMIT 1;")) {
        return true; //worked ok
    }

    return false; //didn't work

}

    /**
    * resets the counter in the database and session
    */
    public function reset($to = 0) {
        if (is_numeric($to)) {
            $to = $this->db->escape($to); //just to be safe ;)
            //notice the limit clause, use this to indicate to SQL that it no longer has to keep searching for more to update
            return $this->db->query("UPDATE stats SET visits={$to} LIMIT 1;");
            } else {
            throw new Exception("Expected a number, got something else!");
        }
        //something went wrong
        return false;
    }

    /**
    * resets the counter in the database and session
    */
    public function get_total_hits() {
        //the (int) is called type casting, so false (if the query fails for example) will return 0
        return (int) $this->db->result($this->db->query("UPDATE stats SET visits={$to} LIMIT 1;"));
    }

}

StackExchange Code Review Q#17944, answer score: 5