Skip to content

/ WindowsCommunityToolkit

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

Jump to bottom

Inconsistent naming for singleton properties (Shared|Default|Instance) #3493

Open
Sergio0694 opened this issue Sep 22, 2020 · 5 comments
Open

Inconsistent naming for singleton properties (Shared|Default|Instance) #3493

Sergio0694 opened this issue Sep 22, 2020 · 5 comments
Labels
bug 🐛 improvements ✨ introduce breaking changes 💥 open discussion ☎️

Comments

@Sergio0694
Copy link
Member

@Sergio0694 Sergio0694 commented Sep 22, 2020 •
edited

Describe the issue

We have a bunch of singleton properties in the toolkit, but with inconsistent naming. I'm wondering whether it wouldn't make sense to have them all converge on a single term, to make them more intuitive for developers. Changing the name would obviously be a breaking change, so this seems like a good moment to consider this with 7.0 around the corner.

A few examples:

WindowsCommunityToolkit/Microsoft.Toolkit.Uwp/Helpers/SystemInformation.cs

Line 40 in a7f8976

public static SystemInformation Instance { get; } = new SystemInformation();

WindowsCommunityToolkit/Microsoft.Toolkit.Mvvm/Messaging/StrongReferenceMessenger.cs

Line 89 in 32656db

public static StrongReferenceMessenger Default { get; } = new StrongReferenceMessenger();

WindowsCommunityToolkit/Microsoft.Toolkit.HighPerformance/Buffers/StringPool.cs

Line 166 in 1739fb8

public static StringPool Shared { get; } = new StringPool();

For reference, here's the singleton instance ArrayPool<T>.Shared from CoreCLR: link.

Expected behavior

Given that in all these cases we're dealing with singleton instances that are also thread-safe, hence having a similar role (with the given differences due to them being in different types), I figure maybe a single name should be used in all cases. To be consistent with CoreCLR too and since all instances do share the thread-safety feature too, I'm thinking probably Shared?

StringPool will definitely need to use Shared to be consistent with ArrayPool<T>.Shared, so the other two would need to be changed in that case. Otherwise, we could just leave the names different in those cases, looking for feedbacks on this 😄

Additional notes

A decision on this should be made before the 7.0 release is published (including #3230), and I was thinking maybe it'd be best to decide on this before #3424 is merged so that the updated name would already be included in the next preview package for the MVVM Toolkit, so that devs would have more time to get used to that?

Note regarding switching to Shared for messenger, that'd be different from Default being used in MvvmLight, but not necessarily an issue since devs moving from there would already have to make some code changes anyway.

cc. @michael-hawker @jamesmcroft
@msftbot msftbot bot added the needs triage 🔍 label Sep 22, 2020
@msftbot
Copy link
Contributor

@msftbot msftbot bot commented Sep 22, 2020

Hello Sergio0694, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌
@michael-hawker
Copy link
Member

@michael-hawker michael-hawker commented Sep 22, 2020

I see the argument for all the current names, they're all slightly different cases. I think Instance is the original we started with which is kind of the standard for the pattern?

I mean I see developers grabbing the instance of SystemInformation more than them regularly going to grab the other ones?

So, I'm not sure if I see this as a big problem if we want to keep them separate. It also helps for the messengers to not lead developers to think they should be going to go grab the messenger instance and start using it, eh?

@azchohfi thoughts?
@Sergio0694
Copy link
Member Author

@Sergio0694 Sergio0694 commented Sep 22, 2020

Yeah that's a fair point about SystemInformation using Instance, also because that class is mostly just a collection of info rather than a service-like instance actually doing work on its own like the others (especially eg. ArrayPool<T>).

I was mostly thinking about the Default for the messengers, and wondering whether there it wouldn't be clearer to use the same Shared name as the ArrayPool<T> in the BCL to underline the fact that the messengers are also thread-safe? 🤔
@azchohfi
Copy link
Contributor

@azchohfi azchohfi commented Sep 22, 2020

If we change anything, we should deprecate, instruct with a clear warning message, and then delete in a future version. Lets not directly remove any property which would cause a big breaking change and frustration to devs.
I don't mind that much, but agree that consistency would be good. Now... for which one we should pick, that is a complex call hehe
I like Instance way more than Shared, but this is a complete personal view.
@Sergio0694
Copy link
Member Author

@Sergio0694 Sergio0694 commented Sep 22, 2020

"for which one we should pick, that is a complex call hehe"

Ah, naming being the most difficult problem in software engineering strikes back 🤣

If we don't want to deprecate/break things here (which is a good point I can agree with!), I guess then the remaining question for now would mostly just be about what to use for Default in the messengers? As in, whether to leave that and have a 3rd different name, or to switch them to Shared to be at least in line with ArrayPool<T> and StringPool and signal that thread-safety thing?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug 🐛 improvements ✨ introduce breaking changes 💥 open discussion ☎️
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
@azchohfi @Sergio0694 @michael-hawker
You can’t perform that action at this time.