U4-8199 - Remove unused/obsolete/deprected classes/methods

Created by Shannon Deminick 17 Mar 2016, 12:46:24 Updated by Andy Thompson 05 Jun 2018, 11:53:39

Tags: Up For Grabs Community Contrib

Subtask of: U4-5419

This is a container issue, what needs to happen is we need to go through the codebase and find:

  • Code that is unused - this can be done with Resharper by right clicking on the solution, selecting "Find Code Issues", when that is done one of the categories in it's report is unused methods, etc...
  • Code that is obsolete/deprecated - There is TONS of Obsoleted classes, methods, etc... throughout the codebase. Some will be very easy to remove, some will be much harder since it would require migrating all usages over to the new format. Many of the easier ones will have an Obsolete attribute containing terms like "This is no longer used" and/or "removed from the codebase in future versions" and/or just "future version"

For anything that is to be removed, report it here and we'll determine if we should create sub-issues to track what is being removed.

Removed so far as part of this task

  • 'PublicAccessServiceExtensions.HasAccess`
  • IPublicAccessService.AddOrUpdateRule
  • Umbraco.Core.Models.Template.GetTypeOfRenderingEngine
  • SerializableData
  • LegacyTree
  • SerializedTreeType.XmlTree
  • EncodedStringWriter
  • JSONSerializer
  • SerializedTreeType

Comments

Steven Newstead 21 Mar 2016, 16:24:36

I've just submitted a pull request to get rid of:

\Umbraco-CMS\src\Umbraco.Web_Legacy\Utils\SerializableData.cs \Umbraco-CMS\src\Umbraco.Web\umbraco.presentation\umbraco\Trees\LegacyTree.cs

See pull request:

https://github.com/umbraco/Umbraco-CMS/pull/1187

Hopefully it's all good, but if there is anything else I can do on this please shout!


Steven Newstead 21 Mar 2016, 16:38:41

Oh sorry, just noticed you only wanted reports of things to be removed here, I was a bit gung-ho...


Shannon Deminick 21 Mar 2016, 16:41:16

That's totally ok! Looks like you've done a stellar job :) Only reason I mentioned about reporting it first is to make sure that it's not already done or in progress by someone else.


Shannon Deminick 21 Mar 2016, 17:05:34

@mangopieface I've pulled in your PR, great work! It's worth noting that anything relating to SerializedTreeType.JsTree and (and basically anything relating to JsTree) is obsolete and can be removed too :)


Steven Newstead 21 Mar 2016, 17:32:17

Awesome, I'll take a gander later on!


Steven Newstead 21 Mar 2016, 22:04:30

I've not taken on the JSTree yet, but tried to get rid of the following:

\Umbraco-CMS\src\Umbraco.Web_Legacy\Utils\EncodedStringWriter.cs \Umbraco-CMS\src\Umbraco.Web_Legacy\Utils\JSONSerializer.cs

These are in PR https://github.com/umbraco/Umbraco-CMS/pull/1191


Shannon Deminick 31 Mar 2016, 16:51:37

I'm going to put this back into Open, feel free to submit more PRs for more code removal :)


Nik 31 Mar 2016, 20:54:19

@Shandem I'm looking at removing the old SQL and MySQL installer bits that are marked as obsolete in umbraco.datalayer.

@mangopieface have you looked into these yet? (Thought as there was so much I'd offer a hand).


Steven Newstead 31 Mar 2016, 21:04:21

@nik nope, not yet I'll update here before I start removing anything


Shannon Deminick 04 Apr 2016, 08:55:46

@Nik that part of the codebase is going to be VERY hard to remove, the problem is that we need to migrate over all of the usages of the old business logic code from the umbraco.cms assembly. This is an ongoing (and very slow) process. I'll create tasks for each one of those very soon but until all of that is done there won't be much you can remove for the old sql stuff. That said, if you can remove anything from those assemblies that aren't being used or are easily migrated to new code please do!! :)


Andy Thompson 11 Nov 2017, 19:36:10

Hi, I want to join in and collaborate but as I'm new to github I would like to start very slowly and safely by making some extremely narrow changes, such as removing unneeded "using" directives, shortening object reference paths and adding/cleaning unit tests. Nothing that would make a major difference but would gain me experience in using pull requests and your process. Fixing these tiny things will make me read and learn the v7 source code before moving on to bigger tasks. Does that sound OK?


Andy Thompson 22 May 2018, 20:36:12

@Shandem I'd like to get involved and help clean the code. As you asked to be notified before changes are made, and as this will be my first proper PR with code, I'd like to look at the Umbraco.Core.StringExtensions class.

Would you need the work done in both v7 and v8 branches?

Thanks.


Shannon Deminick 30 May 2018, 05:37:10

Hi @Mundairson ! There are currently unit tests for StringExtensions found in Umbraco.Tests.Strings.StringExtensionsTests, see: https://github.com/umbraco/Umbraco-CMS/blob/temp8/src/Umbraco.Tests/Strings/StringExtensionsTests.cs

If you find there are extension methods untested please feel free to add unit tests :) I'd be happy if you want to just target v8 and if we see the same code surface area exists in v7 we can backport them if necessary.


Andy Thompson 05 Jun 2018, 11:53:39

I've submitted a PR to refactor the IsNullOrWhiteSpace() extension method.

https://github.com/umbraco/Umbraco-CMS/pull/2671

The IsNullOrWhiteSpace() extension method's implementation can be replaced by a call to the native, static version with no loss in execution speed (there's actually a small gain on average). The main two reasons for the replacement, however, are a simple reduction in Core logic and no new string construction just to count the length. The call to Trim() could return a new string which is then thrown away. The native version steps along the string and is therefore pure, as this version now is.

This is just a PR change just to make sure I perform the PRs correctly. I added several unit tests around the change, plus the method itself is used in several other unit tests.


Priority: Up for grabs

Type: Task

State: Open

Assignee: Shannon Deminick

Difficulty: Normal

Category:

Backwards Compatible: False

Fix Submitted:

Affected versions:

Due in version: 8.0.0

Sprint:

Story Points:

Cycle: