Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Base class for ViewResult and PartialViewResult #6984

Closed
rjperes opened this issue Oct 23, 2017 · 6 comments
Closed

Base class for ViewResult and PartialViewResult #6984

rjperes opened this issue Oct 23, 2017 · 6 comments

Comments

@rjperes
Copy link

rjperes commented Oct 23, 2017

Since ViewResult and PartialViewResult share so much (properties and execution), why not have them inherit from a, abstract base class and just override ExecuteResultAsync, or some abstract method?

@Eilon
Copy link
Member

Eilon commented Nov 3, 2017

They probably could have shared a base class for consolidating a bit of code, but I don't think it would have proven very useful - at least, no one has asked for it yet. That is, where would anyone use a new ViewResultBase type and operate only on that base type?

As a side note, ASP.NET MVC 1-5 did have a ViewResultBase

So, if there are concrete scenarios where having a base class (or an interface, or whatever) would prove useful, we would certainly consider it.

In the meantime closing this issue - we can reactivate if there are specific scenarios to consider.

@Eilon Eilon closed this as completed Nov 3, 2017
@rynowak
Copy link
Member

rynowak commented Nov 3, 2017

This is already a thing - all of the significant logic is shared, reusable, and extensible.

https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/ViewExecutor.cs

@rjperes
Copy link
Author

rjperes commented Nov 6, 2017

This is not a big thing, granted, but since we (Microsoft+community) are making a fresh start, why not do it right? ViewResult and PartialViewResult and ViewResultExecutor and PartialViewResultExecutor share 99% of the code, the only difference I see (I may be wrong, though) is that ViewResultExecutor sets IsMainPage to true and PartialViewResultExecutor does not. It may be important to inspect an action result (in a filter, likely) to see if it is returning a view, in which case, a check of "context.Result is ViewResultBase" is easier than "context.Result is ViewResult || context.Result is PartialViewResult". Happy to contribute with a PR if you see value in it.

@Eilon
Copy link
Member

Eilon commented Nov 6, 2017

@rjperes it isn't about right and wrong, it's a design choice. We felt that code factored by encapsulation is better than code factored by derivation.

@rjperes
Copy link
Author

rjperes commented Nov 7, 2017

@Eilon: a good discussion to be held elsewhere! :-)
I just wanted to highlight that we have a lot of code duplication, that's all.
As I said, it's no big deal, happy for this to remain closed.

@Eilon
Copy link
Member

Eilon commented Nov 7, 2017

@rjperes no worries, it's good to discuss these things and share thoughts!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants