IApplicationMessageReceiver - Proposal

[dotnet]

IApplicationMessageReceiver - Proposal

[chkr1011]

Good proposal BUT the problem (why I didn't added it yet) is that the methods behave completely different:

1. The MqttClient sends it directly. So async is required and it will throw an exception (maybe)
2. The ManagedMqttClient enques it internally. So no async required and no excpetion
3. The MqttServer also just enqueues the message for all subscribers. So no async and exception here

I wasn't able to define a proper interface which matches all clients. Also because the methods have different names (because they performing different operations internally).

One idea is probably a wrapper class which has a uniform Send method and based on the internal client or server it will use the proper method regardless of the name etc.

Do you or anyone else may have a better idea?

[ChristianRiedl]

Yes, I have seen that on one side is sync and the other is async.
I think that there is no difference in the behavior of FunctionA and FunctionB. But anyhow it is not nice.
It requires that the caller handles it in both cases like async function.

Task FunctionA (int arg) 
{
    if (arg < 0)
         throw new InvalidArgumentException ("xxx");
    return Task.Completedtask;
}
void FunctionB (int arg) 
{
    if (arg < 0)
         throw new InvalidArgumentException ("xxx");
}

Fortunatelly your functions have different namings ( XxxxAsync). So the common interface can provide PublishAsync, implemented on the server explicitely and visible only for apps using the IApplicationMessagePublisher interface.

It is simpler to explain when I show some code.

    public interface IApplicationMessagePublisher
    {
        Task PublishAsync();
    }
    public interface IMqttClient : IApplicationMessagePublisher
    {
    }
    public class MqttClient : IMqttClient
    {
        public async Task PublishAsync()
        {
            await Task.Delay(0);
        }
    }
   // not derived from IApplicationMessagePublisher `!!!`
    public interface IMqttServer
    {
        void Publish();
    }
    public class MqttServer : IMqttServer, IApplicationMessagePublisher
    {
        public void Publish()
        {
        }
        Task IApplicationMessagePublisher.PublishAsync()
        {
            Publish();
            return Task.CompletedTask;
        }
    }

[chkr1011]

I added a interface for publishing messages. Please have a look at develop. The interface is implemented for both clients.

[ChristianRiedl]

Christian,

API and interface design is an interesting thing and a matter of taste. So my view of this things is not the only possible.
I write this comment because your current implementation breaks the compatibility with prev version more than necessary.

I think that a standard user writes an application for the server or a type of client (managed or not managed) and he should see in the interface the functions he expects.
And the type of the function async or sync should be clear. When a sync function is required like (server and managed client) he should see in the interface only Publish(msg). This kind of users even do not need interfaces instead of classes. When he uses MqttClient he does not think about IApplicationMessageReceiver.

People writing more generic tools are happy when there are the same interfaces for all client types and the server. And they understand that the interface function is async because one of the interface implementations requires async. These users typically take a look into the sources to understand what is really going on.

For IApplicationMessageReceive the situation is easy. Functionality on client and server is the same so I would move the interface and the event definition to the MQTTNet.Core namespace and use them for client and server.

For IApplicationMessagePublisher the situation is different. On Server and ManageClient it is sync for Normal client it is async.

When the user sees that he can publish only with PublishAsync even when using ManagedClient or Server, than he will be confused that he has to write his app in async style even when only sync is required. I think you should in this case hide the "artificial" PublishAsync so that it is seen only by users who know what they are doing.

How to do it :

IMqttClient can derive from IApplicationMessagePublisher as PublishAsync is the only possibilty.

IManagerMqttClient and IMqttServer should not derive from IApplicationMessagePublisher, both interfaces should contain the old Publish(msg).
This has also the big advantage that you do not break compatibility with prev version.
But MqttServer and MqttManagedClient should implement the additional IApplicationMessagePublisher interface for the advanced user needing this abstraction.

So such user who wants to use the abstraction has to use in this case a cast like

IApplicationMessagePublisher msgPublisher = mqttServer as IApplicationMessagePublisher;

In detail

interface IMqttServer : IApplicationMessageReceiver
{
	...
	void Publish (msg); 
}
class MqttServer : IMqttServer, IApplicationMessagePublisher 
{
	// IMqttServer Implementation
	public void Publish (msg);

	// IApplicationMessagePublisher Implememtation	
	public Task PublishAsync (msg);
	or 
	private Task IApplicationMessagePublisher.PublishAsync (msg);		// hiding PublishAsync in MqttServer
}

Anyhow in my opinion this new Interfaces should be moved to the MQTTNet.Core namespace, they are no more client / server specific. And It can be done without breaking compatibility.

Christian