1
\$\begingroup\$

I'm studying OOP and MVC structure, and all my controllers extends the main controller, and almost all of them has a delete method, basically this:

public function delete () {

        if( isset($_POST) && !empty($_POST) ) {

            $Validator = new Validator;
            $Validator->validation_rules(array(
                    'id'    => 'required|numeric',
            ));

            $validated = $Validator->run($_POST, true);
            if($validated === false) {
                    echo helperModel::toJson(
                    array(
                        'success'=>0,
                        'message'=>$Validator->get_readable_errors(true)
                    ));
            } else {
                $delete_contact = (new contactModel)->delete($validated['id']);
                if($delete_contact){
                    echo helperModel::toJson(array('success'=>1));
                }
            }
        }

    }

I created a variable in the child controller with the name $default_model to refer to the controller model.

Example: in the userController I use the userModel, and copied the structure of the method to the main controller. Instead of using the specific name of the model, I just put:

(new $this->$default_model)->delete($validated['id'])

Is it a correct/recommended practice? If not, what should I do?

\$\endgroup\$
0

1 Answer 1

1
\$\begingroup\$

Three part answer here: a quick answer, a more detailed answer with alternatives, and some specifics to your code.

Quick Answer

There isn't anything wrong with having abstract controller methods, anymore than there is to having abstract methods in general. That being said, the example you gave isn't an abstract method: it is simply a parent method. An abstract method is a method without a definition that must be defined by the child class. That would be a bad idea in your use case, because there might be cases when you don't need to delete entries but would have to provide a definition anyway. You are simply looking to implement some commonly used functionality in your parent class for easy-reuse, which is perfectly reasonable. The fact that this is a controller doesn't change the overall picture at all, although there are a few caveats to keep in mind.

Finally, having the behavior of the parent method change based on definitions in the child method is perfectly reasonable to. I use that pattern myself frequently (I'm referring to the definition of $default_model in the child class). That could be considered an abstract property, except that PHP doesn't have such a thing. As a result the only issue to watch out for there is the possibility that the developer that uses this system forgets to define the necessary properties. There are two good ways to avoid that:

  1. Instead of using a property, use an actual abstract method, which is supposed to return a string with the same value
  2. Inside the parent method, check for a correctly defined $default_model and throw a detailed exception if it is missing: 'Cannot execute delete method for class ' . get_class($this) . ' because $default_model is not defined' This way if you try to use this later and forget to declare all the properties, you get a clear and immediate warning telling you exactly what you did wrong.

Long Answer

Let's consider your goal: you have lots of controllers to implement lots of CRUD operations. (for example) You need to CRUD users, you need to CRUD Customers, more CRUDs for Orders, products, categories, and so forth. After putting together your second controller filled with repetitive CRUD methods, you really should be wondering "what can I do to avoid repeating myself so much" which is what you are really asking here. You are trying to fix this problem by putting some basic CRUD functionality into the parent. As I said, that is a reasonable approach and can avoid a lot of repetition. It's worth mentioning the potential downsides:

  1. This is a non-starter if your framework automatically routes to public controller methods. This isn't the norm, but I've seen frameworks that do it. If your public controller methods are automatically valid routes in your system, then putting any public methods in a base class means that every controller that extends that base has all those routes by default. This is a very easy way to introduce accidental security vulnerabilities.
  2. Your current implementation will also cause you trouble if you ever need flexibility in how the delete method is called. I like the old adage "The only thing worse than no abstraction is the wrong abstraction". For instance, what if you have a public-facing user's page and the user should only be able to delete themselves? You won't be able to use this guy. The validation rules are fixed, the calling sequence is fixed, etc... Baking in a little more flexibility may be good, once you know what areas you need flexibility for.
  3. Although less of a problem, this can result in unnecessary code "living" in your controllers if you put it in the base but don't actually need it all the time.

I consider that first one to be a show-stopper, but the second one is one you can plan for, and the third one is not a big deal, so (again), what you are doing isn't crazy. There are a couple other ways to do this though, just because:

  1. You can use traits, and have a trait for each CRUD method. This way you only have to throw in the ones that you want.
  2. You could put the CRUD functionality in separate classes that get generated and called as needed. You could imagine having a deleteManager class which is passed in a model name (default_model) and the $_POST input, and which then does everything you have in your example. This will again give you more flexibility. Your controller only contains the functionality it needs, and you can have as many variations as you need without cluttering up your base class. Maybe you have one deleteManager that does exactly what you have here, another that extends it and adds some authentication checks, etc... Maybe not the best examples but you should get the idea.

A review of your code

This is code review, and while I certainly want to answer your immediate question, a big goal of this site is to review everything. So if you don't mind me making some other comments:

  1. Your indentation is off. You indent 8 spaces after the first line, and four spaces elsewhere. I'm sure it was just a mistake, and it is best to try to fix those things before posting here, but it is more important to fix it in your code. Following code formatting best practices makes reading code much easier, which makes debugging code much easier, which makes coding much easier. You'd be surprised how often the little things matter.
  2. You have a lot of inconsistencies in how you stylize your variable names and method names. Choose snake_case, camelCase, or TitleCase and stick to it. You use all three: Validator, validation_rules, toJson. If uncertain just use PSR.
  3. Your code doesn't properly addressing all possible branches. No action is taken if the data validates but the delete fails. This may cause problems for whatever client is calling this API endpoint.
  4. You aren't using dependency injection. That isn't exactly something that you fix overnight, but maintaining any size code base without automated tests (i.e. unit and integration testing) is a gigantic pain, and doing that requires dependency injection. It makes for better code anyway. Now is the time to start learning it.
  5. I think there is some real code smell here: helperModel::toJson. What is this helperModel and why does it have the job of converting to JSON? Sounds suspiciously like a God Class.
  6. I think some of your variables could be named better. Granted, naming variables is hard, but it is worth the effort. For instance, $default_model. That doesn't give much hint as to what it is, and presumably the variable itself doesn't contain the model but rather the name of the model. model_name might be better. Similarly delete_contact is probably better off as a simple contact. It is going to be deleted but it is still just a contact. That one might be a nitpick though.
  7. Finally, return early (aka guards).

I've loosely pulled together most of these suggestions for a counter-example:

public function delete(){
    if ($_SERVER['REQUEST_METHOD'] != 'POST'){
        echo 'error';
        return;
    }

    if(empty($_POST)){
        echo 'error';
        return;
    }

    // since the validator can't do anything without validation rules,
    // I would just pass them in to the constructor rather than
    // calling a separate method.
    $validator = new validator(array(
        'id'    => 'required|numeric',
    ));

    $validated = $validator->validate($_POST, true);
    if($validated === false){
        echo 'error';
        return;
    }

    $modeClass = $this->modeClass;
    $model = new $modelClass;
    if (!$model->delete($validated['id'])){
        echo 'error';
        return;
    }

    echo 'success';
}
\$\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.