search

rubberduck-vba / Rubberduck

Every programmer needs a rubberduck. COM add-in for the VBA & VB6 IDE (VBE).
https://rubberduckvba.com
GNU General Public License v3.0
1.91k stars 299 forks source link

Modularization strikes again - Let's move some stuff around #3862

Open Vogel612 opened 6 years ago

Vogel612 commented 6 years ago

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

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

[ex]: https://github.com/rubberduck-vba/Rubberduck/issues/3862#issuecomment-381977282

bclothier commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 5 years ago

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.