0

Today I was re-factoring a library that I created and share code across multiple platforms (WPF, WF, WP7, WP8).

The idea is that I use inheritance to extend functionality in classes. The top class expose couple of events to the client, this event is raised in a method that is passed down to the base class using the constructors that accept some parameters.

The object is basically used as a singleton.

Example:

public class SomeHandler : BaseHandler
{
   public event EventHandler<Custom1EventArgs> RequestCompleted = delegate {};
   public event EventHandler<Custom2EventArgs> ActionHandled = delegate { };

   protected SomeHandler()
      : base(new CustomUtil(), new CustomRepository(), 
       new AnotherRepository(ADelegateMethod), 
       AnotherDelegateMethod, AnotherOneDelegateMethod) {}

   #region [ Singleton ]
   public static SomeHandler Instance
      {   get { return Nested.instance;}  }

   class Nested
   {
       internal static readonly SomeHandler instance = new SomeHandler ();
   }
   #endregion

   private static void ADelegateMethod(Custom1EventArgs args)
   {
      Instance.RequestCompleted (Instance, args);
   }

   private static void AnotherDelegateMethod(
       Custom2EventArgs args, CustomObject result)
   {
       // Do stuff....
       AnotherCustomObject cusObj = new AnotherCustomObject(args);
       Instance.ActionHandled (Instance, cusObj);
   }

   private static void AnotherOneDelegateMethod(CustomObject result)
   {
       // Do some stuff...
   }
}

OK, if you have noticed, I need to mark the delegate methods as static because the inject is happened in the constructor parameters. But this forced me to make the events static. To workaround I relied to the fact that the user is always using the Instance singleton instance of my object, though the object can be initialized if they want, sealed is not an option at the moment because it is also used as base inherited class to another class in a specific special implementation.

Is it bad to change the events to static? It doesn't seem proper to me, what do you think? Can this design get better?

Actually I use the delegates as a part of code that needs to be executed in a specific time from other objects new AnotherRepository(ADelegateMethod) or the BaseHandler class so i can basically provide the client with information.

MattDavey
  • 8,897
  • 3
  • 31
  • 54
