1
\$\begingroup\$

I'm simply using Slim PHP MVC framework in my API project. PDO::FETCH_ASSOC is used in the database calls. So no data objects. The router handles requests and pass to the controller. controller calls the necessary model to do database operations and response is delivered.

This is the scenario that I want to handle different user types. Each user_type has user_type based specific db transaction to be done. I want to make this more reusable and remove code duplication.

Sorry if there's any wrong thing here, and for my English.

My Route

$this->post('/users/preferences', \UserPreferenceController::class . ':store')

UserPreferenceController => Controller

public function store($request, $response, $args){
    $parsedBody = $request->getParsedBody();
    $user_id = $parsedBody['user_id'];
    $preferences = $parsedBody['preferences'];

    $user = $this->User->getUserInfo($user_id);

    if ($user["type_id"] == 'Admin') {
        $preferenceAdded = $this->User->AddAdminUserPreferences($user_id, $preferences); // DB transaction operation
    } else if ($user["type_id"] == 'Customer') {
        $preferenceAdded = $this->User->AddCustomerUserPreferences($user_id, $preferences); // DB transaction Operation
    } else if ($user["type_id"] == 'Sales') {
        $preferenceAdded = $this->User->AddSalesUserPreferences($user_id, $preferences); // DB transaction Operation
    } 

    // return if the transaction is rollbacked
    if(!$preferenceAdded){
        return $response->withJSON([
            "error" => true, 
            "message" => "cannot add preferences for user"
            "data" => $user_id
        ]);
    }

    if ($user["type_id"] == 'Admin') {
        $staisticsAdded = $this->User->AddAdminUserPreferencesStatistics($user_id, $preferences); // DB transaction operation
    } else {
        $staisticsAdded = $this->User->AddOtherUserPreferencesStatistics($user_id, $preferences); // DB transaction operation
    }

    // return if the transaction is rollbacked
    if(!$staisticsAdded ){
        return $response->withJSON([
            "error" => true, 
            "message" => "cannot add statistics for user"
            "data" => $user_id
        ]);
    }

    return $response->withJSON([
        "error" => false, 
        "message" => "operation completed successfully"
        "data" => null
    ]);
}

User.php => User model

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

public function AddAdminUserPreferences($user_id, $preferences)
{
    $this->db->beginTransaction();

    try {
        // $sql1 execute (common function for every user_type)
        // rollback if $sql1 fails

        // $sql2 execute (a user_type specific function)
        // rollback if $sql2 fails

        $this->db->commit();
        return true;
    } catch(\PDOException $e){
        $this->db->rollBack();
        return false;
    }
}

My Container

$container = $app->getContainer();

$container['UserPreferenceController'] = function ($c) {
    return new App\Controllers\UserPreferenceController($c);
};

$container['User'] = function ($c) {
    return new App\Models\User($c);
};
\$\endgroup\$
7
  • \$\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 13 '20 at 9:17
  • \$\begingroup\$ How different Add Preferences functions' code differ? Can't you just have a single function AddUserPreferences? \$\endgroup\$ – Your Common Sense May 13 '20 at 9:19
  • \$\begingroup\$ @YourCommonSense.. the Add Preferences function have a common function which insert data into the same table and also a user specific function which inserts to a user specific table \$\endgroup\$ – camille May 13 '20 at 9:25
  • \$\begingroup\$ A question. Shouldn't setting preferences and statistics be wrapped in a common transaction? Would it make sense to have preferences set but statistics not? \$\endgroup\$ – Your Common Sense May 13 '20 at 10:58
  • 1
    \$\begingroup\$ @YourCommonSense it can be. But some future user types (future requirement) may not want set statistics function. \$\endgroup\$ – camille May 13 '20 at 11:19
2
\$\begingroup\$

I think it's a very good case to employ OOP, namely polymorphism and inheritance.

First, we've got to create a dedicated class to handle the user preferences.

A common ancestor to hold all the common code

abstract class UserPreferences
{
    protected $userId;
    protected $db;

    public function __construct($userId, $db) {
        $this->userId = $userId;
        $this->db = $db;
    }
    protected function addGeneric($preferences) {
        $sql = "INSERT INTO preferences (...) VALUES (?,?,?)";
        $this->db->prepare($sql)->execute($preferences);
    } 

    abstract protected function addSpecific($preferences);

    public function add($preferences)
    {
        $this->db->beginTransaction();

        try {
            $this->addGeneric($preferences);
            $this->addSpecific($preferences);
            $this->db->commit();
        } catch(Throwable $e){
            $this->db->rollBack();
            throw $e;
        }
    }
}

and then implementations for different types

class AdminPreferences extends UserPreferences 
{
    protected function addSpecific($preferences) {
        $sql = "INSERT INTO admin_preferences (...) VALUES (?,?,?)";
        $this->db->prepare($sql)->execute($preferences);
    }
} 

class SalesPreferences extends UserPreferences 
{
    protected function addSpecific($preferences) {
        $sql = "INSERT INTO sales_preferences (...) VALUES (?,?,?)";
        $this->db->prepare($sql)->execute($preferences);
    }
} 

...and so on.

Then in the User class create an instance of the Preferences class

class User 
{
    public $preferences;
    public function __construct($db, $type) {
        $this->userId = $userId;
        $this->db = $db;
        $this->preferences = $this->setPreferences($db, $type);
    }
    protected function setPreferences($db, $type) {
        switch($type) {
            case 'Admin':
                $this->preferences = new AdminPreferences($db);
                break;
            case 'Sales':
                $this->preferences = new SalesPreferences($db);
                break;
        }
    }
}

