Rubberduck: Modularization strikes again - Let's move some stuff around

[Vogel612]

A few things should be refactored a bit to get modularization up to par with the size of the codebase:

• [x] Extract Refactorings from Rubberduck.Core into Rubberduck.Refactorings. (Done in #4006)
• [x] Merge Navigation/CodeMetrics from Rubberduck.Core with Rubberduck.Inspections into Rubberduck.StaticCodeAnalysis (Waiting for a GUI, can directly also address #3522 & #403)
• [x] Extract UnitTesting from Rubberduck.Core into Rubberduck.UnitTesting. (In Progress for @Vogel612)
• [ ] Extract Win32 functionality into a Rubberduck.Win32 project. Related #3706 -- Keep in mind there are also overloads of Win32. In cases where the uses is not limited to the Rubberduck.VBEditor project, it should be wrapped up in a service class.
• [ ] Organize Rubberduck.Parsing/VBA for navigability
• [x] Extract Resources into a separate project (Done (#3976, #3982))
• [x] Separate VBEditor interfaces from implementation to avoid accidental RCW referencing explanation (In progress; @mansellan already did most of the work in his VB6 PR #3935, we need to also move the factory, pending PR #3975 to complete the isolation)
• [ ] Refactor out all those faux static/singleton/single-use classes such as VbeNativeServices, VbeEvents, and other similar classes from RubberduckIoCInstaller.RegisterInstances into the Rubberduck.Main, making the implementations private to the project and thus owned by the Extension class ultimately, forcing all access via interfaces only. That would allow us to convert those classes into a regular instancing classes that only Extension and its children can control.
• [ ] Set up VbeNativeServices to listen to WM_DESTROY messages and initiate shutdown in addition to the existing shutdown procedures in response to Extension.Shutdown(), and ensure that all methods for shutting down services are idempotent.
• [ ] ... ?

Each of these can (and should) be handled in a separate PR and can be done independently.

[bclothier]

I like. I did not like how the Core project had a Refactoring folder and then a UI\Refactoring folder which seems a bit confusing for new users. A dedicated UI project and a Refactoring would make the separation more cleaner.

[bclothier]

Just to remind of one more case that @Hosch250 mentioned -- #3706 --- that should be its own project, IMO. Maybe Rubberduck.Win32. There are nuget packages that maintains the user32 and kernel32 so we can leverage that to avoid needing to maintain our own extern procedure.

[bclothier]

Also -- while reviewing the PR, it occurs to me that the RubberduckUI.resx may be overdue for splitting, perhaps? As it is now, the names and the number of entries is becoming very unwieldy and is also a source of PR conflicts whenever we add new resources to the file from more than one PR. Can we consider whether we can split the resource files among the features?

[bclothier]

Working on a PR it occurs to me that the maintenance of DI is getting a bit mite too hairy...

If you look at some classes, we have quite a number of parameters going into the constructors. This also impacts the tests since every time we modify the ctor, we have to visit the tests to update them all.

Is it crazy to suggest that those be refactored so that we inject some kind of factory that provides commonly used objects like RPS, UI dispatcher, and etc? That way we only need to update the factory and not touch the ctor's signature?

[retailcoder]

Is it crazy to suggest that those be refactored so that we inject some kind of factory that provides commonly used objects like RPS, UI dispatcher, and etc? That way we only need to update the factory and not touch the ctor's signature?

@bclothier I fully support this idea!

[MDoerner]

I have to say that I do not really like the idea of a factory providing all commonly used parts of RD. There are two main reasons:

1. The factory would introduce artificial coupling. Most constructors need some of the parts used more often, but not all of them. Injecting a factory providing all will couple the constructors with all interfaces provided. That said, I would not see a problem to introduce different facade services bundling constructor parameters, as has been done in the ParseCoordinator. However, then they should only be injected into constructors requiring all interfaces provided.

2. I do not see how this helps with the problem with updating the unit tests. It will just push the problem to updating the wiring up of the factory in the unit tests. To provide the same amount of functionality of the objects provided, we will need to put the same amount of objects into the factory.

I think the problem with the unit tests is not the parameters injected, but the non-centralized way the major objects like the RubberduckParserState are wired up in quite a few tests. So, instead of changing the DI in the production code, I would propose to go through the tests and add factory methods in the Mocks namespace providing things like the RubberduckParserState with appropriate standard injections for testing. We already do that in most tests. There are just some combinations of parameters to customize that are not covered yet and some tests from before the introduction of the other factory methods that have not been updated so far.

With this approach, we would usually have to change only two to three places when adding a constructor parameter to one of the main parts of RD: the mocks factory, the experimental API and maybe the IoC installer, in case the parameter needs special attention.

[Vogel612]

While we're at the IoC .... what do you guys think of the UI knowing how to wire the UI components? Like... ViewModel bindings, Command bindings and all that mess. If we could "tuck that away nicely" into Rubberduck.Core (which at that point should become Rubberduck.UI), should we do so?

[bclothier]

@Vogel612 FWIW, in the Extract Method branch, I did revamp the flow because it was confusing me and there were too many plumbing details in the way. The approach can be seen here in the ExtractMethod branch. Basically we use a factory and define the needed data. That seems to help with keeping the logic clean and avoid newing anything relating to the WinForm dialog or the WPF Usercontrol; only ViewModel needs to be newed up.

[bclothier]

Another idea that was floated a while back in the chat and posting for posterity/further considerations in light of the upcoming PR on VB6 support:

Once the PR is merged we will now have Rubberduck.VBEditor.VBA and Rubberduck.VBEEditor.VB6. I think it would be best for all depending projects that they do not reference either concrete implementations at all. Therefore we would move all interfaces (and some utility codes) into Rubberduck.VBEEditor to represent the abstractions.

In the VB6 PR, we will gain a new provider used in the Rubberduck.Main to pick the correct concrete implementation. Therefore, only Rubberduck.Main would know about the Rubberduck.VBEEditor.VBA and Rubberduck.VBEEditor.VB6 but the rest of projects (Core, Parsing, etc.) would only need to reference the Rubberduck.VBEEditor.

This is hoped that this will also aid in avoid accidental referencing any concrete implementation or raw RCWs.

[bclothier]

Note the comments in #2826 where we discuss the idea of stairway pattern. We should review and ensure we apply where it is warranted.

[bclothier]

I have a potentially controversial question --- should we create a dedicated project for the constants or resources?

One thing I don't really like ATM is the location of RubberduckGuid.cs and RubberduckProgId.cs -- they are currently in the Core project but are also referenced by API and Main. That's a mite too octupus-y for me.

I see two alternatives:

1) split it up so that each "project" has its own ProgId/Guid
2) a constant-only non-building project that any project can reference.

I'm leaning toward # 2 because that means all COM visible components are enumerated in one place which is good for maintenance, and being a separate project, there is no harm in referencing the new project since it's not a true dependency (since constants gets burned in).

I then thought about the resources but came to the opposite opinion w/ resources - felt that they were best split across project since there is nothing shared and we have tooling for managing localization of the resources anyway.

Thoughts?

[retailcoder]

@bclothier I'm with you on # 2.

Caveat: if any const value changes, all referencing libraries need to be re-compiled, given how const values are inlined at call sites at compile time. If a value doesn't need to be a const, let it be public static?

[bclothier]

Per the chat discussion, it might be more desirable to create a Rubberduck.Resources and house all the resources and other constants. The project would split the resource files across the concerns, so that we end up with namespaces such as the following (as illustration only; nothing set in concrete):

Rubberduck.Resources.UI
Rubberduck.Resources.UI.Commands
Rubberduck.Resources.UI.MenuItems
Rubberduck.Resources.UnitTesting
Rubberduck.Resources.Inspections
Rubberduck.Resources.QuickFixes
Rubberduck.Resources.Images
Rubberduck.Resources.Images.Icons
Rubberduck.Resources.COM.Guid
Rubberduck.Resources.COM.ProgID

The projects that needs the resources or constants would be able to freely reference the new project and easily have access to shared resources without having to decide/figure out if a resource could be used across more than one concerns.

Thoughts?

[retailcoder]

IMO Guids & ProgIds belong under the same namespace, Rubberduck.Resources.COM sounds good.

Also it's probably best to name the files as per a given feature, e.g. RubberduckUI.UnitTesting.fr.resx, RubberduckUI.Inspections.de.resx, and leave them under the Rubberduck.Resources namespace, instead of having a bunch of single-file namespaces.

[bclothier]

I was thinking - the com collector stuff are in the Rubberduck.Parsing. It deals a lot with COM particulars, involving pointer and unmanaged types management. Would it be better to separate that into its own project, so that there is no direct dependency on COM stuff from within the Rubberduck.Parsing project? Thoughts?

[bclothier]

Note: Updated the checklist to reflect the recent discussion around the various classes that are missing a service locator and has been implementing.... _strange_ design pattern to avoid the need for an anti-pattern. Also noted the need to make distinct between the common forms for pinvokes vs specialized pinvokes WRT win32; there are some functions within VBEditor that has uncommon pinvoke form.