CODEWITHSUNDEEP
c#asp.net-mvc
hopeless
hopeless

Reputation: 96

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

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?

Upvotes: 3

Views: 684

Answers (2)

Erik Philips
Erik Philips

Reputation: 54638

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.

Upvotes: 1

Jon Ryan
Jon Ryan

Reputation: 1637

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.

  1. I removed "using". Like the others have said it's best to let your DI framework decide when to dispose of the injected class.
  2. 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.
  3. I made the injected class "protected" rather then "private" as other methods in both inheriting controllers may need it too.

Upvotes: 0

Related Questions