2
\$\begingroup\$

Overview

We are learning PHP OOP and decided to create a simple app with 3 database tables, one a basic users table, the other two containing information about the users' favorite food and favorite restaurant.

To help help others learn this and other best practices a bit easier, we welcome PRs, issues, and other contributions to our GitHub repository.

Goal/Output

Our current goal is to create two pages:

Page 1 output:

"Welcome to the app, Jenn"

<?php
namespace PHPBestPractices1OOP\Controller;
use PHPBestPractices1OOP\Request\Request;
use PHPBestPractices1OOP\Response\Response;
use PHPBestPractices1OOP\Domain\User\UserFactory;
class GreetSingleUserPage
{
    /**
     * @var Response
     */
    private $response;
    /**
     * @var Request
     */
    private $request;
    /**
     * @var UserFactory
     */
    private $userFactory;
    /**
     * GreetSingleUserPage constructor.
     * @param Request $request
     * @param Response $response
     * @param UserFactory $userFactory
     */
    public function __construct(
        Request $request,
        Response $response,
        UserFactory $userFactory
    ) {
        $this->response = $response;
        $this->request = $request;
        $this->userFactory = $userFactory;
    }
    public function __invoke()
    {
        // create user object
        $userObject = $this->userFactory->createUser(2);
        // this will display greeting message to the user
        $this->response->setView("greetSingleUser/index.php");
        $this->response->setVars(
            array(
                "name" => $userObject->getName()
            )
        );
        return $this->response;
    }
}

Page 2 output:

"My name is Jenn, age is 28, favorite restaurant is KFC and favorite food is Fried Chicken"

<?php
namespace PHPBestPractices1OOP\Controller;
use PHPBestPractices1OOP\Request\Request;
use PHPBestPractices1OOP\Response\Response;
use PHPBestPractices1OOP\Domain\User\UserFactory;
class DisplayUserInformationPage
{
    /**
     * @var Response
     */
    private $response;
    /**
     * @var Request
     */
    private $request;
    /**
     * @var UserFactory
     */
    private $userFactory;
    /**
     * DisplayUserInformationPage constructor.
     * @param Request $request
     * @param Response $response
     * @param UserFactory $userFactory
     */
    public function __construct(
        Request $request,
        Response $response,
        UserFactory $userFactory
    ) {
        $this->response = $response;
        $this->request = $request;
        $this->userFactory = $userFactory;
    }
    public function __invoke()
    {
        // create user object
        $userObject = $this->userFactory->createUser(2);
        // this will display the user's information
        $this->response->setView("displayUserInformation/index.php");
        $this->response->setVars(
            array(
                "name" => $userObject->getName(),
                "age" => $userObject->getAge(),
                "restaurant" => $userObject->getFavoriteRestaurantName(),
                "food" => $userObject->getFavoriteFoodName()
            )
        );
        return $this->response;
    }
}

Database

In practice these are sql databases, note though if you happen to look at our github we've "faked" this db using PHP arrays to make the project easier to run and try out

users
id    name     age    favoriteRestaurantId   favoriteFoodId
1     Mike     30     1                      1
2     Jenn     28     3                      3

restaurants
id    name            averagePrice     style
1     McDonalds       1                American
2     Taco Bell       1                Mexican
3     KFC             2                American

foods
id    name            caloriesPerOunce
1     French Fries    100
2     Hamburger       135
3     Fried Chicken   97

User class

Here is our User.php. We also have Restaurant and Food classes.

<?php
namespace PHPBestPractices1OOP\Domain\User;
use PHPBestPractices1OOP\Domain\Food\Food;
use PHPBestPractices1OOP\Domain\Restaurant\Restaurant;
class User
{
    // region vars
    /**
     * @var int
     */
    private $userId;
    /**
     * @var string
     */
    private $name;
    /**
     * @var int
     */
    private $age;
    /**
     * @var Restaurant
     */
    private $favoriteRestaurant;
    /**
     * @var Food
     */
    private $favoriteFood;
    // endregion
    // region set
    /**
     * @param $userId
     */
    public function setId($userId)
    {
        $this->userId = $userId;
    }
    /**
     * @param string $name
     */
    public function setName($name)
    {
        $this->name = $name;
    }
    /**
     * @param int $age
     */
    public function setAge($age)
    {
        $this->age = $age;
    }
    /**
     * @param $favoriteRestaurant
     */
    public function setFavoriteRestaurant(Restaurant $favoriteRestaurant)
    {
        $this->favoriteRestaurant = $favoriteRestaurant;
    }
    /**
     * @param $favoriteFood
     */
    public function setFavoriteFood(Food $favoriteFood)
    {
        $this->favoriteFood = $favoriteFood;
    }
    // endregion
    // region get
    public function getId()
    {
        return $this->userId;
    }
    /**
     * @return string
     */
    public function getName()
    {
        return $this->name;
    }
    /**
     * @return int
     */
    public function getAge()
    {
        return $this->age;
    }
    /**
     * @return Restaurant
     */
    public function getFavoriteRestaurant()
    {
        return $this->favoriteRestaurant;
    }
    /**
     * @return string
     */
    public function getFavoriteRestaurantName()
    {
        return $this->favoriteRestaurant->getName();
    }
    /**
     * @return string
     */
    public function getFavoriteFoodName()
    {
        return $this->favoriteFood->getName();
    }
    /**
     * @return Food
     */
    public function getFavoriteFood()
    {
        return $this->favoriteFood;
    }
    // endregion
}

UserFactory

