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.csusing 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<ConnectionStateEventArgs> 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    /// <summary>    /// Initialises a new instance of a Heart    /// </summary>    /// <param name=isserver>Specifies whether this is the server</param>    /// <param name=autostart>specifies if the Heart should start on creation</param>    /// <param name=port>Specifies the port (this will set the server port if isserver is true, else will set the client port)</param>    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();    }    private void Heart_ConnectionStateChanged(object sender, ConnectionStateEventArgs e) {        if (e.ConnectionState == ConnectionState.Connected)            connected = true;        else            connected = false;    }    /// <summary>    /// Initialises a new instance of a Heart    /// </summary>    /// <param name=timespan>Specifies the timespan between each beat</param>    /// <param name=isserver>Specifies whether this is the server</param>    /// <param name=autostart>specifies if the Heart should start on creation</param>    /// <param name=port>Specifies the port (this will set the server port if isserver is true, else will set the client port)</param>    public Heart(double timespan = 1500, bool isserver = false, bool autostart = true,int port = 10000) {        Timer.Interval = timespan;        isServer = isserver;        ep = new IPEndPoint(IPAddress.Any, port);        if (isserver) {            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();    }    #endregion    #region Events    #region handled events    private void clTimer_Elapsed(object sender, System.Timers.ElapsedEventArgs e) {        try {            SendUdpPacket();            ConnectionState = ConnectionState.Connected;        } catch {            ConnectionState = ConnectionState.Disconnected;        }    }    private void srTimer_Elapsed(object sender, System.Timers.ElapsedEventArgs e) {        // Calculate the Timespan since the Last Update from the Client.        timeSinceLastHeartbeat = DateTime.Now.ToUniversalTime() - lastUpdate;        // Set Lable Text depending of the Timespan        if (timeSinceLastHeartbeat > TimeSpan.FromMilliseconds(Timer.Interval))            ConnectionState = ConnectionState.Disconnected;        else            ConnectionState = ConnectionState.Connected;    }    protected void Dispose(bool disposing) {        if (disposing) {            IsDisposed = true;        }        Dispose(disposing);    }    public void Dispose() {        this.stopBeating();    }    #endregion    #endregion    #region Beat    /// <summary>    /// Start the Hearbeat    /// </summary>    public void startBeating() {        Timer.Start();    }    /// <summary>    /// Stop the Heartbeat    /// </summary>    public void stopBeating() {        Timer.Stop();    }    /// <summary>    /// Sends a packet to the HeartServer    /// </summary>    private void SendUdpPacket() {        byte[] data = new byte[1024];        Socket udpClientSocket = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp);        data = Encoding.ASCII.GetBytes(lubdub);        udpClientSocket.SendTo(data, 0, data.Length, SocketFlags.None, ipep);    }    /// <summary>    /// Recieves a packet from the HeartClient    /// </summary>    /// <param name=iar></param>    void ReceiveData(IAsyncResult iar) {        byte[] buffer = new byte[1024];        IPEndPoint sender = new IPEndPoint(IPAddress.Any, 0);        EndPoint tempRemoteEP = (EndPoint)sender;        Socket remote = (Socket)iar.AsyncState;        int recv = remote.EndReceiveFrom(iar, ref tempRemoteEP);        string stringData = Encoding.ASCII.GetString(buffer, 0, recv);        Console.WriteLine(stringData);        lastUpdate = DateTime.Now.ToUniversalTime();        if (!this.IsDisposed) {            udpServerSocket.BeginReceiveFrom(buffer, 0, 1024, SocketFlags.None, ref ep, new AsyncCallback(ReceiveData), udpServerSocket);        }    }    private class ConnectionStateChangedEventArgs {        private ConnectionState value;        public ConnectionStateChangedEventArgs(ConnectionState value) {            this.value = value;        }    }    #endregion}}Enums.csnamespace HakSock { public enum ConnectionState {    Connected,    Disconnected }}
Heartbeat (haksock)
c#;networking
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.  