Minor tweaks to allow to programmatically configure providers.

[gabikliot]

Basically, made one method public instead of internal plus protection against null ref in a couple of places.

[gabikliot]

2 different way to solve the same problem. I picked that way. Why does it matter?

[yevhen]

Just an easier way. And less code. And more readable (if you care about readers) :)

[gabikliot]

Logically, the configuration may have children and may not to. So from that perspective I find it cleaner to specify "I don't have children" via null value vs. empty array. There is also an issue of efficiency - redundant memory allocation. We had that discussion in the past: #18. And of course it is an internal implementation detail (the field is private) so the efficiency may be more important than a more intuitive API.
I think the key point is that this minor detail really does not matter much and is up to the implementer.

[yevhen]

But how does it affect efficiency? It's done only once on startup and then discarded. It's a throw-away object. We're not talking about something like GrainReference.

Logically, the configuration may have children and may not to.

Indeed. Empty list means there no children. Iteration will stop at that point since there nothing to iterate.

I think the key point is that this minor detail really does not matter much and is up to the implementer.

Nope. You're doing an OSS, and you want ppl to contribute, so you should care about them reading your code. The code is written once but read many times. Anyone can write programs for computer but writing them for ppl is something that need to be mastered.

P.S. Not for this case, but the general observation is: API has always more priority than anything else. You create it for other developers to consume. The success of your project depends on whether they will be satisfied with API and documentation/samples or not. It's something I learnt the hard way.

P.P.S. Null checks all over the place are considered a code smell. Switch to eager initialization where possible and myriad of this ugly checks will go away. Abuse of lazy initialization is a plague of current Orleans' codebase.

[sergeybykov]

I think this is a good point. There are a handful of hot paths in the code where not allocating objects matters for performance. In most other places empty lists/dictionaries can be cleaner and easier to read.

[yevhen]

I understand that this is minor issue and we can forget about any good programming practices, for sure. So you can forget about that, I'm too tired today to fight around such a crumb. It's still doesn't fix the big picture around programmatic configuration of Boostrappers, anyway :)

Maybe, I'll find some time to make an API close to:

config.Global.RegisterBoostrapper<ServiceLocatorBootstrapper>();

If I won't sink in a sea of null checks ....

[yevhen]

Sorry, guys, If I offended in you some way. It's 3AM here and that last whiskey glass was extra :)

[gabikliot]

"It's still doesn't fix the big picture around programmatic configuration of Boostrappers, anyway :)"

I actually think it does.
I think programmable configuration of providers works now.
Can you please provide more details of what exactly does not work?

[sergeybykov]

@yevhen No, you did not offend anybody. You brought a valid and important point. Thank you for that!

[yevhen]

@gabikliot I didn't say it doesn't work. It does. But it's still cumbersome. With your change, I can now skip part related to setting childConfigurations and childProviders to new List<XXX> via reflection. I've outlined steps required to register custom BootstrapProvider here.

What's still left is this:

void RegisterBootstrapProviders(params[] Type bootstrapProviders)
{
    var category = Configuration.Globals.ProviderConfigurations.Find("Bootstrap");

    if (category == null)
    {
        category = new ProviderCategoryConfiguration
        {
            Name = "Bootstrap",
            Providers = new Dictionary<string, IProviderConfiguration>()
        };

        Configuration.Globals.ProviderConfigurations.Add("Bootstrap", category);
    }

    foreach (var bootstrapProvider in bootstrapProviders)
        Register(bootstrapProvider, category);
}

public void Register(Type bootstrapProvider, ProviderCategoryConfiguration category)
{
    var fullName = bootstrapProvider.FullName;
    var config = new ProviderConfiguration(properties, fullName, fullName);
    category.Providers.Add(config.Name, config);
}

So, how on Earth, should an end-user (developer) know, without diving into Orelans sources, that in order to register bootstrap provider, he need to:

1. Create some special object which is ProviderCategoryConfiguration
2. And it should have a magical name Bootstrap
3. And it should be then added to special Configuration.Globals.ProviderConfigurations property
4. And I should also check whether it's already there or not (I can have mixed xml/code configuration)
5. And that I should also pre-initialize its Providers property to a new Dictionary<string, IProviderConfiguration>, since it's null by default (same problem with abuse of lazy init as we were talking before)
6. And then there is also another special ProviderConfiguration object, which he need to create and fill in a right way
7. How should developer reckon that the string, required by ProviderConfiguration should be a full type name? Why not just type name? Maybe it should be an assembly qualified name?
8. What should he put into the second parameter of ProviderConfiguration ctor?
9. And don't forget to add it then to ProviderCategoryConfiguration under respective name? Is first parameter of Add is important? Maybe. Maybe not.

I can continue with this list, believe me (I started my carrier 15 years ago as QA engineer :), but I hope you get the point. There is too much (magic) code involved for such a casual task as registering bootstrap provider, which in my opinion is a crucial element of the framework. Would you put all of these steps into documentation of the BoostrapProvider? I don't think so.

The fact that you've generalized all kinds of providers is an inner detail. It should not be visible to clients. You used a code compression technique, which has it's cost, such as clarity. It's ok to use such generalizations but when its visible in public API it looks really weird :)

That's why I wrote, that such an important element as BoostrapProvider deserves a special treatment:

config.Global.RegisterBoostrapper<ServiceLocatorBootstrapper>();

All of that burden could be hidden behind that simple Facade method.

[gabikliot]

Such a façade could indeed be a useful enhancement. It will make a programmatic configuration of bootstrap providers easier. It would be great to see your PR in that area.

[veikkoeeva]

I realise I'm late, perhaps this adds something to the conversation:

DO NOT return null values from collection properties or from methods returning collections. Return an empty collection or an empty array instead.

-- Guidelines for Collections

There's a thread in SO that has more discussion around this.

I suspect many of the seasoned .NET developers expect most of the guidelines should be followed and these issues weigh considerably when tools are chosen, at least in the projects I've been involded in (sometimes, oftentimes, nobody seems to care, but that tend to show in results too). Nevertheless, this was a good discussion on the pros and cons in this situation. I wonder if we should add some notes to contributor guidelines?

[gabikliot]

Thank you @veikkoeeva for the useful links!
The main difference I see is that in the discussion above we are not designing a public API. No collection is returned anywhere (neither empty nor null). It is an internal implementation detail of this class, the field we are taking about it private.
For public APIs I could not agree more.

[veikkoeeva]

@gabikliot Yep. I didn't see contributor policies written, but one possible formulation I see is that on public surface following the Framework Design Guidelines is preferred. I would put it so that it's preferred internally too, but it's OK to do otherwise too when reasons arise. What comes to me, I'd be OK someone just commenting to the code "we'll use null to optimize storage, performance etc. and this code isn't expected to change and these shouldn't leak outside", or something similar.

As I would see it, it would lower the barrier for random people to do actual refactoring, however small the improvements would be, and be assured even small contributions could make it to the codebase (work won't go in vain, all work is good work and worth doing). Good for cleaning up code rot and technical debt, I think. Perhaps, when there's a chat and wider discussion with random people (like me), it'd provide a starting point to argument for and against with stable basis.

[gabikliot]

Yes, I agree.