the same goes for the statistics

and finally in your controller simply

$preferences = $parsedBody['preferences'];
$this->User->preferences->add($preferences);
$this->User->preferencesStatistics->add($preferences);

return $response->withJSON([
    "error" => false, 
    "message" => "operation completed successfully"
    "data" => null
]);

Note that checking $preferenceAdded manually is not the way to go. There should be a dedicated error handler to do the job.

\$\endgroup\$
4
  • \$\begingroup\$ hi @your-common-sense thanks for the answer. Seems like I have lot of refactoring to do. My User model class consists only with database functions and not any attributes. Also I have several controllers functions (in other controllers) which needs to handle those specific user types. Do you have any tip for handling those user types in App Level? \$\endgroup\$ – camille May 13 '20 at 12:11
  • 1
    \$\begingroup\$ You asked for it :) The main idea of this refactoring is to reduce the reprtitions. The "common function for every user_type" is written only once and then reused. Also the final calling code in the controller is also greatly reduced. As ot the other controllers, it's just the same way as in this controller. Add new public methods to Preferences classes and call these methods. \$\endgroup\$ – Your Common Sense May 13 '20 at 12:26
  • \$\begingroup\$ One final question, Is it okay to write functions with MySQL queries inside User class? \$\endgroup\$ – camille May 13 '20 at 12:35
  • 1
    \$\begingroup\$ It's entirely up to you. There are two competing approaches, ActiveRecord(AR) where it's OK to have the database interaction in the User class, and DataMapper(DM) where a satellite class is used, UserMapper to hold all the database operations for the User class. \$\endgroup\$ – Your Common Sense May 13 '20 at 13:00
1
\$\begingroup\$

I'll start by listing some notes on the code itself and then I'll suggest a new architecture that I think will help improve the 'maintainability' of the code.

  1. As @YourCommonSense said, the AddPreferences methods should be only one method that will take the user_id and preference_id and add it to the database, and this will be it's sole purpose. Also what if you got new roles? What if you got new preferences? You will have to add new functions for each one of these, the best way is to go abstract and generalize the thing, later on when you add new preferences, the method doesn't care, it just takes the user_id and the preference_id.

  2. Throw Exceptions, and let the controller handle them, and then return exceptions with error codes. Don't return that error flag, message. So for example let's say you wanted to add a a preference to a user that already exists, this should throw an exception from the AddPreferences method, the Controller will handle the exception and then return a response with status_code = 400 // for example, maybe you want another code to represent that and the exception message that was thrown, this way you can handle things better in the front end and debug with ease.

  3. Why not move the database logic to a separate generic class and use it, maybe you can create a class that you can extend and give it the table_name, and this way it can control all database operations specific to that table, just like ORM models, you can search for ActiveRecord or Eloquent to learn more.

  4. Don't catch Exceptions in low layers unless you have another option to use of their fail, otherwise just throw them and let them bubble. For example, int the AddAdminUserPreferences, you caught the exception and returned false, okay, but how should I know what has gone wrong? How should the user know? What will happen next?

So for the Architecture, Please search MVC, there is plenty of resources out there, and after that, there is there repository pattern which will do a really good job in separating the concerns, leaving the database layer away from business logic.

As a side note, I'd suggest you look at 'lumen', it a pretty REST framework, very easy to use and has a wide range of things, including a smart Service Container and Discovery, so you wont need to bind (almost)anything, it also has a really good ORM and Query builder.

EDIT: Example on the 2nd Point.

instead of

// return if the transaction is rollbacked
    if(!$preferenceAdded){
        return $response->withJSON([
            "error" => true, 
            "message" => "cannot add preferences for user"
            "data" => $user_id
        ]);
    }

You can do this

// return if the transaction is rollbacked
    if(!$preferenceAdded){
        return $response->withJSON([
            "message" => "cannot add preferences for user"
        ])->statusCode(400);  // BAD_REQUEST, and I'm not sure if this is how to set a statusCode, sorry.
    }

Now you are in the browser and sent your request, the old way is you will check whether the error flag is true or false, and act based on that, and you will repeat that check in every call to the api, and the method of sending an error flag itself is not a reliable(and i don't like it, not beautiful :) ).

but if you return a status_code = 400 // or anything other than success codes, this will throw exception in the request you made from the browser, say you're using fetch.

fetch('www.example.com').then(response => response.json()).catch(e => {
    // Do something with the exception
})

Instead of

fetch('www.example.com').then(response => {
    response = response.json()
    if(response.error) {
        // Do something with the error
    } else {
        // Do something with the response
    }
}).catch(e => {
    // Do something with the exception
})

See How you separated the response logic from the code logic?.

\$\endgroup\$
4
  • \$\begingroup\$ hi @mahmoud, Thanks for the great answer. I didn't actually get the 2nd point. Could you be able to give me an example for that. \$\endgroup\$ – camille May 13 '20 at 12:12
  • \$\begingroup\$ Hi @camille I've updted my answer with an example, please see if you have any questions. \$\endgroup\$ – Mahmoud Farouq May 13 '20 at 12:21
  • \$\begingroup\$ When a database fails, it's rather a server error (500), not a request error (400) \$\endgroup\$ – Your Common Sense May 13 '20 at 12:29
  • \$\begingroup\$ Well, there might be any Kind of errors, notice // or anything other than success codes, you can return what ever you want, just not a success code such as 200 or 201 \$\endgroup\$ – Mahmoud Farouq May 13 '20 at 12:31

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.