Should same methods from different controllers be moved to a CommonController?

Question

I have two controllers that have few same methods:

public class Controller1 : Controller
{
    private readonly ITestBL bl;

    public Controller1(ITestBL bl)
    {
        this.bl= bl;
    }

    [HttpGet]
    public ActionResult Method1(string data)
    {
        using (bl)
        {
            var res = ...

            return Json(res, JsonRequestBehavior.AllowGet);
        }
    }

    [HttpGet]
    public ActionResult Method2(string data, int data2)
    {
        using (bl)
        {
            var res = ...

            return Json(res, JsonRequestBehavior.AllowGet);
        }
    }

    // other methods
}

And the second controller also has those two methods.

Should I create some common controller to keep those methods? So, it will look like this:

public abstract class CommonController: Controller
{
    private readonly ITestBL bl;

    protected Controller1(ITestBL bl)
    {
        this.bl= bl;
    }

    [HttpGet]
    public ActionResult Method1(string data)
    {
        using (bl)
        {
            var res = ...

            return Json(res, JsonRequestBehavior.AllowGet);
        }
    }

    [HttpGet]
    public ActionResult Method2(string data, int data2)
    {
        using (bl)
        {
            var res = ...

            return Json(res, JsonRequestBehavior.AllowGet);
        }
    }
}

And my Controller1 and Controller2 will be:

public class Controller1 : CommonController
{
        private readonly ITestBL bl;

        public Controller1(ITestBL bl)
            :base(bl)
        {
        }

        // methods
}

Is that the proper way to do that? Do I miss anything or is there a better way?

Yes, everything that is common should go, if possible, in a common base class, this avoids code duplication. — InBetween, Mar 05 '18 at 10:07
That said, your `using` statements don't look too good... they seem like a very bad idea to say the least. — InBetween, Mar 05 '18 at 10:09
@InBetween, should it be the way: "var res; using(bl) { res = ... ; } return res;" ? — hopeless, Mar 05 '18 at 10:11
No. First, don't dispose something you *don't own*. `bl` is being injected via the constructor, let whoever did that decide when to dispose it. Second, `bl` is *disposed* before your `Method1` or `Method2` exit (that's what the `using` statement is guaranteeing you). The first call to either one will work, on any subsequent calls `bl` will already be disposed which doesn't bode well for the outcome of the call, but like I said, you have no business disposing it to begin with. — InBetween, Mar 05 '18 at 10:13
@InBetween: Fully on board with the issue with `using`. However, due to MVC constantly remaking controllers to handle a single request, OP is likely not going to encounter any issues as long as he only disposes the object at the end of his action. Still not good practice, of course. — Flater, Mar 05 '18 at 10:29
Possible duplicate of [What are good candidates for base controller class in ASP.NET MVC?](https://stackoverflow.com/questions/6119206/what-are-good-candidates-for-base-controller-class-in-asp-net-mvc) — NightOwl888, Mar 05 '18 at 10:58
You can refactor it as a common service. Here's a good example: [common services](https://stackoverflow.com/questions/49111585/multiple-urls-same-action-method) — Eric Grassl, Mar 05 '18 at 15:09

Erik Philips · Accepted Answer · 2018-08-28T01:37:56.857

Should same methods from different controllers be moved to a CommonController?

Yes and you should not use Inheritance. I'm sure there are plenty of people who may disagree, however your example is extremely generic and gives very poor context there is no good reason all controllers need the same code (Inheritance or not). Your questions context has no reason for it to be the case in the OOP realm of Has A vs Is A (Excerpt below).

A House is a Building (inheritance);
A House has a Room (composition);

What it appears you are doing is neither of these.

If your interface was IVehicleEngine and your controllers were FerarriVehicleController and FordVehicleController now it makes sense in a context. In this case each controller should use inheritance, it makes sense.

In my own humble opinions, inheriting from your own controller can be quite difficult in multiple terms. First, it's not intuitive; that is it will become tribal knowledge because it replaces a normal convention that most programmers adhere to (that is deriving from the base MVC controller). Secondly, I've seen it become the one place that everyone decides to add code to (God Object) even though it may not be applicable from some controllers. Thirdly, it makes it difficult to reuse url's that make sense for the derived type but not the base type (/search?searchFor=). There are a number of other considerations that are very specific to MVC because of it's exposure to the web (security etc etc).

Depending on implementation you may also experience difficulty in determining which URL to use under what circumstances.

Is /Controller1/Method1/Data/1 the same as /Controller2/Method1/Data/1 but different from /Controller3/Method1/Data/1? If they are all the same or some are the same and some are different then there is most likely something wrong with the architecture.

score 0 · Answer 2 · answered Aug 17 '18 at 16:36

Nothing wrong with inheriting from a base controller. As it adheres to the DRY principle. I would go with :

public abstract class CommonController: Controller
{
    protected readonly ITestBL bl;

    protected Controller1(ITestBL bl)
    {
        this.bl= bl;
    }

    [HttpGet]
    public virtual ActionResult Method1(string data)
    {
            var res = ...

            return Json(res, JsonRequestBehavior.AllowGet);
    }

    [HttpGet]
    public virtual ActionResult Method2(string data, int data2)
    {
            var res = ...

            return Json(res, JsonRequestBehavior.AllowGet);
    }
}

Main differences being.

I removed "using". Like the others have said it's best to let your DI framework decide when to dispose of the injected class.
Make the action results virtual. The inheriting controllers may have the same requirements now, but there is no guarantee that they will stay that way in the future. So declaring them as virtual allows for future changes in scope/requirements as they can be overridden.
I made the injected class "protected" rather then "private" as other methods in both inheriting controllers may need it too.

Should same methods from different controllers be moved to a CommonController?

2 Answers2