search

Lakritzator / harmony

C# library for connecting to and controlling the Logitech Harmony Hub.
MIT License
5 stars 0 forks source link

Have a tried and tested Open and Close methods for our client #17

Closed Slion closed 8 years ago

Slion commented 8 years ago

We should just remove our static Create method and have proper Open and Close methods on our client. We could still have a static constructor that does a new and open but I would not bother really.

Lakritzator commented 8 years ago

The problem with constructors is that you can't use async code in them, a factory method is a logical replacement. The close is done via the dispose, which is a generally accepted and much used pratice in C#.

There must be reason behind your request, one I don't understand with the little information you gave. But do mind, C# is not C nor C++. There are implementation patterns, which are usual for the language.

Slion commented 8 years ago

You can checkout the changes on my branch. I implemented most of that.

I once read all you need to know about Dispose and more but I probably already forgot most of it. Is Dispose not supposed to get called by the framework just before your object is actually deleted? If I recall well I don't think you are suppose to keep using an object after calling its Dispose.

Most remote connection client usually implement Close and Open methods. That's what XMPP client does for instance. The idea is that you can keep on using the same instance of the object while Opening and Closing it multiple times. That's a really good habit if you want to check your object implementation is clean and re-entrant. It's a fairly common way to model any connection object, simply because a connection can be opened and closed.

The object constructor is lightweight and takes as parameter whatever is expected not to change between open and close calls, again same as our XMPP client.

Slion commented 8 years ago

Implemented. Things could still be improved when it comes to aborting the open process but that's outside the scope of that ticket.

Lakritzator commented 8 years ago

I wrote some comments on your latest changes, I hope they are not to harsh, but you introduced a lot of potential errors and I didn't get what you wanted to accomplish by the changes.

Slion commented 8 years ago

Any comments are welcome.