My models all tend to look the same

Question

I've noticed that all my models look very similar. Most of them tend to follow a pattern where they are collections of methods containing active record code that are just slight variations on one another. Here is an example:

class Site extends CI_Model {

    public function get_site_by_id($id)
    {
        // Active record code to get site by id
    }

    public function get_sites_by_user_id($user_id)
    {
        // ...
    }

    // ...

    public function get_site_by_user_id_and_url_string($user_id, $url_string)
    {
        // ...
    }

    // Non active record methods and business logic
    // ...

}

This approach has worked fine for me but I'm wondering if there is a more elegant solution. It just doesn't seem right to me that I should have to create a new method every time I need to look up data in a new way. Is this common practice or am I missing a way to refactor this?

This would mostly be a side-effect of using a group of [active record](http://martinfowler.com/eaaCatalog/activeRecord.html) as substitute for full implemented mode layer. You might find [this](http://stackoverflow.com/a/11943107/727208) relevant for options of refactoring. — tereško, Nov 22 '12 at 01:04

score 2 · Accepted Answer · edited Dec 02 '11 at 15:31

Strictly following your request, you could add an intermediate class between the main model class (CI_Model) and your class (Site), something like

class MyCommonMethodsClass extends CI_Model {
}

and you would extend it in your classes (Site), while putting the common code on it. That would work and could be somehow'elegant'. In fact at the end you would end up adding your basic crud, site adapted actions, to it.

Now, if that's 'clean', that's another thing. Again, strictly speaking the model does that. It takes care of common and 'advanced' getters. And yes, they almost always have the tendency to have the same code all around your website. The problem is that, although that looks nice in your code (less code) you're technically sacrificing abstraction between your business logic and the db. Are you a model purist or practical one ?

+1 for "purist" vs. "practical" - too many times, people make more work for themselves trying to do things the "correct" way. — swatkins, Dec 02 '11 at 15:05
Yep thats correct, lets slap an application together and wait for the problems to come knocking. — Sean H Jenkins, Dec 02 '11 at 18:51

score 1 · Answer 2 · answered Dec 02 '11 at 14:54

I think this is matter of opinion but I think best practice is to create some sort of Create, Retrieve, Update, Delete (CRUD) model which does many basic SQL functions like GetID, UpdateByID, GetById and so on.

CRUD models can only go so far in helping you with more modular queries. But it makes sense to call a function called GetId and pass it some parameters than to have different functions for each table.

As I say though, CRUD's can only go so far. For example it would make sense to have a function that queries a database users table to check if a user has verified and username & password match. As this is a unique and not an abstract function, it should have it's own function defined.

Also as a best practice, Logic and Database access should never be mixed in the same file.

score 0 · Answer 3 · answered Dec 02 '11 at 15:03

0

It is common practice to have different methods to handle getting your data like that. The Single Responsibility Principal states that every object should only do one thing, by making multiple methods that get very specific data you are creating very maintainable and easy to debug code.

answered Dec 02 '11 at 15:03

Bob The Janitor

19,164
9
45
71

Maybe for functions that tend to do specific things but not for very abstract functions that can apply to many different situations. – Sean H Jenkins Dec 02 '11 at 15:08
I also think you mis-interept what they mean by 'Single responsibility', this means that the function has one action, like a singleton that activates a database connection, but can still be re-used. 'Single responsibility' is a part of encapsulation in OOP, encapsulation being the principle of re-usable code. – Sean H Jenkins Dec 02 '11 at 15:12
Models are suppose to be very specific, that is the point of a model, the "Single Responsibility" of the model is a specific check of the domain, with in the model you have methods that do a specific task. If you have multiple models that all do the same thing then you probably have code smell, then you move the common code to a new class then extend your model with the common class. – Bob The Janitor Dec 02 '11 at 18:14

score 0 · Answer 4 · answered Dec 02 '11 at 15:36

If you have multiple classes that are providing essentially the same functionality, then this would suggest that there may be something wrong with your class hierarchy (a so-called "code smell"). If they have similar interactions then that suggests that they are related in some way. If that's the case then the chances are they should all be inheriting from a common superclass that implements the functionality common to all your subclasses, with each subclass simply specializing the generalized functionality of the superclass.

The advantages of this approach are:

You're not repeating work (SPOT, DRY)
Code that interacts with the classes can be written in a more general way and can handle any object that inherits from the superclass (substitution)

score 0 · Answer 5 · edited May 23 '17 at 11:55

I do not think there is any thing wrong with creating an 'base' model class to extend you other models by. If it is solid and well tested, it can make you life easier. What is the point of creating the same CRUD functions over and over again?

Another benefit of doing it is that you can have a base development repository that you clone to start all new projects.

If you need an example of how to do this then look at a question I previously asked.

You can also do the same with your controllers.

My models all tend to look the same

5 Answers5