Question

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.

Was it helpful?

Solution

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).

OTHER TIPS

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.

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top