1
\$\begingroup\$

I was given a API project by my co-worker to work on half created a system in slim PHP. The project consists of the Models and controllers. When the route is called it calls the Controller with the necessary function in the Controller, then query data from the database in Model functions and get back the response.

I want to know is this the correct way to implement OOP in PHP API applications? If not what are the changes and components I have to do to make it OOP MVC app?

User Controller -> UserController.php

public function getUser($request, $response, $args)
{
    $userId= $request->getAttribute('id');

    $user = $this->User->getUserById($userId);

    if (!$user) {
        return $response->withJSON([
          "error" => true, 
          "message" => "cannot get user info", 
          "data" => null
        ]));
    }

    return $response->withJSON([
        "error" => false, 
        "message" => "cannot get user info"
        "data" => $user
    ]);
}

User Model -> User.php

public function getUserById($userId)
{
    $sql = "SELECT id, name, email
            FROM users
            WHERE id= :id";

    try {
        $stmt = $this->db->prepare($sql);
        $result = $stmt->execute([
            'id' => $userId
        ]);
        if ($result && $stmt->rowCount() == 1) {
            return $stmt->fetchAll()[0];
        } else {
            return false;
        }
    } catch (\PDOException $e) {
        $this->logger->info("cannot get user by id from DB " . $e);
        return false;
    }
}
\$\endgroup\$
9
  • 3
    \$\begingroup\$ The current question title of your question is too generic to be helpful. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$ – BCdotWEB May 8 '20 at 10:07
  • 1
    \$\begingroup\$ Welcome to Code Review. Did you test this? Does it produce the correct output? \$\endgroup\$ – Mast May 8 '20 at 10:09
  • \$\begingroup\$ Hi I changed the title. \$\endgroup\$ – camille May 8 '20 at 10:12
  • \$\begingroup\$ The code works.. I'm concerning about the design \$\endgroup\$ – camille May 8 '20 at 10:12
  • \$\begingroup\$ added the functionality in title \$\endgroup\$ – camille May 8 '20 at 10:21
3
\$\begingroup\$

There is not much OOP to review it. Actually that's just two functions, so one can review only the code, not the object structure. All in all there is a lot of repetitive or outright useless code

public function getUser($request, $response, $args)
{
    $userId= $request->getAttribute('id');
    $user = $this->User->getUserById($userId);
    return $response->withJSON([
        "error" => !$user, 
        "message" => $user ? "success" : "no such user"
        "data" => $user ?: null
    ]);
}

as you can see, there could be just one return statement

public function getUserById($userId)
{
    $sql = "SELECT id, name, email FROM users WHERE id= :id";
    $stmt = $this->db->prepare($sql);
    $result = $stmt->execute(['id' => $userId]);
    return $stmt->fetch();
}

I took out all the useless code, namely

  • it is useless to verify $result. In case of error an exception will be thrown
  • it is always useless to check the rowCount()
  • why use fetchAll() if you need only one row?
  • it is useless to add any condition because fetch() will return FALSE already if no record found.
  • the same goes for the try-catch. There should be a site-wide error handler that will log the error and do something else like show a generic 500 error in the browser
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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