Interface + internal implementation or extension method?

[jansenbe]

Subject

Today we add methods on both our model classes as or model collection classes and we do that in two manners:

• using an interface + methods on model and/or model collection classes
• by adding an extension method on the respective model or model collection classes

Question: should we standardize on extension methods for everything except "Add..." methods? This would give contributors an easy and consistent approach for "enriching" the model. I don't see a ton of value in having the methods appear in the interfaces + this also allows to easier split up the code. One can even argue if all methods on the model should be implemented as extension methods??

Tagging @PaoloPia , @ypcode , @pkbullock and @JarbasHorst for input.

Samples

Model methods

Interface sample. IWeb.cs:

 public Task<IFolder> GetFolderByServerRelativeUrlAsync(string serverRelativeUrl, 
params Expression<Func<IFolder, object>>[] expressions);

and web.cs:

        public async Task<IFolder> GetFolderByServerRelativeUrlAsync(string serverRelativeUrl, 
params Expression<Func<IFolder, object>>[] expressions)
        {
            Folder folder = new Folder()
            {
                PnPContext = this.PnPContext,
                Parent = this
            };

            await folder.BaseGet(apiOverride: BuildGetFolderByRelativeUrlApiCall(serverRelativeUrl), 
fromJsonCasting: folder.MappingHandler, postMappingJson: folder.PostMappingHandler, 
expressions: expressions).ConfigureAwait(false);

            return folder;
        }

Extension method sample. WebExtensions.cs:

        public static async Task<bool> IsNoScriptSiteAsync(this IWeb web)
        {
            await web.EnsurePropertiesAsync(w => w.EffectiveBasePermissions).ConfigureAwait(false);

            // Definition of no-script is not having the AddAndCustomizePages permission
            if (!web.EffectiveBasePermissions.Has(PermissionKind.AddAndCustomizePages))
            {
                return true;
            }

            return false;
        }

Model collection methods

Interface sample. IRecycleBinItemCollection.cs:

public Task RestoreAllAsync();

and recyclebinitemcollection.cs:

        public async Task RestoreAllAsync()
        {
            var item = new RecycleBinItem()
            {
                PnPContext = PnPContext,
                Parent = Parent
            };
            var entity = EntityManager.GetClassInfo(item.GetType(), item);
            string deleteAllEndpointUrl = $"{entity.SharePointGet}/RestoreAll()";

            var apiCall = new ApiCall(deleteAllEndpointUrl, ApiType.SPORest);

            await item.RawRequestAsync(apiCall, HttpMethod.Post).ConfigureAwait(false);
        }

Extension method sample. RecycleBinItemExtensions.cs:

        public static IRecycleBinItem GetById(this IQueryable<IRecycleBinItem> source, Guid id, 
params Expression<Func<IRecycleBinItem, object>>[] selectors)
        {
            if (source == null)
            {
                throw new ArgumentNullException(nameof(source));
            }

            return BaseDataModelExtensions.BaseLinqGet(source, l => l.Id == id, selectors);
        }

[pkbullock]

So I haven't done much on the old sites core but as far as I understand this is predominantly extension based which the existing developers understood - so thinking that keeping an element of familiarity in the implementation would help those currently contributing on that repo to move over. Sounds like you have a direction in mind, sorry I cannot offer a more technical reason either way.

[ypcode]

IMHO, it might indeed help with more concise and clear way to extend a model and would avoid cumbersome task of adding the signature in one place and implementation on another.

If technically, there is no impact on performance (I don't know if it works any differently in CLR for extensions or member methods ?). Then I don't see any technical reasons not to go this way :)

However, 1 thing I would be a bit concerned about is the generated documentation... As a consumer of the SDK I would like to see at once all the methods available and not having to go to 2 documentation sources for the available methods of a same entity... (Based on what we figured out, DocFX is referencing the available extensions methods but redirect to other pages for more details).

I know that VS intellisense will give me everything... :D but I would think we would need the official docs to be clear enough and not to rely of developers editors

[PaoloPia]

That's indeed an interesting discussion. However, I'm not in favor of moving from interface and instance methods to extensions methods. There are multiple reasons supporting my previous sentence. Let me try to explain the reasoning behind.

1. Let’s start with the definition of extension method (https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/extension-methods): Extension methods enable you to "add" methods to existing types without creating a new derived type, recompiling, or otherwise modifying the original type. It means that the purpose of the extension methods is to allow developers to extend types defined by someone else (most likely in another assembly that she/he doesn’t own), without having access to the original type definition. That’s what we have been doing for example in PnP Sites Core with the CSOM types. This is more important and useful when a developer wants to extend a third party sealed type, that as such cannot be inherited and extended relying on OOP practices.
2. The main purpose of extension methods is to provide a syntactic sugar helpful for implementations “like” LINQ (in fact they were introduced together with LINQ) and to provide an easy to use way of invoking an operation on a target type. The extension methods are not comparable to instance methods. In fact, an extension method cannot access private, protected, or internal members of the extended class.
3. Extension methods cannot be overridden from an object oriented point of view, while an instance method declared as virtual, can be overridden in a derived class. Instance methods can also be hidden with the new operator, which again is not available for extension methods.
4. Extension methods are applied with lower precedence than instance methods. If there is an instance method with the same signature of an extension method, the instance method wins. Moreover, extension methods are invoked using a resolution algorithm that relies on imported namespaces and target type. As such, it is possible to have multiple extension methods, with the same signature, targeting the same type, defined in different namespaces. Based on the using directives defined in code, the extension method invocation could target one or the other extension method. This by design behavior is really useful in scenario “like” LINQ (again) where you could have extension methods like Where() that can change their behavior depending if they are used in the context of LINQ to Object, LINQ to Entities, etc. At the same time, this by design behavior could be really dangerous and error prone when used by developers consuming our SDK, depending on the using directives in their code modules.
5. Lastly, consider the official guidelines (https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/extension-methods#general-guidelines) from Microsoft: For those occasions when the original source isn't under your control, when a derived object is inappropriate or impossible, or when the functionality shouldn't be exposed beyond its applicable scope, Extension methods are an excellent choice. But at the same time: it's still considered preferable to add functionality by modifying an object's code or deriving a new type whenever it's reasonable and possible to do so.

Performances are not a real problem, because under the cover extension methods and instance methods perform almost the same. Actually, extension method are on average a little bit slower than instance methods, but it is really a tiny difference that I wouldn’t care of.

So, to wrap up … that’s why I’m not in favor of replacing instance methods with extension methods and I would keep on writing the domain model the right way, using the correct OOP patterns.

Sorry for the long reply, but I wanted to be clear (hopefully).

[jansenbe]

Very clear and well written arguments @PaoloPia. The main reason why I brought this up is consistency, today we've a mix of both which is confusing for developers. With your above arguments in mind let's then standardize on regular methods and rewrite the existing model extension methods to regular methods.