4
\$\begingroup\$

Is it appropriate for the class to subscribe to its own events like this? Should the class not subscribe, and instead move that code to before the OnEventName(Object args) methods call the EventHandlers? I'm curious if there are any downfalls to this approach.

On one hand, with this approach I could remove all the null checks on the event-firing methods. On the other hand, what if another event down the road needs information from the event handling I have already added a subscription to. I.e. in the future a method needs the NetworkServer_Error method to have been guaranteed to have fired before it can work.

I'm mostly concerned with the event logic here, not so much the implementations. None of these implementations are concrete, but I want to make sure I have done this part properly first.

public class NetworkServer
{
    NetPeerConfiguration mainServerConfiguration;
    NetServer mainNetServer;
    List<NetConnection> connectedClients = new List<NetConnection>();

    public NetworkServer()
    {
        mainServerConfiguration = new NetPeerConfiguration("SecretKey");
        mainServerConfiguration.AcceptIncomingConnections = true;
        mainServerConfiguration.Port = 5501;
        mainNetServer = new NetServer(mainServerConfiguration);

        // Wire up our events
        Error += NetworkServer_Error;
        DataReceived += NetworkServer_DataReceived;
        StatusChanged += NetworkServer_StatusChanged;
        ClientConnected += NetworkServer_ClientConnected;
        ClientDisconnected += NetworkServer_ClientDisconnected;
    }

    void NetworkServer_DataReceived(object sender, NetMessageEventArgs e)
    {
        Program.LogLine("Data recieved from: " + e.Message.SenderConnection.RemoteEndPoint.ToString() + ", Payload size: " + e.Message.LengthBytes.ToString(), LoggingType.Information);
    }

    void NetworkServer_Error(object sender, MessageEventArgs e)
    {
        Program.LogLine(e.Message, LoggingType.Error);
    }

    void NetworkServer_ClientConnected(object sender, ConnectionEventArgs e)
    {
        if (!connectedClients.Contains(e.Connection))
        {
            connectedClients.Add(e.Connection);
            Program.LogLine("New client discovered: " + e.Connection.RemoteEndPoint.ToString(), LoggingType.Information);
        }
    }

    void NetworkServer_ClientDisconnected(object sender, ConnectionEventArgs e)
    {
        if (connectedClients.Contains(e.Connection))
            connectedClients.Remove(e.Connection);

        Program.LogLine("Client lost: " + e.Connection.RemoteEndPoint.ToString(), LoggingType.Information);
    }

    void NetworkServer_StatusChanged(object sender, StatusChangedEventArgs e)
    {
        switch (e.Connection.Status)
        {
            case NetConnectionStatus.Disconnected:
                OnClientDisconnected(new ConnectionEventArgs(e.Connection));
                break;
            case NetConnectionStatus.Connected:
                OnClientConnected(new ConnectionEventArgs(e.Connection));
                break;
            default:
                Program.LogLine("Unhandled StatusChanged: " + e.Connection.RemoteEndPoint.ToString() + " now " + e.Connection.Status + ".", LoggingType.Warning);
                break;
        }
    }

    public Task RunServer()
    {
        return Task.Run(() =>
        {
            mainNetServer.Start();
            DateTime started = DateTime.UtcNow;

            Program.LogLine(string.Format("The server was started on {0} at {1}.", started.ToString("dd-MM-yyyy"), started.ToString("HH:mm:ss.fffffff")), LoggingType.Important);

            while (true)
            {
                NetIncomingMessage msg;
                while ((msg = mainNetServer.ReadMessage()) != null)
                {
                    switch (msg.MessageType)
                    {
                        case NetIncomingMessageType.VerboseDebugMessage:
                        case NetIncomingMessageType.DebugMessage:
                        case NetIncomingMessageType.WarningMessage:
                        case NetIncomingMessageType.ErrorMessage:
                            OnError(new MessageEventArgs(msg.ReadString()));
                            break;
                        case NetIncomingMessageType.StatusChanged:
                            OnStatusChanged(new StatusChangedEventArgs(msg.SenderConnection));
                            break;
                        case NetIncomingMessageType.Data:
                            OnDataReceived(new NetMessageEventArgs(msg));
                            break;
                        default:
                            Program.LogLine("Unhandled type: " + msg.MessageType, LoggingType.Warning);
                            break;
                    }
                    mainNetServer.Recycle(msg);
                }

                System.Threading.Thread.Sleep(1);
            }
        });
    }

