Need to refactor according to best practices

[astrohart]

Mads, as per our discussion several months ago, and I forget which issue it is in, but the need was assessed by me, and you seemed to be on board, for the code to be overhauled and refactored in order to be more:

• SOLID
• Fluent
• Documented
• Extensible
• Maintainable

To this end, this issue is highlighting the need for this to be carried out on the codebase.

The impetus is that this extension in particular is one of your more useful/popular ones, and, as a contributor, I am running up against some brittleness in the existing codebase.

I am not denigrating anyone's programming abilities; I am simply pointing out the need to reduce tech debt for further contributions by doing the improvements noted above, as you and I spoke about.

I have ReSharper and CodeMaid and I also have a huge bank of snippets and file templates to make this process go fast. The other great thing about this is that you can avoid "reference collisions" when you're trying to use pieces from disparate parts of the software system.

[madskristensen]

Good idea. Perhaps even rewrite to use the Community.VisualStudio.Toolkit package instead of the regular VSSDK and the DTE

[astrohart]

Using the Community.VisualStudio.Toolkit is a great idea, Mads, but given the scope of the change that that would entail, I would suggest creating a separate Issue in GitHub just for that change, so that the merges/PRs can be tracked better.

In other musings:

One thing I was also thinking of doing is turning the ImportWindow class into more of a "proper" MVVM (Model View-ViewModel) or even just MVP (model-view-presenter) architecture.

Perhaps it might be also appropriate to create shared modules for the window functionality but then to actually break up ImportWindow into 3 separate windows: ImportWindow, ExportWindow, and InstallSolutionWindow (or some such, maybe I can think up a better name later). These can all be "siamese twins joined at the heart" so to speak under an MVVM/MVP architecture, I would bet.

The advantage of doing separate ImportWindow, ExportWindow, and InstallSolutionWindow classes would be that then the functionality for the different windows (it does vary slightly) could then be segregated out by operation purpose instead of all crammed into one window with lots of if statements and switch/case branches.

For example, Issue #49 requests functionality that makes it necessary to implement certain UI/UX, and code behind, on the Export side but not the Import side.

Another thing I like to do is create static classes whose names are verbs (such as Get, Show, Read etc.) and then write robust methods under them that carry out the actions needed. Kind of like they have it in the System.IO namespace, where we have called such as File.Exists and Directory.CreateDirectory however, a hypothetical method here would be Get.RegistryValue or somesuch. (I am not saying such a method needs to be written, I am just giving an example.)

Also, I know people throw shade on the Singleton pattern, but it can be useful in fluent programming. For example, we can say BuildNew.Foo.ForDisplayName where Foo is the static instance member of the singleton. However, the class BuildNew maintains a state if we are using the Builder pattern, so it can't be static. We can also segregate functionality using a singleton that implements multiple interface, one for each group of functionality. You just pull up the methods for the category of functionality you want to each respective interface. The class implements them all but has several static Singleton instance properties, each one of the type of one of the interfaces.

This is how API controllers do it. I've found it to be highly useful.

Finally, I'd like to propose that I do multiple PRs for this issue, to kind of take an 'evolutionary' approach. That is, rewrite a bit at a time and do successive PRs for each batch of related Commits.

Kind of like, when you're a member of a team and you stage 5 commits, and then do a Pull then Push to move it up to the remote. Here, I'd do a bunch of commits for a related part of this re-write, then do a PR, and then you could merge them incrementally.

[astrohart]

@madskristensen I am going to go ahead and fork and clone this out. I am going to just caution you, please don't get alarmed if, for my first few PRs, the changes make the code look a little wonky, meaning as we start the re-writes, it will be kind of half old and half new, if you know what I mean. I will do incremental PRs along the way.

I will also, every so often, refactor your code as I work on other issues. So, besides the actual change itself, I will also put some re-architecting in as well. On those PRs, I will say "Fixed issue NN and addressed 59" to refer to this issue as well.

[astrohart]

Hi @madskristensen sorry for all the confusion earlier in regards to VSIX Synchronizer. I was operating under mistaken impressions as to just how a Shared Items type of project works in Visual Studio. I need to make that project go away, due to the refactoring requirements by conventions. One of the conventions is "name all namespaces after the project that contains them," and another is "Name all namespaces for files in a folder under a project with the project name and then the dotted path to that folder."

An example of the 2nd convention mentioned above is, if I have a visual studio project, say, in a folder, and it is called MyProject and there is a file and folder structure like:

MyProject
   |
   --- Foo
         |
         --- Bar
                |
                --- Baz.cs

Then the root namespace of MyProject should be defined in project properties to be MyProject and the namespace of class Baz in the file Baz.cs is to be named namespace MyProject.Foo.Bar, i.e.

/* Baz.cs */
namespace MyProject.Foo.Bar
{
    public class Baz
    {
        /* ... */
    }
}

