Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor the class ServiceBusHelper into multiple classes #686

Open
ErikMogensen opened this issue Oct 2, 2022 · 6 comments
Open

Refactor the class ServiceBusHelper into multiple classes #686

ErikMogensen opened this issue Oct 2, 2022 · 6 comments

Comments

@ErikMogensen
Copy link
Collaborator

ErikMogensen commented Oct 2, 2022

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.

@supereddie
Copy link
Contributor

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?

@SeanFeldman
Copy link
Collaborator

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.

@ErikMogensen
Copy link
Collaborator Author

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.

ErikMogensen pushed a commit that referenced this issue Dec 22, 2024
* Move Queue management functions to separate class

---------

Co-authored-by: supereddie <>
SeanFeldman pushed a commit that referenced this issue Jan 1, 2025
* Move Topic management function to separate class, and added base class for entities

* Removed private field

* Add Log method to baseclass and make OnCreated/OnDeleted generic

* Use auto-property for ServiceBusEntity.Scheme

* Re-add async/await to methods that had it originally

---------

Co-authored-by: supereddie <>
SeanFeldman pushed a commit that referenced this issue Jan 10, 2025
* Move Rule management functions to separate class

* Minor refactor for instantiating new servicebus classes

* Remove some unneeded changes

* Minor change to using MessagingFactory

---------

Co-authored-by: supereddie <>
@supereddie
Copy link
Contributor

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

@ErikMogensen ErikMogensen removed their assignment Jan 13, 2025
@ErikMogensen
Copy link
Collaborator Author

I suggest you go for the easiest (least difficult) path. Small steps are better.

@SeanFeldman
Copy link
Collaborator

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.

SeanFeldman pushed a commit that referenced this issue Jan 20, 2025
* Move Relay management functions to separate class

* Don't seal internal class

---------

Co-authored-by: supereddie <>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants