8
\$\begingroup\$

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.

<?php
class VisitorCounter
{
    protected $numVisits;
    protected $db;

    public function __construct($dbh)
    {
        $this->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

  1. 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.

  2. 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?

  3. 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?

  4. How would you write this class?

  5. I was also hoping that you might know of a very simple open source PHP project that someone built that I can study and learn from. One that follows all the best practices mentioned here on this website. For instance that uses DI, and follows all the principles of good coding practices. I downloaded the Zend framework and CodeIgnitor but they are too complex and too big for my current understanding.

\$\endgroup\$

4 Answers 4

5
\$\begingroup\$

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

  1. No type casting so ANY object could be passed to the constructor
  2. What if the database object changes? The method calls on the db object inside the VisitorCounter class will break
  3. 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.
  4. 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;"));
    }

}
\$\endgroup\$
13
  • \$\begingroup\$ Ok your answer was awesome! Some things.... I was actually only trying to update the user counter if it was that person's first SESSION visit to the site. I wasn't actually trying to keep track of each user's total visits. But I would like to do what you said, but I dont' quite understand what you mean when you said this "You can implement the visitorcounter in exactly the same way i have done for the DB & Site Counter, but instead for Session & Visitor Counter" ? But everything else you said, I completely follow, and agree with. All the answers were great but this one went above and beyond! \$\endgroup\$
    – darga33
    Oct 26, 2012 at 20:22
  • \$\begingroup\$ "You can implement the visitorcou..." i mean if you want to keep track of how many hits a visitor has made during that specific session you can add that using the same design (with the interface, class, passing an instance of the class through the construct etc) as i did with the site_counter class. But instead of a database, your using a session ;) If you still don't understand, don't worry about it as long as you followed and understood that code i wrote ;) glad it helped. \$\endgroup\$ Oct 26, 2012 at 20:32
  • \$\begingroup\$ A couple things... In your function get_total_hits(), I think you accidentally forgot to run reset() and in your interface comment I think you meant "programming to an interface". I just wanted to mention that for when other people learn from your answer. And one incredibly awesome thing that I read about yesterday is about returning early. I want to start doing that. So for instance, in each one of the functions where we check to see if something is numeric, could we do... if(!is_numeric(){ throw exception return false;}, Then there is less if nesting! I have a lot to learn!!!! \$\endgroup\$
    – darga33
    Oct 26, 2012 at 20:34
  • \$\begingroup\$ Ok, I see what you mean now! Thanks again!!!! \$\endgroup\$
    – darga33
    Oct 26, 2012 at 20:38
  • \$\begingroup\$ I'm going to implement all of these things when I write a PDO class, any good PDO class that isn't crazily complicated that you know about that I can take a look at? \$\endgroup\$
    – darga33
    Oct 26, 2012 at 20:40
4
\$\begingroup\$

In addition to Peters advice, I would recommend:

2) to increase to value in the db without data from a previously read query. This allows race conditions to happen. Imagine what could happen (in this order, may also appear from different tabs with the same user):

  • user a reading the value 1
  • user b also reading 1
  • user a updating to 2
  • user b updating to 2

Better use "UPDATE stats SET visits=visits+1 WHERE id=2" to let the db increment stuff for you. As a side effect you don't need the $numVisits field anymore.

5) make fields and helper methods private. Search phrase for further reading here is encapsulation.

4) Write a client, which uses the class to see if it works as expected. Usually this is done via unit-test. You may then encounter the problem that you want to somehow mock the db - connection. Often, and as well in this case, simple examples seem to be overloaded when using 'real' separation of concerns, but try to hide the db behind an interface (e.g. Counter) which has one implementation for production (DBCounter) and one for testing (MockCounter). You can inject the counter via constructor and make a private $counter field. Then call $counter.increase or $counter.get in your code and move all the db-related stuff to the db-implementation. This not only allows testing your business-logic separately but also leads to classes more reusable and with higher cohesion.

\$\endgroup\$
6
  • \$\begingroup\$ Thx for the info! 2. I never thought of that! Good idea! Is it advisable to have a hardcoded SQL statement? This class is only reusable if a project has a "stats" table, with a record with an id of 2? Should I pass this data into the constructor as "options"? I want to do things the way a senior dev would design it. 4. I appreciate the info on unit testing, etc, but I'm not ready to learn that yet. I'm just trying to figure out how to write well designed classes. Also, 2., if getNumVisits() was necessary, would it be ok to run that code in the conditional of a different method? \$\endgroup\$
    – darga33
    Oct 26, 2012 at 16:14
  • \$\begingroup\$ About making the fields and helper methods private. The only reason I didn't make them private is because I thought that it would be better to make them protected in all my classes so that if I needed to extend them down the road, I wouldn't have to change the properties from private to protected later. Is the proper way to make them all private, and then if you need to extend them later to make them protected? @mseancole \$\endgroup\$
    – darga33
    Oct 26, 2012 at 19:39
  • \$\begingroup\$ @darga33: If you are absolutely positive that you are going to allow those fields to be extended, then I see no problem with this. Otherwise using private ensures that an element isn't extended when you don't want it to be. Here's some further reading on the subject. BTW: tagging me in another's comments isn't going to ping me. I just happened to see the new content and answered. \$\endgroup\$
    – mseancole
    Oct 26, 2012 at 20:34
  • \$\begingroup\$ @darga33 In my Opinion a senior dev would design it writing tests. So this is not a contradiction. \$\endgroup\$ Oct 29, 2012 at 11:35
  • \$\begingroup\$ @darga33 If you are pushing the implementation into a DBCounter class, you only have to touch this code if you need another DB structure. It depends on whether you are designing the code the run in one and only one app (where it is unlikely to change and therefore probably best hardcoded in the DBCounter class). Or are you designing a framework api, where the client of the DBCounter wants to have control over the table name and where-clause? Then you probably want to inject this. I do belief design is often not about wrong or right but making considerations about what trade-off fits best. \$\endgroup\$ Oct 29, 2012 at 11:43
3
\$\begingroup\$
  1. The construcotr may not contain any business logic/work process
  2. Your logic is not enough clear to me (hard coded WHERE clause first [id=2] then in getNumVisits() is just a simple select all query, etc.)
  3. No they should not be used in classes, force to inject them into the worker methods (ISession interface)
  4. Clarify your logic please
  5. Just read the common OO and SOLID principles, search for Martin Flowler's name
\$\endgroup\$
1
  • \$\begingroup\$ Thx!! 2. That WHERE clause is checking the database for a table called "stats". In that table there is a single record that happens to have an id of 2. That is where the number of unique visitors is being stored. I don't know how best to do this. Georg mentioned that I don't need a numVisits property and the getNumVisits() method anymore which is great! I'm just trying to figure out how a seasoned developer would write this exact simple Counter class. 5.I read the SOLID principles yesterday before making this post. Good stuff! I want to find an actual complete small project to study. \$\endgroup\$
    – darga33
    Oct 26, 2012 at 16:22
2
\$\begingroup\$

I think the other two answers answered your specific questions adequately. This will instead focus on the content.

Comments missing from a class are not as important as comments missing from an API or interface or abstract class. I wouldn't worry about it here. However, I would eventually expect to see some sort of interface and/or abstract class be implemented. In fact, it is normally considered best practice to start off with these.

Whitespace is your friend. Don't scrunch everything up to one line. You can get away with this if the lines are short, less than 80 characters including whitespace, but even then I would say that adding a newline makes them a little easier to read. This is a preference but one many appreciate.

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

I'm not really familiar with SQL, but your SQL logic looks like it might be plain MySQL. I would consider using PDO or MySQLi instead and taking advantage of the prepared statements. Additionally, as you have previously asked, these SQL statements probably shouldn't be hardcoded into your class. This limits its extensibility, however, I'll leave that open in case anyone else wants to add a more thorough explanation.

If you have a return statement that reflects a boolean value depending on the state of some expression, then you don't have to explicitly define those boolean states. You can just return the expression directly. Doing so may sometimes mean that your return values may not be pure boolean values but with the loose typing PHP has this shouldn't be much of a problem. However, sometimes it is undesirable to have anything other than a pure boolean value. In this case you can typecast the result.

return $this->db->query( $sql );
//typecasting
return ( bool ) $this->db->query( $sql );

//similarly
return isset( $_SESSION[ 'new_visitor' ] );

Overall this looks pretty good, but, as you said, this is a pretty simple class. Good job never-the-less, and I commend your abandoning the globals. The world is a better place because of it. If, as you seem to be implying, you are still using the globals outside of your class, I would strongly urge you to try refactoring to remove them.

\$\endgroup\$
9
  • \$\begingroup\$ First off, thanks for the answer! I never thought about returning a bool directly without testing for it first. I was using mysql, but only because I wanted to focus on learning how to build proper classes first. Then I am going to learn either PDO or Mysqli and write a class that can be swapped out. Also, I am going to learn docblock notation too. I'm just focusing on class design right now. And as far as the global, I am going to get rid of it completely. It's amazing that you recognized that I still had it hiding! Probably because I didn't write $db = new Db();, is that how you knew? \$\endgroup\$
    – darga33
    Oct 26, 2012 at 17:54
  • \$\begingroup\$ By the way, when I learn PDO, or MySqli which one do you think would be better? I have heard much praise about PDO, so I am leaning towards that! But it seems like it might have a longer learning curve? \$\endgroup\$
    – darga33
    Oct 26, 2012 at 17:54
  • \$\begingroup\$ @darga33: "...so I got rid of my $db global out of the class..." Was just using the queues from your post. Globals are normally really hard to spot, which is one of the reasons they aren't well liked. A tell-tale sign of a global is usually just a variable that appears out of nowhere. Though that could just as easily be a variable-variable which are equally bad but a little easier to distinguish because they are at least still in the local scope. \$\endgroup\$
    – mseancole
    Oct 26, 2012 at 18:15
  • \$\begingroup\$ From my limited experience PDO is fairly similar to MySQL in syntax and seems to be more popular. But as to which one is better, I think they are both equally good and which one you choose depends on your circumstances and preference. \$\endgroup\$
    – mseancole
    Oct 26, 2012 at 18:15
  • \$\begingroup\$ Makes perfect sense! \$\endgroup\$
    – darga33
    Oct 26, 2012 at 19:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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