In any event, due to all the overlapping namespace issues from applying conventional refactoring, the Shared Items project, IMHO needs to go away, and the code for the commands, packages etc. be copied to each of the projects, and commonly-used classes between all 3 of them should be extracted to C# Class Library projects, proper, that sit in the solution. Otherwise the Visual Studio references are going to get all tangled.

The Shared Items project, in principle, was a good call to do, but given that the behaviors of the extension now is needing to be made version-specific (per issue #49 and others), I think the Shared Items project (duplicating classes across the projects for the separate versions of VS in a cross-cutting fashion) has reached the limit of its usefulness. Just do a set of Class Library type projects instead, would be my recommendation.

[astrohart]

Other conventions I need to apply are:

• Method and constructor parameters should be based on an interface, where possible, instead of a concrete type;
• Fields should be typed by interfaces and constructor dependencies should be injected as interfaces;
• Factory methods should be in their own static class, not a static method inside the type that they create.

These conventions, especially the 3rd one, enhance modularity and maintainability. It makes the code much easier to add features to.

[madskristensen]

I think it makes sense to add the project's default namespace to the generated class. It's always in the root of the project, so no need to look at folder structures to match the namespace.

The other conventions you mention I feel would solve a problem no one has, and the cost is added complexity for all. I don't think it's worth pursuing.

[astrohart]

The other conventions you mention I feel would solve a problem no one has, and the cost is added complexity for all. I don't think it's worth pursuing.

The other conventions I need to follow, actually, too -- for various reasons that might not be immediately apparent but I know are soundly grounded in my own experience.

How about this, can I open a new branch, have my way with your code on that branch and then show you the PRs? Then you can see what I am talking about and decide, without any risk of damaging the trunk.

I think it makes sense to add the project's default namespace to the generated class. It's always in the root of the project, so no need to look at folder structures to match the namespace.

Are you talking about in VSIX Synchronizer? If so, then please see PR 11 under that project, that is already done. I need that merged and published to the Open VSIX Gallery so that I can update the extension in my VS for this issue.

UPDATE I closed that PR and issue because, I raised that issue being totally ignorant of how a Shared Items project works. If the Shared Items project stays, because of its cross-cutting functionality, there is absolutely no need to prepend the file namespace.

One thing I will mention though the name Vsix for one of the generated classes collides with a class called Vsix in the VSSDK. I strongly recommend changing it to something like VsixAttributes.

[astrohart]

@madskristensen I have an idea.

I'd like to run the Designite tool that is a code analyzer. This will find any architecture smells, design smells, and/or code smells in this extension.

Then I think (unless I have to pay for the tool) it lets me run an Excel export, which I will then upload here to this issue.

Feel free to then identify which smells matter the most to you for me to get rid of. This way, you and I can stop dancing around each other, having a religious war (lol) and instead, we can go off of the more objective output of a code analyzer, and we can come to a consensus about how to tackle this issue.

I am downloading the extension right now; I am not sure if it's a trial, or if you have to pay to get the export feature. I will keep you updated.

[astrohart]

I think that the code in ImportCommand.cs breaks the Single Responsibility Principle of SOLID. It shows the dialog and gathers from the user the list of extensions that the user wishes to import.

There there is a huge morass of code following the invocation of the dialog that actually carries out the import.

Same for ExportCommand although here there is not quite as much implementation code.

Both files should have a better separation of concerns.

I propose creating an ImportService and an ExportService and then handing off the list of selected extensions to the services indicated so that they can process the import and export operations.

[astrohart]

I also think Singletons get a bad rap. Everyone seems to hate them.

True, it's bad practice to have a Singleton ordinarily; there is a static Instance property (or some such) and everyone has to have a reference to where it is defined in order to access it.

Indeed, I've worked on production software systems where Singletons were utilized early on and then the references got tangled once the software system reached a certain size.

The solution is the following: make the Singleton class (Foo say) implement an interface IFoo, say. Make the static Instance method have a type of IFoo. Then create a GetFoo.cs file with a static class called GetFoo and a public static method of return type IFoo called SoleInstance(). Its job is simply to be:

namespace MyProject
{
    public static class GetFoo
    {
        public static IFoo SoleInstance()
            => Foo.Instance;
    }
}

Now, the Foo single instance is accessed fluently by calling code:

GetFoo.SoleInstance().MyMethod();

This may seem like overkill for a small project. But, as we have all experienced, more often than not, small software projects turn into large software projects and the tech debt increases.

Having the IFoo contract and typing the Instance method as IFoo guarantees that all callers of the Singleton will abide by the contract that the IFoo interface sets up.

The static method should ideally be in its own class library, as the Interface and the class Foo should also be.

Then in large software systems, the GetFoo class' library can be referenced and we avoid circular dependency errors.

ExtensionPackTools may be small now, but I caution against being complacent.

I have file templates and snippets that write this code for me blazingly fast.

When you use it in another class, you can create what I like to call a singleton dependency property:

private static IFoo Foo
            => GetFoo.SoleInstance();

This is a great thing to do with a Service class.

Why not just create a static class that exposes a bag of static methods? you might ask.

You can, if the methods don't need to maintain state as several of them get called/call each other. A singleton can maintain state; a static class cannot. Per the Extract Function refactoring catalog, each specific operation in an algorithm should have an extracted function.

I can do this in both a Singleton and in a static class; however, as the methods call each other to implement an algorithm inside a static class, they cannot save state in properties of the instance such as a Singleton can, along the way. Such properties' values may need to be monitored as they change by the clients of the class.

This way, the inputs and outputs of each step are clearly defined, cyclomatic complexity is reduced, and the code is very fluent; it can clearly be read by someone not very familiar with the project in order to deduce what is going on.

You can use this technique in a static class, but it's hard to maintain state from one operation to the other with internal properties, such as you can do with a Singleton.

For example, a Windows Form or a WPF window is a great candidate for turning into a singleton. And you actually, in a way, did so, with the ImportWindow. It's not technically a Singleton, but it is a create-once and then use always (sort-of; I'm being very hand-wavy here) sort of object.

Service classes are other excellent examples of objects that can be turned into Singletons using the techniques above.

Then you also have a consistent contract with the outside world that is enforced by the class' interface.

If a Service has dependencies that get injected by the constructor, then this is where I would perhaps shy away from doing the above. Such as ExtensionService.

[astrohart]

This article sums up nicely the tack I am taking, overall, in this refactor.

[madskristensen]

This is a huge body of work to do and it will leave the code base unrecognizable. Not necessarily a bad thing, but i'm not sure this is worth it. The extension works and have for a long time, so let's not rock the boat

[astrohart]

@madskristensen I can understand your concern to a degree.

However, the flip side is that the code was brittle before this effort began, hard to maintain and/or add features to, and hard to navigate.

I think that has to be weighed.

Also, I encourage you to please bear in mind that this effort is very WIP. I am committing at a very granular level, as well.

The benefits of my work are threefold:

• Non-brittle, easy-to-navigate and extend code.
• Robust, fault-tolerant code that holds up to a lot of edge cases.
• Errors can be easily debugged and fixed without breaking other code, through the use of code contracts enforced by interfaces.

Think of this as a "journey."

Personally, one of my passions is having clean, well-documented, robust code that can be extended and modified with a minimum of tech debt.

Tech debt being, as you may already know, the fact that, as a software system becomes more complex, it becomes harder to modify and extend (in terms of time taken) and easier to break functionality in other parts of the system due to late-breaking changes elsewhere.

By refactoring and cleaning the code now, we're future-proofing this codebase.

I think that makes it worth it, as this particular piece of code, in its utility to a wide cross-section of users, becomes more and more widely adopted.

I speculate that that is what may happen.

If so, then it behooves us to make this code future-proof and robust.

This way, you can gain an enhanced reputation.

As more people download and use this extension, and it consistently works as expected for them, I would surmise that that would improve your own reputation in the developer community.

Therefore, I'd like to make an argument for being allowed to continue.

[madskristensen]

You make good points. Ok, you convinced me. Keep at it and then let me know when ready to merge

[astrohart]

Thank you.

I'd also like to make an argument for being conscientious about providing XML documentation comments for each code element (each class, struct, enum, interface, and the respective members thereof).

Personally, I have a passion for documenting things.

The benefits are also multifold:

• I myself can look at a piece of code later in the development cycle, and remember what its intended purpose is, as well as the acceptable inputs and outputs.
• Debugging is enhanced, since now, when I hover over a method call or property reference, there is clear documentation in the little ToolTip that VS provides.
  • This assists me when debugging.
    • For example, I can compare the actual value of, say, the property to what its documentation says the acceptable values are.
• Other contributors and maintainers of the code can do 1 & 2 above.
  • For example, a maintainer who is not me can hover over the call to a method I wrote, and the Visual Studio ToolTip will inform them what the method does, what its return value(s) are, and what exception(s) if any, it throws, as well as what are acceptable values for the parameters.
• A tool, such as Vsxmd, or doxygen etc. can be run over the collected XML comments
  • This serves to automatically generate online documentation that can be put to a number of uses online.
  • For example, each class, struct, enum, interface, and the respective members thereof can have .md files that display their appropriate usage and the exceptions thrown on the GitHub code browser.

There are a whole host of other benefits.

I've made heavy use of automation to assist me in generating these comments quickly and with minimal keystrokes.

I've already done quite a lot in that regard.

I'd like to get your approval to continue being conscientious about documentation.

I'd also want to sound you out on doing this for protected, private, and internal code elements.

The thinking being that these code elements will still be seen, e.g., in the debugger.

When they have certain values, or are invoked in certain ways, the XML documentation showing in the ToolTip can provide guidance as to what the acceptable value set is, even for these members. I can compare what the Watch window tells me to the acceptable value set, and quickly gain insight into the code base and where the fix needs to take place.