George Taskos
  • 8,324
  • 18
  • 82
  • 147
  • Not sure if I got that correctly, but the whole "delegates" kitchen reminds me just virtual methods calling, but only in pretty weird and trully refined way. he he Is not that right? (If not, then what's the difference?) (And also why do you need a singleton here?) – Agat Nov 10 '13 at 01:30
  • It has nothing to do with virtual methods. Virtual methods will provide an implementation and child classes can override it or implement and call base implementation too. I send a delegate (method) to be invoked in a specific point in runtime from the parent class or another class. I have my reasons to want the user to use only one instance of my module. – George Taskos Nov 10 '13 at 01:46
  • 2
    Well, I am just telling that those delegates look like virtual methods (by logic concept). They are still are private, so they are called only by parent, but this is how virtual methods should work(!) You don't have to define the methods body in parent -- you can mark those as abstract. (You should overrride them anyway then). But if you have it empty (the basic class method body) -- no need to do anything in children (including passing something somewhy to the parent). Please, let me know if I miss something on that. – Agat Nov 10 '13 at 01:51
  • Ok now you are correct, not using virtual methods but abstract I can drop the delegates, implement the methods in the child class and invoke them in the parent where I need, this indeed will save me from 2 of 3 delegates, thanks, never thought of this feature. This will fix the 2 delegates, what happens with the one that is passed to another class that is used as a property in the base class? What do you think? – George Taskos Nov 10 '13 at 01:56
  • Which property do you mean? – Agat Nov 10 '13 at 02:03
  • In the constructor, see this parameter, new AnotherRepository(ADelegateMethod), it's a WebServiceRepository that accepts a delegate as a parameter constructor to be invoked in some point. Do you think that it would be better to use an event inside the WebServiceRepository and handle in the SomeHandler class? BTW if I have ClassC inherits ClassB inherits ClassA the abstract methods will add complexitity, I think, will have to try. – George Taskos Nov 10 '13 at 02:07
  • 1
    Side notes: I've formatted your code to remove too much white spaces... (I agree with Agat that forcing derived class to pass delegates instead of implementing virutal methods or using normal events is strange... Also you've already figured out that design is strange because you can't pass instance methods as delegates in constructor...) – Alexei Levenkov Nov 10 '13 at 02:13
  • Well, pretty many questions! Lots possible to do! First, about abstract/virtual methods: this is what they call "Polymorphism" and exactly why those methods exist! As for WebServiceRepository -- that's hard to judge, as I don't really see the whole logic you want to achive... Possibly, you should decouple the class for even more items, but to understand that you should explain what's the real purpose of the "SomeHandler" and "BaseHandler" at least. – Agat Nov 10 '13 at 02:17
  • Don't worry, I already have the information I need, I think with couple of changes, using abstract/virtual methods and events to some point will make it better in design perspective. Thank you. Please answer this so I can mark it as accepted answer for you. Thanks. – George Taskos Nov 10 '13 at 02:20

2 Answers2

3

The main thing I would suggest to change is:

  • to replace AnotherDelegateMethod and AnotherOneDelegateMethod passing (and then calling from parent class) with corresponding virtual methods.

As for another things, that all depends on the logic your class is suppose to implement. Possibly, some more decoupling might be needed.

  • new AnotherRepository(ADelegateMethod). That's really a tough question how to do everything right -- more info about general logic would be needed, as different approaches possible:

    • you can replace that also with virtual methods, like ADelegateMethod -- the same way as mentioned two above, but new AnotherRepository as protected virtual ISomeRepositoryInterface CreateRepositoryInstance() (for instance);
    • you can use dependency injection into some outside classes which would use your handler to pass the the repository, the same, actually, as Tools or another repository (but, again, everything depends on the general logic);

    • one of the ways to customize your handler(s) is to make the basic class as Generic and to inherit the children with providing some concrete class, which logic is related exactly to the inheritor, but mostly used only in parent class.

About events:

  • I would recommend you to read something like this and generally get familiar with Rx (Reactive Extensions) -- they are modern and awesome (you can consider using them instead of events in many cases (if not in this handler class, then in some other situations)). (However, currently I can't see the situation when you really would need to use events in your class).

Also, as for @durilka's answer (however, that's really funny to trust him considering his nickname "liar" from Russian, he he): he mentioned correctly (before I got into that) about:

  • to remove the nested class and replace with static constructor (if in base class). But that must be carefully rethinked (especially, if you really use multithreading widely).
Agat
  • 4,577
  • 2
  • 34
  • 62
  • Thanks for your time. Consider this was an example to the whole concept but I will look for some more decoupling if needed, imagine that SomeHandler is the client that is responsible for the inject of dependencies, one that uses it shouldn't do anything about.And the concrete classes to the constructor are really interfaces to parent classes. I will re-factor with suggestions. Yes multithreading is important and it will be used. – George Taskos Nov 10 '13 at 12:38
0

Get rid of the Nested. Declare default constructor private. Use it in your Instance property while returning existing instance or creating a new one (aka return instance ?? instance = new SomeHandler()). And start thinking of moving from delegates to some kind of message bus.

durilka
  • 1,399
  • 1
  • 10
  • 22
  • Is this going to be thread safe? What do you mean message bus? Implementing abstract methods as suggested? – George Taskos Nov 10 '13 at 02:17
  • For thread safety, add double locking or use Lazy. By message bus I mean a class that accepts subscribers for particular messages and permits sending those messages without actually knowing if anybody's listening. – durilka Nov 10 '13 at 04:34
  • I would like an example about message bus or a link demonstrating this pattern. Thank you. – George Taskos Nov 10 '13 at 11:38
  • 1
    http://en.wikipedia.org/wiki/Publish%E2%80%93subscribe_pattern I guess, search for "message bus" is currently overhyped on google with unnecessary enterprise stuff. – durilka Nov 11 '13 at 11:01
  • I just wanted something from your research and experience to the topic, it's not that I don't know how/where to type. Thank you. – George Taskos Nov 12 '13 at 19:37
  • I don't posses any exceptional experiences. Most of the time things I know are better formulated on the wikipedia. – durilka Nov 13 '13 at 10:24