2
\$\begingroup\$

I have written this code in C# and have tested this code to ensure it works well. Any other suggestions for this code would be appreciated as I have a number of users who are sending data on this port.

private Socket serverSocket = null;
private List<EndPoint> clientList = new List<EndPoint>();
private byte[] byteData = new byte[1024];
private int port = 1517;




private void button1_Click(object sender, EventArgs e)
{
    this.serverSocket = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp);
    this.serverSocket.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ReuseAddress, true);
    this.serverSocket.Bind(new IPEndPoint(IPAddress.Any, this.port));
    EndPoint newClientEP = new IPEndPoint(IPAddress.Any, 0);
    this.serverSocket.BeginReceiveFrom(this.byteData, 0, this.byteData.Length, SocketFlags.None, ref newClientEP, DoReceiveFrom, newClientEP);

}

private void DoReceiveFrom(IAsyncResult iar)
{


    try
    {
        EndPoint clientEP = new IPEndPoint(IPAddress.Any, 0);
        int dataLen = 0;
        byte[] data = null;
        try
        {
            dataLen = this.serverSocket.EndReceiveFrom(iar, ref clientEP);
            data = new byte[dataLen];
            Array.Copy(this.byteData, data, dataLen);
        }
        catch (Exception e)
        {
        }
        finally
        {
            EndPoint newClientEP = new IPEndPoint(IPAddress.Any, 0);
            this.serverSocket.BeginReceiveFrom(this.byteData, 0, this.byteData.Length, SocketFlags.None, ref newClientEP, DoReceiveFrom, newClientEP);
        }

        if (!this.clientList.Any(client => client.Equals(clientEP)))
            this.clientList.Add(clientEP);

        //     DataList.Add(Tuple.Create(clientEP, data));

        string s = Encoding.ASCII.GetString(data);


    }
    catch (ObjectDisposedException)
    {
    }
}

public void Stop()
{
    this.serverSocket.Close();
    this.serverSocket = null;
    this.clientList.Clear();
}
\$\endgroup\$
  • 1
    \$\begingroup\$ You shouldn't catch exceptions you can not handle, aka no empty catch. \$\endgroup\$ – MX D Dec 8 '16 at 14:14
  • 2
    \$\begingroup\$ Please add some description of what your code does, why it does it, and any particular concerns you have about the code. Your question is collection votes to be put on hold as "unclear what you're asking". \$\endgroup\$ – RubberDuck Dec 28 '16 at 12:46
  • 1
    \$\begingroup\$ @RubberDuck While I agree this question could've been greatly improved by adding more context, doesn't the title give away enough to know what it's doing? \$\endgroup\$ – Mast Dec 28 '16 at 13:34
  • \$\begingroup\$ I think so @Mast, but others are casting close votes. \$\endgroup\$ – RubberDuck Dec 28 '16 at 13:58
  • \$\begingroup\$ Hey Thanks for your comment . We have gps device that send udp packet on server via ip and port I written this code and working good. \$\endgroup\$ – Shridhar Dec 29 '16 at 6:21
2
\$\begingroup\$

@MX D is correct in that you should not be catching exceptions, unless you are going to do something about them.

This code can and should be written without catch statements.

private void DoReceiveFrom(IAsyncResult iar)
{


    try
    {
        EndPoint clientEP = new IPEndPoint(IPAddress.Any, 0);
        int dataLen = 0;
        byte[] data = null;
        try
        {
            dataLen = this.serverSocket.EndReceiveFrom(iar, ref clientEP);
            data = new byte[dataLen];
            Array.Copy(this.byteData, data, dataLen);
        }
        catch (Exception e)
        {
        }
        finally
        {
            EndPoint newClientEP = new IPEndPoint(IPAddress.Any, 0);
            this.serverSocket.BeginReceiveFrom(this.byteData, 0, this.byteData.Length, SocketFlags.None, ref newClientEP, DoReceiveFrom, newClientEP);
        }

        if (!this.clientList.Any(client => client.Equals(clientEP)))
            this.clientList.Add(clientEP);

        //     DataList.Add(Tuple.Create(clientEP, data));

        string s = Encoding.ASCII.GetString(data);


    }
    catch (ObjectDisposedException)
    {
    }
}

you should also remove some of the empty space from the code as well.

The Commented code...what is it there for? If this is dead code then you should remove it.

Declaring a Variable at the end of this code has me a little confused....

  string s = Encoding.ASCII.GetString(data);

Get rid of that too, it isn't used anywhere in the code that you present in the Original Post

I also got rid of the Try because there isn't a need for a try if you aren't going to catch anything or finally anything. This also removes a level of unnecessary indentation.

The code now looks like this:

private void DoReceiveFrom(IAsyncResult iar)
{
    EndPoint clientEP = new IPEndPoint(IPAddress.Any, 0);
    int dataLen = 0;
    byte[] data = null;
    try
    {
        dataLen = this.serverSocket.EndReceiveFrom(iar, ref clientEP);
        data = new byte[dataLen];
        Array.Copy(this.byteData, data, dataLen);
    }
    finally
    {
        EndPoint newClientEP = new IPEndPoint(IPAddress.Any, 0);
        this.serverSocket.BeginReceiveFrom(this.byteData, 0, this.byteData.Length, SocketFlags.None, ref newClientEP, DoReceiveFrom, newClientEP);
    }

    if (!this.clientList.Any(client => client.Equals(clientEP)))
        this.clientList.Add(clientEP);
}

and while I am at it, I would implement the IDisposable interface and use a Dispose method instead of this

public void Stop()
{
    this.serverSocket.Close();
    this.serverSocket = null;
    this.clientList.Clear();
}

it would then look something like this

public void Dispose()
{
     this.serverSocket.Close();
     this.serverSocket = null;
     this.clientList.Clear();
}

this also gives you other options when using the class in your code.

\$\endgroup\$

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.