paolosalvatori
Refactor the class ServiceBusHelper into multiple classes
To enable migrating the code to use the latest SDK tracks for Relay, Notification hubs, Event hubs and Service bus in a sensible manner, the class ServiceBusHelper should be split into multiple classes, one class for handling each messaging service. It is probably a good idea to have a common, abstract base class for all of them.
This should be done in multiple PRs.
- #816
- #818
- #819
- #820
- #822
- #823
- #824
- #826
- #827
- #828
I can spend some time to (make a start with) this issue. Initially I think it is easier to 'just move code' to separate classes without changing the implementation too much. This should make for smaller PRs that are easier to review.
I was also thinking of making a split between 'messaging actions' and 'management actions', though that might be going a bit overboard?
I was also thinking of making a split between 'messaging actions' and 'management actions', though that might be going a bit overboard?
Agree. Don't bother until there's a need for it.
Also, we should try to get away from "helpers". Those end up being classes with mixed concerns.
If think it would be nice if you split the classes between 'messaging actions' and 'management actions', but it does not need to be 100%. Just where you see you can do it without too much effort.
I have taken a short peek that the current way the messages are send, and a lot of it is via the public MessagingFactory property. A lot of the sending/receiving is already outside the ServiceBusHelper class - it's just not in the correct place: UI controls. But it seems quite challenging to de-couple the UI controls from the specific SDK implementation for sending/receiving messages (creating queue/topic/subscription clients and such), especially with the different structure in the new SDK.
This de-coupling could probably be part of the other issue about using the new SDK? That way this issue can be more focused on refactoring itself to make the later de-coupling easier?
Or is it better to try to do the de-coupling in this issue as well? A lot of the management actions also use/expose classes that are bound to the SDK (looking at you, TopicDescription and friends). These would need to be abstracted in a bunch separate data classes, together with things like ServiceBusHelperEventArgs...
I suggest you go for the easiest (least difficult) path. Small steps are better.
What @ErikMogensen said. UI coupling is an issue all over the place for SBE just because it's a traditional WinForms app that started as a utility and grew into a monster. My personal take would be whatever makes sense. And if the strategy needs to change, then you change it.
To further cleanup the ServiceBusHelper, I thought it would be good to remove all properties and methods that are not being used currently. However, I noticed that there are some System.Reflection usages which could use a property/method and it would not show up in Find References. I wondered if System.Reflection is being used on the ServiceBusHelper class, or if the usage is limited to the EntityDescription (TopicDescription, QueueDescription, etc)?
Good idea! 👍🏻
I don't know and I doubt that anyone else knows since that code was written a long time ago. I suggest you comment out those using directives and everywhere you get a build error you specify the fully qualified name. That should tell you where it is being used.
I wondered if System.Reflection is being used on the ServiceBusHelper class
Not as far as I know. If you suspect reflection is used, might help to search by the type that you want to rule out.