PHP - How can I refactor this store function (which handles different database operation for user types) in order to reduce duplication?

Question

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);
};

Answer 1

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.

Answer 2

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?.