Heartbeat (haksock)

I've recently been working on a small project to provide a simple heartbeat between server and client applications. Pretty happy with it, just wondering if it could function better (I know it could use cleaning, and a standard naming convention, I like to code quick and dirty then clean up)

Any advice would be greatly appreciated.

Haksock.cs

```
using System;
using System.Net;
using System.Net.Sockets;
using System.Text;

namespace HakSock {
public class Heart : IDisposable{
public sealed class ConnectionStateEventArgs : EventArgs {
public ConnectionState ConnectionState { get; private set; }
public ConnectionStateEventArgs(ConnectionState connectionState) {
ConnectionState = connectionState;
}
}
public event EventHandler ConnectionStateChanged;
#region Globals
private System.Timers.Timer Timer = new System.Timers.Timer();
private IPEndPoint ipep;
bool isServer;
public bool connected = false;
private byte[] buffer = new byte[1024];
private DateTime lastUpdate;
private Socket udpServerSocket;
private EndPoint ep;
public TimeSpan timeSinceLastHeartbeat;
private ConnectionState _connectionState = ConnectionState.Disconnected;
public ConnectionState ConnectionState {
get { return _connectionState; }
set {
if (value != _connectionState) {
_connectionState = value;
var tmp = ConnectionStateChanged;
if (tmp != null)
tmp(this, new ConnectionStateEventArgs(value));
}
}
}
public bool IsDisposed {
get;
private set;
}
#endregion

#region constructor/destructor
///
/// Initialises a new instance of a Heart
///
/// Specifies whether this is the server
/// specifies if the Heart should start on creation
/// Specifies the port (this will set the server port if isserver is true, else will set the client port

Your code would have a bug related to the protected Dispose(bool) method

protected void Dispose(bool disposing) {
    if (disposing) {
        IsDisposed = true;
    }
    Dispose(disposing);
}

if you would use it. Luckily you don't use it, otherwise you would get likely get some kind of overflow exception.

So please remove this method so you won't trap inside the trap.

At a second thought it may be better to keep it after doing it right, because the ReceiveData() method relies on the IsDisposed property.

private void Heart_ConnectionStateChanged(object sender, ConnectionStateEventArgs e) {
    if (e.ConnectionState == ConnectionState.Connected)
        connected = true;
    else
        connected = false;
}

this should be simplified to just

private void Heart_ConnectionStateChanged(object sender, ConnectionStateEventArgs e) {
    connected = e.ConnectionState == ConnectionState.Connected;
}

The Heart class could use some constructor chaining, which removes some duplicated code.

So this

public Heart(bool isserver, bool autostart, int port = 10000) {
    ;
    Timer.Interval = 1500;
    Timer.AutoReset = true;
    isServer = isserver;
    this.ConnectionStateChanged += Heart_ConnectionStateChanged;
    if (isserver) {
        ep = new IPEndPoint(IPAddress.Any, port);
        Timer.Elapsed += srTimer_Elapsed;
        udpServerSocket = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp);
        udpServerSocket.Bind(ep);
        udpServerSocket.BeginReceiveFrom(buffer, 0, 1024, SocketFlags.None, ref ep, new AsyncCallback(ReceiveData), udpServerSocket);
        udpServerSocket.Ttl = 255;
    } else {
        Timer.Elapsed += clTimer_Elapsed;
        ipep = new IPEndPoint(IPAddress.Loopback, port);
    }
    if (autostart)
        startBeating();

}

would become this

public Heart(bool isserver, bool autostart, int port = 10000) 
        :this(1500, isserver, autostart, port)
{}

Seeing something like this

private System.Timers.Timer Timer = new System.Timers.Timer();
private IPEndPoint ipep;
bool isServer;

just doesn't look right. You should always add access modifiers to your properties and methods making the indent more clear.

Please read regarding the use of regions: Are #regions an antipattern or code smell?

public ConnectionState ConnectionState {
    get { return _connectionState; }
    set {
        if (value != _connectionState) {
            _connectionState = value;
            var tmp = ConnectionStateChanged;
            if (tmp != null)
                tmp(this, new ConnectionStateEventArgs(value));
        }
    }
}

I don't really like that setter, it is doing IMO to much. You should consider to extract the raising of the event to a separate method like so

protected void OnConnectionStateChanged(ConnectionStateEventArgs e)
{
    var stateChanged = ConnectionStateChanged;
    if (stateChanged != null)
    {
        stateChanged(this, e));
    }
}

to be called form the setter like so

set {
        if (value != _connectionState) {
            OnConnectionStateChanged(new ConnectionStateEventArgs(value));
        }
    }

private class ConnectionStateChangedEventArgs {
    private ConnectionState value;

    public ConnectionStateChangedEventArgs(ConnectionState value) {
        this.value = value;
    }
}

You should make the ConnectionState value readonly.

Using braces {} although they might be optional will make your code less error prone.

StackExchange Code Review Q#111015, answer score: 3