    void OnError(MessageEventArgs args)
    {
        if (Error != null)
            Error(this, args);
    }

    void OnStatusChanged(StatusChangedEventArgs args)
    {
        if (StatusChanged != null)
            StatusChanged(this, args);
    }

    void OnDataReceived(NetMessageEventArgs args)
    {
        if (DataReceived != null)
            DataReceived(this, args);
    }

    void OnClientDisconnected(ConnectionEventArgs args)
    {
        if (ClientDisconnected != null)
            ClientDisconnected(this, args);
    }

    void OnClientConnected(ConnectionEventArgs args)
    {
        if (ClientConnected != null)
            ClientConnected(this, args);
    }

    public event MessageEventHandler Error;
    public event NetMessageEventHandler DataReceived;
    public event StatusChangedEventHandler StatusChanged;
    public event ConnectionChangedEventHandler ClientConnected;
    public event ConnectionChangedEventHandler ClientDisconnected;
}

Here are the EventHandler delegates:

public delegate void MessageEventHandler(Object sender, MessageEventArgs e);
public delegate void NetMessageEventHandler(Object sender, NetMessageEventArgs e);
public delegate void StatusChangedEventHandler(Object sender, StatusChangedEventArgs e);
public delegate void ConnectionChangedEventHandler(Object sender, ConnectionEventArgs e);

And, lastly, the EventArgs classes:

public class ConnectionEventArgs : EventArgs
{
    private NetConnection _Connection;
    public NetConnection Connection { get { return _Connection; } }

    public ConnectionEventArgs(NetConnection client)
    {
        this._Connection = client;
    }
}

public class MessageEventArgs : EventArgs
{
    private string _Message;

    public string Message { get { return _Message; } }

    public MessageEventArgs(string message)
    {
        this._Message = message;
    }
}

public class NetMessageEventArgs
{
    public NetIncomingMessage Message { get; private set; }

    public NetMessageEventArgs(NetIncomingMessage message)
    {
        Message = message;
    }
}

public class StatusChangedEventArgs : ConnectionEventArgs
{
    public StatusChangedEventArgs(NetConnection client)
        : base(client)
    {
    }
}

Lastly, the Program class calling the NetworkServer:

class Program
{
    static LoggingType logMessageTypes = LoggingType.All;

    static void Main(string[] args)
    {
        NetworkServer ns = new NetworkServer();
        Task t = ns.RunServer();
        t.Wait();
        LogLine("Done!", LoggingType.Information);
    }