<?php
namespace PHPBestPractices1OOP\Domain\User;
use PHPBestPractices1OOP\Domain\UsersTransactions\UsersTransactions;
use PHPBestPractices1OOP\Domain\Restaurant\RestaurantFactory;
use PHPBestPractices1OOP\Domain\Food\FoodFactory;
class UserFactory
{
    /**
     * @var UsersTransactions
     */
    private $usersTransactions;
    /**
     * @var FoodFactory
     */
    private $foodFactory;
    /**
     * @var RestaurantFactory
     */
    private $restaurantFactory;
    /**
     * UserFactory constructor.
     * @param UsersTransactions $usersTransactions
     * @param FoodFactory $foodFactory
     * @param RestaurantFactory $restaurantFactory
     */
    public function __construct(
        UsersTransactions $usersTransactions,
        FoodFactory $foodFactory,
        RestaurantFactory $restaurantFactory
    ) {
        $this->usersTransactions = $usersTransactions;
        $this->foodFactory = $foodFactory;
        $this->restaurantFactory = $restaurantFactory;
    }
    /**
     * create an instance of User class
     *
     * @param int $userId
     * @return User
     */
    public function createUser($userId)
    {
        $userObject = new User();
        // get user data from the db and set to User object
        $userRow = $this->usersTransactions->getUserById($userId);
        $userObject->setName($userRow["userRow"]["name"]);
        $userObject->setAge($userRow["userRow"]["age"]);
        // create restaurant object and set user's favorite restaurant
        $restaurantObject = $this->restaurantFactory->createResturant($userRow["userRow"]["favoriteRestaurantId"]);
        $userObject->setFavoriteRestaurant($restaurantObject);
        // create food object and set user's favorite food
        $foodObject = $this->foodFactory->createFood($userRow["userRow"]["favoriteFoodId"]);
        $userObject->setFavoriteFood($foodObject);
        return $userObject;
    }
    /**
     * create collection of User class instances for user's ids
     *
     * @param array $userIdsArray
     * @return array
     */
    public function createUsersCollection($userIdsArray)
    {
        // users collection
        $usersCollection = array();
        // loop through users array
        foreach ($userIdsArray as $userId) {
            $usersCollection[] = $this->createUser($userId);
        }
        return $usersCollection;
    }
}

Our Question

Our current User class works fine, but is slower than it could be for Page 1. Page 1 only requires the user's name, Jenn, which is data in the users table. Yet we still have to get data from the restuarants and foods tables in order to create our user object.

We imagine that in the future half of our app's pages only need info from the users table, while the other half of our app's pages need info from all 3 tables. Since we want our app to be fast and responsive, would it be a good idea to have our user class only include things from the users table, and then maybe create another class UserDetail which includes the Food and Restaurant objects? On pages that we only need to display basic user table info, we use our User class, and on pages where we need to display info about their favorite food and restaurant we use UserDetail class? Or is there some other, better way to do this?

\$\endgroup\$
8
  • \$\begingroup\$ You've only posted part of a User class, which is, in my opinion, too sketchy to qualify for a code review according to the rules in the help center. \$\endgroup\$ Commented Sep 18, 2018 at 20:49
  • \$\begingroup\$ @200_success did the same with our previous question and it seemed to work well: codereview.stackexchange.com/questions/203261/… Have also linked to the full code. Should I really copy all of the code here? \$\endgroup\$ Commented Sep 18, 2018 at 20:52
  • 1
    \$\begingroup\$ Your other question was a bit on the sketchy side, but at least the "Current Usage" section was a real reviewable excerpt. This one is just a skeleton with some "what do we do now?" questions. \$\endgroup\$ Commented Sep 18, 2018 at 20:54
  • \$\begingroup\$ "Sketchy" or not, it very clearly got the point across, resulting in upvotes, stars, and a clear, helpful answer. I think my question here is pretty clear as well, but I'll pour in the unncessary parts of the user class if that makes you happy. Imo it makes the problem less easy to understand, adding many lines of unncessary code which is not complete anyway. \$\endgroup\$ Commented Sep 18, 2018 at 20:57
  • \$\begingroup\$ I'm OK with inferring what the User class code looks like based on a small excerpt, but including the GreetSingleUserPage is absolutely essential to the question, if that's the code that you want to have reviewed. Thanks for adding it. \$\endgroup\$ Commented Sep 18, 2018 at 21:02

1 Answer 1

1
\$\begingroup\$

Pages

You can get rid of a lot of boilerplate with constructor promotion (PHP 8.0):

// imports

abstract class Page
{
    public function __construct(
        protected readonly Request $request,
        protected readonly Response $response,
        protected readonly UserFactory $userFactory,
    ) {}

    abstract public function __invoke(): Response;
}

Due to similarities between your page classes, this is an abstract class that they should both inherit from. It also uses readonly properties (PHP 8.1) and a return declaration.

User.php

You might as well replace almost all your getters and setters with public properties as you're not doing any logic (just give them all types too). The exceptions are the ones that get names, though I question the utility of this when you can just call (for example) $user->favoriteRestaurant->getName().

Alternatively, instead of public properties, maybe you could use the spread operator to easily pass your arguments to a constructor.

I don't see a reason to make more than one class for partial user objects. There's no speed benefit to it, since you could just not add the foreign data to the instance.

UserFactory

This could be slow because you're getting everything one at a time instead of all at once:

foreach ($userIdsArray as $userId) {
    $usersCollection[] = $this->createUser($userId);
}

That's also my thought with the other method. You're getting all the foreign data (presumably through a second call to the database) even when you don't need it.

Also, createUser doesn't create a user, it merely fetches it from the database. It would be more appropriate to call it get, and let the code tell you that it's returning a User with a return declaration.

You need more return declarations elsewhere, and don't forget to explicitly type the input to the function too (and it's possible to get a better doc comment):


/**
 * create collection of User class instances for users' ids
 *
 * @param int[] $userIdsArray
 * @return User[]
 */

public function createUsersCollection(array $userIdsArray): array
\$\endgroup\$

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.