Feedback on PHP Class

[mmcdonald]

Hi all,

I've been playing around with PHP for a while now and never really studied hardcore (Just used it as best as I could on a whim) but I've now decided to sharpen the skill and get much better. I'm playing around with my first class and would like some feedback on its structure and general syntax. Is it a fairly good start? Are there any concerning issues I should be aware of? Thanks in advance! I've commented throughout so people are aware of what they're supposedly looking at.

 <?php

     class action{

        ////////////////////////////////////////////////////////

        private function connect(){

            $mysqli = mysqli_connect($config['host'], $config['user'], $host['password'], $config['db']);

            if (mysqli_connect_errno()) {
                printf("Connect failed: %s\n", mysqli_connect_error());
                exit();
            }

        }

        ////////////////////////////////////////////////////////

        private function disconnect(){

            $thread = $mysqli->thread_id;
            $mysqli->close();
            $mysqli->kill($thread);

        }

        ////////////////////////////////////////////////////////

        public function userInfo(){

            // Make the userInfo array available on a global scope.
            global $userInfo;

            //Lets make sure we are connected to the SQL DB
            if ($mysqli_connection->connect_error) {
               action::connect();
            }

            //Lets be awesome and protect ourself from SQL injections
            $stmt = $mysqli->prepare("SELECT * FROM `users` WHERE `authuser` = ?");
            $stmt->bind_param('s', $_SESSION['auth']);
            $stmt->execute();
            $stmt->store_result();

            //This will allow us to use the array anywhere such as: $userInfo['authuser'];
            $userInfo = $stmt->fetch_array(MYSQLI_ASSOC);

            // Free up resources
            $stmt->free();

            //We need to keep the system optimised by closing our connections!
            action::disconnect();

            }

        ////////////////////////////////////////////////////////

        public function checkAuth(){

            // Do we have the users firstname? We should - but it's best to check
            if(isset($_SESSION['auth'])){
                if ((basename($_SERVER['PHP_SELF']) != "profile.php") && $userInfo['fn'] == null){
                    header('Location: profile.php');
                    exit();  
            }
                // Stop logged in users from viewing login.php
                if (basename($_SERVER['PHP_SELF']) == "login.php"){
                    header('Location: index.php');
                    exit();
                }

            }

            // Okay, this user is not authorised to enter the platform. 
            // Lets make sure they're not accessing premium content.
            else if (basename($_SERVER['PHP_SELF']) != "login.php"){
                header('Location: login.php');
            }

        }

        ////////////////////////////////////////////////////////

     }
?>

What I'm ultimately looking to do is improve on my PHP; thanks!

[cereal]

First, remove global $userInfo; from line 33, and set it as public/protected/private variable at the top of the class:

