2

I'm currently working on a bot and have about 8 seperate classes for different dialogs (all in the same namespace!). They all contain different Tasks and because of this I have ran into a little problem. I'm wondering what the best practice would be: using an interface, using the factory pattern … All of these options, however, force me to use internal methods. (And I get that with an interface because you promise certain behavior, but with the factory pattern I don't really know - to be honest..) Because there is no definition for the specific methods - but this means I would have to make a lot of these internal methods and I'm trying to avoid redundancy.

First I just instantiated a new object, but I soon realised that this meant a new object of every kind/dialog was made everytime a call was made to the bot - which isn't very efficient right? I have also tried to instanciate them in the constructor, but this forces me to use an interface and this gives me the same problem as I've stated before. I've also looked into partial classes but I'm not sure if using 8 partial classes is really.. okay?

Right now I'm trying out the factory pattern with this code: (Credits to this thread: How to prevent an instantiation of an object in c#)

public class DialogFactory
    {
        private NameDialog _nameDialog;
        private CertificateDialog _certificateDialog;
        private ProfileDialog _profileDialog;
        private ClassDialog _classDialog;
        private LocationDialog _locationDialog;
        private SkillDialog _skillDialog;
        private EducationDialog _educationDialog;
        private SpecializationDialog _specializationDialog;

        public DialogFactory CreateDialog(string dialog)
        {
            switch (dialog.ToLower())
            {
                case "name": return new NameDialog();
                case "certificate": return new CertificateDialog();
                case "profile": return new ProfileDialog();
                case "class": return new ClassDialog();
                case "location": return new LocationDialog();
                case "skill": return new SkillDialog();
                case "education": return new EducationDialog();
                case "specialization": return new SpecializationDialog();
                default: throw new Exception("That dialog does not exist.");
            }

            throw new Exception("That dialog does not exist.");
        }
    }

To give some context of what the dialogs seem like I will add the name dialog here as well:

public class NameDialog : DialogFactory
    {
        ProfileService profileService = new ProfileService();

        public async Task AddNameResponse(ITurnContext turnContext, Profile profile, string value { … }
     }

I try to access the AddNameResponse Task in the main method as follows: await dialog.CreateDialog("name").AddNameResponse(turnContext, profile, value); This is not accepted however giving me the following warning: 'DialogFactory' does not contain a definition for 'AddNameResponse' and no accessible extension method 'AddNameResponse' accepting a frist argument for 'DialogFactory'. The fix would be an internal Task, but I'm trying to avoid this (reason for this given earlier).

I'm really at a loss here because I don't know what the best practice would be. I'm trying to make the most efficient and clean code that I can make and avoid redundancy and use as much loose coupling as possible - but I have no idea how to make that happen in this situation… I hope I've formulated my problem well enough (and also the question)!

Noor VE
  • 67
  • 7
  • 1
    `however, force me to use internal methods` - But Why? This seems to be a fairly normal usecase for an `IDialog` with a `name` property and `show(dialogContext)` method? Why do you think otherwise? – Mat J May 20 '19 at 08:55
  • Is the bot allowed to open multiple dialog instances simultaneously or not? If not then yes, you probably want the instances to be a singleton to save memory, however, I'm not sure why you'd need a factory to do this. Do these `Task`s exist because the IO is async (over the web?) and you need to await the request/response to the bot? If you want to avoid instantiation - you could create a static class and make all instances accessible via that You can't access instance methods when calling the `CreateDialog` method because you are returning the factory itself not a dialog instance... – Charleh May 20 '19 at 08:57
  • @MatJ I was thinking that as well yes! But because the IDialog would be used for all the other dialogs as well this means that I would have to make internals classes in those dialogs because of the behavior promised when using an interface. And this will result into internal tasks in the 7 other and vice versa, which is not really clean in my opinion.. – Noor VE May 20 '19 at 08:58
  • 2
    The return type of `CreateDialog` is `DialogFactory` - but that would mean that all dialog instances would inherit from `DialogFactory` and then you'd have `FactoryCeption`. Is this definitely your code? – Charleh May 20 '19 at 08:58
  • 2
    If you know enough about the dialog that should be presented that you want to provide a response to it, most likely a *general* solution isn't the right thing. Why can't you do `new NameDialog().AddNameResponse(...)` directly? – Lasse V. Karlsen May 20 '19 at 08:59
  • `But because the IDialog would be used for all the other dialogs as well this means that I would have to` - So? Where does the internal methods come to play in there? – Mat J May 20 '19 at 09:01
  • `IDialog` would not give you specific implementation details (such as specific dialog instance methods) - in which case you'd either have to cast to the correct type at the point of use, or design an interface for each dialog type that has those instance methods as part of the interface contract (and cast to that interface). Either way, the consumer **needs** to know about the type-specific methods unless you want to write your whole application using reflection! `IDialog` or some base dialog class would make a factory possible but, still, do you really need a factory? – Charleh May 20 '19 at 09:03
  • @Charleh I've just stumbled across the factory pattern while trying to find a solution haha - but I haven't tried using singletons. So I might try that. The Tasks are async yes and I've tried to make the createclass static but it gives me this warning: `access modifiers are not allowed on static constructors`. – Noor VE May 20 '19 at 09:08
  • @LasseVågsætherKarlsen I haven't really thought of that.. Probably too fixated on creating a special class or interface or whatever to fix the problem. I can do this yes, but now I'm wondering if this really is the most 'formal' way to do this? – Noor VE May 20 '19 at 09:14
  • @Charleh I am aware of the contract when using interfaces (I've also stated that in my post). Making a specific interface for every dialog and calling upon those interfaces again in the main method kind of seemed redundant to me so that was why I tried to do something else (because making one interface for them all and using internal methods also is redudant) and stumbled across the factory pattern.. – Noor VE May 20 '19 at 09:40
  • Well, a static class would be similar to a factory but you'd just keep a static instance of each dialog so you only needed to instantiate once. Can you show some expected "usage" for the factory, show some code where the dialogs are used and some ideas of what you are expecting to write to use them? It might help us understand what you are after or suggest a different approach that fits your need. – Charleh May 20 '19 at 12:59

1 Answers1

2

The factory pattern makes sense for polymorphism. That means that, for example, we want an IDialog and we don't care what the implementation is. We don't want to know. That way our code depends on IDialog and isn't coupled to any particular class that implements it.

If you're trying to do this:

dialog.CreateDialog("name").AddNameResponse(turnContext, profile, value);

...and the error is that whatever gets returned from dialog.CreateDialog("name") doesn't have a AddNameResponse method, that reveals that your code depends on some more specific class than what is returned from the factory.

The factory won't reduce coupling for a few reasons:

  • Your code still depends on NameDialog. You need that exact class with its AddNameResponse method.
  • Even if the factory returned that exact class, now you're coupled to the factory and that class.

It makes sense that you'd want to reduce the coupling, because if a class is tied to NameDialog it's also tied to ProfileService. It's impossible to test a class that depends on NameDialog without also depending on ProfileService.

The potential solutions will involve changing classes that depend on NameDialog. Here are few thoughts:

  • Define an abstraction (such as an interface or a delegate) that describes what your class needs to do with NameDialog. Inject that into your class. Now your class depends on the abstraction, not the concrete class.
  • If NameDialog does something really simple, perhaps you can inject it instead of an abstraction, and a better fix is to define an abstraction that represents ProfileService and inject that into NameDialog.
  • Possibly do both.

What each of these mean is that you're

  • Avoiding coupling by depending on abstractions
  • Giving your dependency injection/IoC container the responsibility of creating objects

That will work better than combining creation of all those objects into a single factory. That approach would only make sense if any object returned by the factory is substitutable for any other type - you don't need to know what the concrete type is.

Scott Hannen
  • 27,588
  • 3
  • 45
  • 62
  • Thumbs up for DI - dependency injection with a registered singleton instance would make this setup a breeze, even if the resulting resolve returns a concrete class - it's still better than coupling to the factory or newing up instances where needed, especially if complexity increases later on down the line. – Charleh May 20 '19 at 14:32
  • Thank you for this anwser! I've researched the dependency injection (I wasn't familiar with it before so it was a nice tip) and found out that it's really easy to achieve with ASP.NET (https://learn.microsoft.com/en-us/aspnet/core/fundamentals/dependency-injection?view=aspnetcore-2.2). I've also refactored my code and added two interfaces - however the Dialog interface now does sometimes use the wrong object. I'm thinking about using the Strategy pattern to solve this, but I was wondering what your opinion is regarding this matter? – Noor VE May 21 '19 at 10:46
  • I don't have the information to say why the interface sometimes uses the wrong object. I recommend experimenting a lot with it. Three years ago it was hard to mention DI because it was like opening a big can of worms, but now it's more familiar since it's part of ASP.NET Core. That was more helpful than I realized because now everyone is exposed to it. – Scott Hannen May 21 '19 at 11:47