5
\$\begingroup\$

Explained what I'm doing in the comments:

class User {

    public $id;
    public $email;
    public $username;
    public $password;
    public $rep;

    public function __constructor($id, $email, $username, $password, $rep) { // Assign all 'required' fields for our object upon instantiation

        $this->id = $id;
        $this->email = $email;
        $this->username = $username;
        $this->password = $password;
        $this->rep = $rep;

    }

    public static function getUsers($query, $mysqli) { // Pass the query to select desired users and the mysqli connection object

        $all_users = $mysqli->query($query);
        $user = array();
        while($users = $all_users->fetch_assoc()) {

            $temp_user = new User($users['id'], $users['email'], $users['username'], $users['password'], $users['rep']);
            $user[] = $temp_user;

        }

        return $user;

    }

}

Which allows me to do something like this:

$users = User::getUsers("SELECT * FROM users", $mysqli); // Returns an array of user objects containing each users details

Which I can loop through at will.

Is using a static function like this a good idea? What might be the pitfalls to my design? One that just occurred to me when writing this is that I might not always needs every user field, but it wouldn't be hard to customize the constructor to allow only desired fields to be assigned values.

Here is what I was doing BEFORE the previous example. Is this technically better? The thing that was bothering me is there are several occasions where I'm duplicating the block of code below to instantiate an array of objects. Should I create another class to take care of this process, or am I still going about this the wrong way?

$all_users = $mysqli->query($query);
$user = array();
while($users = $all_users->fetch_assoc()) {

    $temp_user = new User($users['id'], $users['email'], $users['username'], $users['password'], $users['rep']);
    $user[] = $temp_user;

}

class User {

    private $id;
    private $email;
    private $username;
    private $password;
    private $rep;

    public function __constructor($id, $email, $username, $password, $rep) { // Assign all 'required' fields for our object upon instantiation

        $this->id = $id;
        $this->email = $email;
        $this->username = $username;
        $this->password = $password;
        $this->rep = $rep;

    }

    public function getID() {

        // etc

    }

    public function setPassword($password) {

        // etc

    }

}
\$\endgroup\$

2 Answers 2

6
\$\begingroup\$

No, this is not OO code.

You are missing some of the key parts of OO.

Data Hiding

All of the class properties are public. Use protected for id, email, username etc. This stops outsiders from modifying the object without your control. The object should only be modified using the public interface that you provide for it.

Encapsulation

Did you implement a public interface for your object, while hiding implementation details away from the programmer who will use your object?

I think the answer is that you haven't really provided a complete interface here. Unfortunately your one method is misleading. Your class is a User class, however it returns Users (plural). This class actually looks like a DataMapper for users rather than a User class. This also leads me to think that you don't need the properties in the object as you will likely just want to be returning the results.

Static

OO and static methods do not mix. You kind of hit it with your usage example. It will work without you having any object created.

$users = User::getUsers("SELECT * FROM users", $mysqli);

Observe how this is really just calling a function. None of the class properties are used within the method.

I don't believe there is any place for static methods in OO. Also, static causes a tight coupling in your code. Look at your usage, what will you need in place to run that code? See if you come up with the same answer as me before you read on.

The things you will need to run the static code are: A class named User with a method named getUsers.

Now, look at the following call to an object method and tell me what you need:

$user->getUsers("SELECT * FROM users", $mysqli);

Answer: An object with a getUsers method. You have just removed your dependency on the User class. Now if you want to you can pass in a SuperUser object and get super users. This is an example of polymorphism which is one of the key concepts of OOP.

Testability

By calling User::getUsers in your code you have a hard-coded break out of the unit, making the code very difficult to test as a unit. Using an object allows you to pass in a special object to help you test.

\$\endgroup\$
6
  • \$\begingroup\$ Hey Paul, thanks for taking the time to answer. Please see the part I added to the bottom of the question in light of what you said. \$\endgroup\$
    – Anonymous
    Apr 19, 2012 at 10:40
  • \$\begingroup\$ Yes, I think the one you just posted (your previous version) was good OO. Its a bit hard to know what will be best without knowing the full details of what you are trying to achieve. An important thing will be that you match the objects in your solution to the problem at hand. Also, staying clear of static will definitely be beneficial. Good luck! \$\endgroup\$
    – Paul
    Apr 19, 2012 at 10:53
  • \$\begingroup\$ Thinking about it a bit more, maybe a Data Mapper class to retrieve all of the users is what you want. Search for Data Mapper pattern. \$\endgroup\$
    – Paul
    Apr 19, 2012 at 10:56
  • 2
    \$\begingroup\$ Static methods don’t contradict OOP, and both can be used in conjunction. Having a static method here is totally fine. Saying that there is no place for static methods in OOP is simply wrong. This would preclude constructors which are just static methods in disguise. Put differently, OP’s getUsers is a constructor in disguise. Again: this is a totally valid pattern. \$\endgroup\$ Apr 19, 2012 at 13:14
  • \$\begingroup\$ @KonradRudolph If you can avoid tight coupling and can unit test easily with static methods then let me know. I am in the PHP chat room frequently. \$\endgroup\$
    – Paul
    Apr 19, 2012 at 14:47
1
\$\begingroup\$

I think you'd benefit from taking a look at the documentation for the 2 major PHP ORMs (Object Relational Mapping) Doctrine and Propel to get some ideas on how to approach it.

They both provide classes that map to a record (row) and classes for interacting with the db tables (Doctrine call theirs "Table" classes, Propel calls them "Peer" classes).

When you need data, you call a method in the table/peer class, and that returns a collection of objects containing your data.

\$\endgroup\$

Your Answer

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

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