class Action
{
    public $userInfo;

So, at line 47 you can write:

$this->userInfo = $stmt->fetch_array(MYSQLI_ASSOC);

[mmcdonald]

@Cereal - thanks for your input. How would this be beneficial? Would you mind explaining why someone would do this? Not being an ass I just want to fully understand what it is you're explaining. Also, why $this? Has this modification changed how I echo $userInfo['value']; in other files?

[NardCake]

Looking good so far, just a few things that need to be corrected.

First, in the connect method where does that config array come from?

In the disconnect method, where does the mysqli variable come from?

In the userInfo method you call the disconnect method statically with the scope resolution operator when it's not declared static.

Also you should start using properties.
Anyways those are the things that jump out at me, good luck!

[NardCake]

$this refers to the current class.

[cereal]

No problem. For example because this will make the property (variable) available inside the extended classes:

class Mailer extends Action
{        
    public function send_mail()
    {
        $emailTo = $this->userinfo['email'];

        # ... other code here ...
    }
}

It will be more easy to deal with data between classes. Check these links for more information:

• http://www.php.net/manual/en/language.oop5.properties.php
• http://www.php.net/manual/en/language.oop5.static.php

[mmcdonald]

First, in the connect method where does that config array come from?

This class file (classes.php) is included below config.php in all files, which contains the array.

In the disconnect method, where does the mysqli variable come from?

The $mysqli variable comes from the connect method and is only called after a connection is already open.

In the userInfo method you call the disconnect method statically with the scope resolution operator when it's not declared static.

Could you please explain?

Also you should start using properties.

I have no idea what these are so I'm opening Google now :)

$this refers to the current class.

Thanks! Is this built into PHP then? Can $this only be used within a classes methods?

[mmcdonald]

No problem. For example because this will make the property (variable) available inside the extended classes.....

Okay that makes sense. So by setting it globally in the class rather an a method I can call upon it in any other class too? Useful! I'll check out the links now.

[mmcdonald]

So ultimately what we've identified here is that for me to successfully use classes I need to look into properties and static. Thanks guys!

Here's a question.

Using the example above, how do I connect to SQL in anthother file? I would imagine I'd write action::connect(); but that would require the cnnect method to be static. Is this the rigth way to do it or is there a better way?

[NardCake]

You would only use action::connect() if connect is declared as:

private static function connect() {
    ...
}

Otherwise with how it is originally $this->connect() is the best.

[mmcdonald]

Don't I first need to call $this = new action; followed by $this->connect();? I.E. declare the object?

[NardCake]

Not when you're inside the class.

[diafol]

DB connections are a hot topic of debate. Some suggest the creations of singletons, other static classes. Pass them via dependency injection...

You certainly want to stay clear of including DB connection details in every class you create (that need a DB connection). OOP classes should do one job only and things like DB classes should be generic enough that they can be used with all your classes by offering a tight interface (that sounds contradictory - but what I mean by that is that the DB class isn't coupled to any other class, that is it has no knowledge of which class is using its methods or what that class does).

Creating a dedicated DB class should be worthwhile. Use php built-in functionality, e.g. extend native classes, rather than reinvent the wheel. For example, you could extend the PDO class. There are quite a few PDO wrapper classes out there. Useful for picking apart to see how they work and more importantly how they can be used with other disparate classes.

[NardCake]

Yeah, PDO is great!

[veedeoo]

Please allow me to add my opinion about developing an extensible codes in OOP. It is always nice to start on the right track than going back for some bits and pieces that were left behind.

There are 10 design patterns in PHP. I was 13 when I first hit my keyboard to type <?php ?>, and as far as I can remember there were only 3 to 5 design patterns commonly used in PHP. Today, PHP has become a powerful Object oriented programming language and the design patterns acceptable to PHP development are now 10. It might be more, but I will only give the 10 to help you out which one will be your gig.

Just be very careful though, because design pattern does not apply to all situations. Important quote I picked up along the way "When you are holding a hammer, everything looks like a nail."

The reason I brough up this subject is to help people know that for every application there is a proper tools or coding techniques that should be implemented. I know very well that typing echo "this" returns the string "this", but that is only a speck of dust hovering by the PHP's front door. The core of PHP is evolving at an extremely high speed. If you ever tried writing codes in python or ruby there isn't a lot of upgrade activity there, while in PHP the update is happening every day if not hours.

After learning the construct, one should take at least about 2 weeks in learning design patterns as listed below. Order does not reflect the order of importance. Your choice...

1. Singleton pattern
2. Factory pattern
3. Chain of command pattern
4. Observer Pattern
5. Strategy Pattern
6. State Pattern
7. Iterator pattern
8. Adapter pattern
9. Decorator pattern
10. Delegator Pattern ... Not sure It might be delegate Pattern or something... my memory is not serving me well at times.

There was a book published by O'reily and it was called " Head First Design Patterns " The book is not necessarily for PHP, but should apply to all developer.

I strongly suggest to read the book, before diving into the hardcore of OOP. Who knows the next time you post a question here it is for someone to test your very own framework.

[pritaeas]

" Head First Design Patterns "

Personally I prefer this one.

[mmcdonald]

Hi all - thanks for the insight; I've only just managed to get back to this. This doesn't report any errors but it doesn't give any output at all. How do I actually echo out array values from a method?

<?php

    class user{

        public $get;

        function get(){

            $user = "mmcdonald";

            $link = mysqli_connect("localhost", "root", "", "studybubble");
            $stmt = $mysqli->prepare("SELECT * FROM `users` WHERE `authuser` = ?");
            $stmt->bind_param('s', $_SESSION['auth']);
            $stmt->execute();
            $stmt->store_result();

            $this->get = $stmt->fetch_array(MYSQLI_ASSOC);

        }

    }

    $object = new user;

    // If I wanted the current users first name, fn is the column name in users table
    echo $object->get['fn']; 

    // If I wanted the current users last name
    echo $object->get['ln']; 

    // If I wanted the current users email address
    echo $object->get['authemail']; 

?>

As this topic already suggests - this is my first run into OOP and as much as I love it... I just don't get it yet :3

[Dani]

This code is untested as I'm just typing it off the cuff, but try something more along these lines:

<?php

    class database {

        ... blah blah blah

        public function fetch($query = '')
        {
            ... blah blah blah ...
        }

    }

    class user {

        private $person;

        public function get($property) {

            return $this->person[$property];

        }

        // Constructor function that gets called when creating an object
        public function __construct($authuser = 0)
        {
            global $database;

            $this->person = $database->fetch("SELECT * FROM `users` WHERE `authuser` = $authuser");
        }

    }

    $database = new database;

    $person = new user(12345);

    // If I wanted the current users first name, fn is the column name in users table
    echo $person->get('first_name'); 

    // If I wanted the current users last name
    echo $person->get('last_name'); 

    // If I wanted the current users email address
    echo $person->get('email'); 

?>

[diafol]

As this topic already suggests - this is my first run into OOP and as much as I love it... I just don't get it yet :3

It takes some time to get your head around it. I'm still trying :) Having said that, there are very few projects where I don't use OOP anymore.

[mmcdonald]

Thanks Diafol :) I'm probably going to open a few more topics tomorrow xD