Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up extension methods #17051

Open
wants to merge 15 commits into
base: v15/dev
Choose a base branch
from

Conversation

ronaldbarendse
Copy link
Contributor

Prerequisites

  • I have added steps to test this contribution in the description below

Description

Umbraco has accumulated quite a lot of extension methods over the years, especially on native .NET types. Most of these extension methods have been added without much thought regarding:

  • Performance impact: using slightly more verbose code will perform much better in most cases and otherwise are just as fast as using LINQ methods;
  • Naming: besides the occasional typo, different naming conventions are used and some names are simply extremely verbose to convey their actual use-case;
  • Documentation: most extension methods don't have any or otherwise meaningful XML documentation tags;
  • Usage: public extension methods are added, while they are only used in the codebase once or could otherwise be internal.

Although this PR removes quite a lot of code, there's a lot more that could be removed after some minor refactors. I'll add justifications for each of the removed extension methods, hopefully making reviewing/testing this PR a bit easier (or otherwise at least start the discussion to clean these up).

/// </summary>
/// <param name="assembly"></param>
/// <returns></returns>
public static FileInfo GetAssemblyFile(this Assembly assembly)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was unused and can otherwise be easily retrieved using assembly.Location.

/// </summary>
/// <param name="assemblyName"></param>
/// <returns></returns>
public static FileInfo? GetAssemblyFile(this AssemblyName assemblyName)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the other overload, this was unused and can otherwise be easily retrieved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can now use collection expressions instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DataTable was used for the Node and DynamicPublishedContent classes, which don't exist anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These extension methods aren't used, but should otherwise use async Task.Delay() instead of sleeping the thread anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deconstructing key value pairs is now natively supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These extension methods weren't used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've only removed the ToDictionary() extension method, as that can be replaced with RouteValueDictionary or HtmlHelper.AnonymousObjectToHtmlAttributes(). I've reformatted the remaining methods and logically ordered them in the file.

/// null)
/// </param>
/// <returns>The children of the content.</returns>
public static DataTable ChildrenAsTable(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the DataTableExtensions, this is legacy code that isn't used anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SanitizeThreadCulture() isn't used and there's no context why this was necessary (although I believe it was used to automatically get the language name in the invariant culture)...

@bergmania
Copy link
Member

Any reason not just to obsolete them now, at remove for 16 instead?

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

Successfully merging this pull request may close these issues.

2 participants