Should I keep "redirect only" methods in my Controller?

Question

Controller:

function indexAction()
{
    if ($condition)
        $this->renumPosition($id);    //LINE #1
}

//Redirect only - function's sole purpose is to call another function
function renumPosition($id)
{
    $this->repository->renumPosition($id);
}

Repository:

function renumPosition($id)
{
    //SQL, DQL, ORM code follows
    ...
}

Question

Should Line #1 (above) be instead

$this->repository->renumPosition($id);

and should the renumPosition within Controller be removed?

The question is .. when do I defer execution directly to my model/repository layer, and when do I call a Controller's function which defers to model/repository layer?

This perhaps is a really subtle question. Just to note, my intent here is NOT to save an extra function call, but to align myself with a programming philosophy.

For example, I can make an argument that when Controller should be cognizant of an action, I should have a public method in Controller that may make a call to repository. But then, when it is an internal action, maybe I can call repository directly without making a separate method call in Controller.

Does that make sense?

What my Goal/Concern is

Here is the Legacy code I started with:

Controller:

function indexAction()
{
    if ($condition)
        $this->renumPosition($id);    //LINE #1
}

function renumPosition($id)
{
    //Heavy SQL, DQL, ORM code follows - say 50-70 lines
    ...

    $db->db_function($sql)

    ...

    $db->db_function($moreSql)

    //some more code here that relates to DB transactions
    $db->db_function($moreSql)
}

As the code above was mostly storage related, I moved it all into my Repository class. I ended up with this in my Controller:

function renumPosition($id)
{
    $this->repository->renumPosition($id);
}

I looked at the above and said to myself -- why do I have a function in my Controller that has a sole purpose of calling another function? Why have this indirection? Why have code cluttering my cognitive and screen space when I can totally and completely eliminate it. And so I did. I called the repository function directly and removed Controller's "redirection" function entirely. But then I asked myself -- Did I do the right thing? That's what I am trying to figure out with my question.

Answer 1

Yes, you did the right thing by removing the function renumPosition from your Controller.
Functions must have a reason to exist and just the fact there was a legitimate function before you started to refactor is not a valid reason to keep the function around after you have refactored its reasons for existence away.

There are two main reasons to create functions:

1. To reduce code duplication. If a piece of functionality is needed at multiple places in the code, then encapsulating that functionality in a function means that you don't have to duplicate that code all over the place (and find it back if it needs a change or bug fix). When creating functions for this reason, you should also keep the next bullet in mind.
2. To create an abstraction. If a piece of functionality consists of multiple operations, it can be helpful in understanding that block of code by giving it a name. The most convenient way of giving a block of code a name is by creating a function with that name that contains the code.

In the original code, the renumPosition function in the Controller created an abstraction over the database operations and had a reason for existing due to that.
After you moved the database interaction to the Model, renumPosition in Controller did not provide a useful abstraction anymore and you can hardly argue that the code duplication increases if you replace a function call to function A with a function call to function B.

Answer 2

I am not entirely sure what are you trying to achieve here. If your reason is code duplication in Controller Actions, you should look into a Service layer that sits between your controller and repository.

From your controller you would call a service function _userService.GetUser(5) and then you can use this function throughout your controller. The GetUser(int id) function would handle anything else that it needs to do, including making a call to repository, mapping the data to view model or whatever other logic you want.

Answer 3

There are two schools of thought when it comes to where the most logic should be placed in an MVC application: 1) the Controller should be stupid and most of your logic should be in the Model; 2) the Model should be stupid and most of your logic should be in the Controller.

Both ways have their up and down sides.

In the end, it doesn't matter which way you choose as long as you are consistent.

Stupid Controller Method

This method makes your Controller act as a traffic cop for all of your Models.

Your Controller methods would instantiate Models, and then pass control to the Model for further processing. Error states (including validation failures) in the Models would require an error to be raised back to the Controller (and eventually the View). Your application's business logic would then be contained in the Models.

In this method, the Model would also be responsible for this like logging either by creating a Logging Model.

Stupid Model Method

This method basically makes your Model not much more than a data validator and storage mechanism.

Your Controller methods would still instantiate Models, set properties and fields, check for validation failures or other error states, and tell the Model to save it's data to storage. Error states could be immediately sent back to the View. Your application's business logic would be contained in the Controller.

In this method, the Controller would also be responsible for things like logging by creating a Logging Model.