    public static void LogLine(string line, LoggingType type, ConsoleColor foreColor = ConsoleColor.Gray, ConsoleColor backColor = ConsoleColor.Black)
    {
        if (type <= logMessageTypes)
        {
            Console.ForegroundColor = foreColor;
            Console.BackgroundColor = backColor;
            Console.WriteLine(DateTime.UtcNow.ToString("O") + ": " + type.ToString() + ": " + line);
        }
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ I have edited your question in a way that the context is at the top. This will lead to more text than "Suppose I have the following:" in the question list and will attract more users. If you feel this shouldn't be the case, feel free to rollback. \$\endgroup\$ – Heslacher Jul 9 '15 at 4:48
3
\$\begingroup\$

Basically the communication between objects is done in two different ways.

  • A parent object is talking to its child object by using the child objects methods and properties.

  • A child object is talking to its parent by using events.

So I support @Thomas W with his answer.

That beeing said, let us start with the refactoring ideas.


The EventArgs classes

  • You should either make the variables you use public readonly omiting the property or use autoimplemented properties with a private setter, because they are only read but never written.

  • The NetMessageEventArgs class is missing the inheriting from EventArgs which should be done.
    See: https://stackoverflow.com/a/6816889/2655508

    it allows people using your classes to use and handle generic *Handler(object sender, EventArgs e) declarations. If you don't inherit from EventArgs, then they have to use explicitly typed

  • I don't see a reason to name the constructors parameter client in the ConnectionEventArgs and StatusChangedEventArgs classes. A more obvious name would be connection.

The result of the applied changes (which aren't needed anymore, but posting them nevertheless)

public class ConnectionEventArgs : EventArgs
{
    public NetConnection Connection { get; private set; }

    public ConnectionEventArgs(NetConnection connection)
    {
        Connection = connection;
    }
}

public class MessageEventArgs : EventArgs
{
    public string Message { get; private set; }

    public MessageEventArgs(string message)
    {
        Message = message;
    }
}

public class NetMessageEventArgs : EventArgs
{
    public NetIncomingMessage Message { get; private set; }

    public NetMessageEventArgs(NetIncomingMessage message)
    {
        Message = message;
    }
}

public class StatusChangedEventArgs : ConnectionEventArgs
{
    public StatusChangedEventArgs(NetConnection connection)
        : base(connection)
    {
    }
}

NetworkServer class

  • mainNetServer and connectedClients should be made readonly so you won't accidently assign some new value to them.

  • You should move the NetPeerConfiguration mainServerConfiguration; inside the constructor, because you only use it there. Or much better you should inject it into the constructor. Your class does not need to know how it is created nor should it need to create it. It only needs to use it.

  • You should declare the NetPeerConfiguration mainServerConfiguration; and NetServer mainNetServer; explicitly private to make this more obvious.

  • Instead of using a List<NetConnection> I would like to encourage you to use a HashSet<NetConnection>. In this way you don't need to check if item is in the set, you can just call Add() and don't have to worry about it beeing already in the set.

  • based on your comments to @Thomas W's answer the only object which is using the provided events is the object itself. So you could use simple methods leaving aside the events.

  • you should use a ILogger interface which should be injected to the constructor instead of using a public static method of the calling object.

  • with using string.Format() you can make the messages easier to read and could make them constant if you want to.

The result of the applied changes

public class NetworkServer
{

    private readonly NetServer mainNetServer;
    private readonly HashSet<NetConnection> connectedClients = new HashSet<NetConnection>();

    public NetworkServer(NetPeerConfiguration mainServerConfiguration)
    {
        mainNetServer = new NetServer(mainServerConfiguration);
    }

    private void ProcessReceivedMessage(NetIncomingMessage message)
    {
        string msg = "Data recieved from: {0}, Payload size: {1}"
        LogMessage(msg, LoggingType.Information,  
                   message.SenderConnection.RemoteEndPoint, message.LengthBytes);
    }

    private void LogMessage(string message, LoggingType loggingType, params Object[] par)
    {
        message = FormatMessage(message, par);
        Program.LogLine(message, loggingType);
    }

    private String FormatMessage(string message, params Object[] par)
    {
        if (par.Length == 0)
        {
            return message;
        }
        return string.Format(message, par);
    }

    private void ProcessErrorMessage(string message)
    {
        LogMessage(message, LoggingType.Error);
    }

    private void AddClient(NetConnection connection)
    {
        if (connectedClients.Add(connection))
        {
            string msg = "New client discovered: {0}";
            LogMessage(msg, LoggingType.Information, connection.RemoteEndPoint)
        }
    }

    private void RemoveClient(NetConnection connection)
    {
        connectedClients.Remove(connection);
        string msg = "Client lost: {0}";
        LogMessage(msg, LoggingType.Information, connection.RemoteEndPoint)
    }

    private void ProcessStatusChangedMessage(NetConnection connection)
    {
        switch (connection.Status)
        {
            case NetConnectionStatus.Disconnected:
                RemoveClient(e.Connection);
                break;
            case NetConnectionStatus.Connected:
                AddClient(e.Connection);
                break;
            default:
                LogMessage("Unhandled StatusChanged: {0} now {1}.",  
                            LoggingType.Warning,  
                            e.Connection.RemoteEndPoint,  
                            e.Connection.Status);
                break;
        }
    }

    public Task RunServer()
    {
        return Task.Run(() =>
        {
            mainNetServer.Start();

            DateTime started = DateTime.UtcNow;

            LogMessage("The server was started on {0} at {1}.",  
                        LoggingType.Important,  
                        started.ToString("dd-MM-yyyy"),  
                        started.ToString("HH:mm:ss.fffffff"));

            while (true)
            {
                NetIncomingMessage msg;
                while ((msg = mainNetServer.ReadMessage()) != null)
                {
                    switch (msg.MessageType)
                    {
                        case NetIncomingMessageType.VerboseDebugMessage:
                        case NetIncomingMessageType.DebugMessage:
                        case NetIncomingMessageType.WarningMessage:
                        case NetIncomingMessageType.ErrorMessage:
                            ProcessErrorMessage(msg.ReadString());
                            break;
                        case NetIncomingMessageType.StatusChanged:
                            ProcessStatusChangedMessage(msg.SenderConnection);
                            break;
                        case NetIncomingMessageType.Data:
                            ProcessStatusChangedMessage(msg);
                            break;
                        default:
                            LogMessage("Unhandled type: {0}", LoggingType.Warning,  
                                       msg.MessageType);                 
                            break;
                    }
                    mainNetServer.Recycle(msg);
                }

                System.Threading.Thread.Sleep(1);
            }
        });
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thank you greatly for this information! It was very helpful. (As were the other posts, though this one was just a tad more helpful than them.) I really appreciate the critiques. I also appreciate the events critiques, even though they are no longer needed. It still helps me remember how to properly handle them when I approach a situation where I might need them. \$\endgroup\$ – Der Kommissar Jul 9 '15 at 13:53
5
\$\begingroup\$

I'm going to suggest "NO" as the answer to this design :)

A few general principles:

  1. Events are for exposing lower-level state/change to higher-level actors, and enabling versatility of coupling.
  2. Higher-level actor would be independent & in a different class.
  3. Here, it's unclear which level of abstraction you are actually trying to build -- is it one coherent level, which shouldn't rely on events, or two separate levels coupled by events, which should be in separate classes?
  4. Events don't seem to me a suitable approach for "null checks"/ null handling.
  5. Event-handling is not guaranteed as to sequence & it's pretty unsafe/brittle to rely on sequentiality of handling (re: your NetworkServer_Error question near the end).

In general, it's probably best to build a solid mechanism with it's own clear internals, and decouple that from the published API (events etc) visible outwards.

\$\endgroup\$
  • 1
    \$\begingroup\$ Now that you mention it, I think you might be right about this. I never realized (until just now) that each of these events will only ever have one object/method subscribed to them. The only one that might need to do more, is the Error event. But even that won't really have much more to do. I'm starting to think I need to break a lot of this out into smaller classes. I added the Program class which will be creating and calling this server as well. \$\endgroup\$ – Der Kommissar Jul 9 '15 at 1:16
  • \$\begingroup\$ Now, where would you recommend I start with rewriting this, keeping in mind that this class is responsible for everything in this project? It will most certainly grow out of control very quickly. \$\endgroup\$ – Der Kommissar Jul 9 '15 at 1:44
2
\$\begingroup\$
  • I think your while(true) loop misses an exit point. At least I can't seem to find any.
  • I think using Thread.Sleep is an anti-pattern in most cases. I don't know how your NetServer is implemented, but Socket and NetworkStream both have the means to block the thread until data is available to read. You should use those, instead of constantly switching context between your threads when there is no data available to read.

As for your question - I agree with others. The cases where you might want to subscribe object to its own event are rare. If you can avoid it, then you should not do it.

\$\endgroup\$
  • 1
    \$\begingroup\$ good point about the while(true). \$\endgroup\$ – Heslacher Jul 9 '15 at 9:43
  • \$\begingroup\$ Unfortunately, the network handling is not the only thing the server has to do. It also has to manage a plethora of state changes to objects that will be updating state constantly, and have no true dependence on the networking issues. (That said, I might make those a separate thread/loop to help segregate the code.) \$\endgroup\$ – Der Kommissar Jul 9 '15 at 11:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.