Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have written the following code, I am quite new to C# and Sockets, so perhaps I have made some big mistakes that only more experienced eyes can detect. This is to be used in a Windows Mobile 6.1 device, so I am trying to avoid using many threads.

using System;

using System.Collections.Generic;
using System.Text;
using System.Net.Sockets;
using System.Net;
using System.Threading;
using System.Diagnostics;

namespace SmartDevice_Server
{
    //ClientConnection saves connection information is used to keep context in Async and Event calls
    public class ClientConnection : EventArgs
    {
        public NetworkStream NetworkStream { get; private set; }
        public byte[] Data { get; private set; }
        public int byteReadCount { get; set; }

        public ClientConnection(NetworkStream networkStream, byte[] data)
        {
            NetworkStream = networkStream;
            Data = data;
        }
    }

    //MySocket - Is a server that listens for events and triggers Events upon Request Completion 
    public class MySocketTCP
    {
        #region Class Members
        TcpListener myTcpListener;
        TcpClient myTcpClient;
        NetworkStream myNetworkStream;

        const string localHost = "127.0.0.1";
        IPAddress myAddress = IPAddress.Parse(localHost);
        int myPortNumber = 58889;
        byte[] myData;

        int bytesReadCount;
        const int MIN_REQUEST_STRING_SIZE = 10;

        int TimeStart;

        //Event
        public event socketReadCompleteHandler socketReadCompleteEvent;
        public EventArgs eventArguments = null;
        public delegate void socketReadCompleteHandler(MySocketTCP myTcpSocket, ClientConnection eventArguments);

        #endregion

        //Constructor
        public MySocketTCP()
        {
            Init();
        }

        //Constructor overloaded to receive IPAdress Host, and Port number
        public MySocketTCP(IPAddress hostAddress, int portNumber)
        {
            myAddress = hostAddress;
            myPortNumber = portNumber;

            Init();
        }

        //Initializes the TCPListner
        public void Init()
        {
            try
            {
                myTcpListener = new TcpListener(myAddress, myPortNumber);

                //myNetworkStream = myTcpClient.GetStream();
            }
            catch (Exception ex)
            {
                throw ex;
            }
        }

        /*TODO_Listener_Timer: After you accept a connection you wait for data to be Read indefinitely
         *Possible solution: Use a timeout to close the socket connection.
         *Check WIKI, TODOS
         * */
        //Listens Asynchronously to Clients, class a recieveMessageHandler to process the read
        public void ListenAsync()
        {
            myTcpListener.Start();

            while (true)
            {
                //blocks until a client has connected to the server
                myTcpClient = myTcpListener.AcceptTcpClient();

                var client = new ClientConnection(myTcpClient.GetStream(), new byte[myTcpClient.ReceiveBufferSize]);

                // Capture the specific client and pass it to the receive handler
                client.NetworkStream.BeginRead(client.Data, 0, client.Data.Length, r => receiveMessageHandler(r, client), null);
            }
        }

        //Callback is used to Process the request Asynchronously, triggers socketReadCompleteEvent
        public void receiveMessageHandler(IAsyncResult asyncResult, ClientConnection clientInstance)
        {
            bytesReadCount = 0;

            lock (clientInstance.NetworkStream)
            {
                try
                {
                    bytesReadCount = clientInstance.NetworkStream.EndRead(asyncResult);
                    clientInstance.byteReadCount = bytesReadCount;
                }
                catch (Exception exc)
                {
                    throw exc;
                }
            }

            if (bytesReadCount < MIN_REQUEST_STRING_SIZE)
            {
                //Could not read form client.
                Debug.WriteLine("NO DATA READ");
            }
            else
            {
                if (socketReadCompleteEvent != null)
                {
                    socketReadCompleteEvent(this, clientInstance);
                }
            }
        }

        //Reads the request, uses the ClientConnection for context
        public string ReadAsync(ClientConnection connObj)
        {
            int bytesReadCount = connObj.byteReadCount;
            byte[] myData = connObj.Data;

            string xmlMessage;

            try
            {
                xmlMessage = Encoding.ASCII.GetString(myData, 0, bytesReadCount);
            }
            catch (Exception ex)
            {
                throw ex;
            }

            return xmlMessage;
        }

        //Deprecated
        public string Read()
        {
            string xmlMessage;

            try
            {
                xmlMessage = Encoding.ASCII.GetString(myData, 0, bytesReadCount);
            }
            catch (Exception ex)
            {
                throw ex;
            }

            return xmlMessage;
        }

        //Deprecated
        public void Write(byte[] outBytes)
        {
            try
            {
                myNetworkStream.Write(outBytes, 0, outBytes.Length);
            }
            catch (Exception ex)
            {
                throw ex;
            }
        }

        //Deprecated
        public void Write(string outMessage)
        {
            byte[] outBytes = Encoding.ASCII.GetBytes(outMessage);

            try
            {
                myNetworkStream.Write(outBytes, 0, outBytes.Length);
            }
            catch (Exception ex)
            {
                throw ex;
            }

            int TimeEnd = Environment.TickCount;
            int TimeResult = TimeEnd - TimeStart;
        }

        //Is used to send the message to the correct socket
        public void WriteAsync(ClientConnection connObj, string outMessage)
        {
            byte[] outBytes = Encoding.ASCII.GetBytes(outMessage);

            try
            {
                connObj.NetworkStream.Write(outBytes, 0, outBytes.Length);
            }
            catch (Exception ex)
            {
                throw ex;
            }

            int TimeEnd = Environment.TickCount;
            int TimeResult = TimeEnd - TimeStart;
        }

        //Closes the client
        public void Close()
        {
            //myNetworkStream.Close();
            try
            {
                myTcpClient.Close();
            }
            catch (Exception ex)
            {

                throw ex;
            }
        }
    }
}
share|improve this question

1 Answer 1

up vote 7 down vote accepted

There are a number of naming issues here so it's quite hard to understand the code, I'll start from those:

  • public properties/events/methods should be named PascalCase (receiveMessageHandler => ReceiveMessageHandler)
  • events are named according to the time they occur. If event describes that some fact occurred, it should be named in passive voice without "Event" suffix (socketReadCompleteEvent => SocketReadCompleted)
  • private fields should be name camelCase (TimeStart => timeStart), I prefer to have underscore prefix
  • classes derived from EventArgs should have EventArgs suffix by convention
  • Async suffix is used to define methods that run asynchronously and return either Task or Task<TResult> objects. In your case ListenAsync just enters infinite loop.

Now let's move to logical issues:

  • if you catch exceptions you should do something rather than just re-throwing them.
  • re-throwing exceptions should be done with throw;, not throw ex;. You loose the stack trace if you do the latter.
  • locking should be done on private fields that are not accessible from outside (see remarks section)
  • You shouldn't expose internal streams to clients (as you do via ClientConnection.NetworkStream)
  • Why do you expose an event that returns raw byte[] and have a separate method that converts this data to string? If you plan to receive strings only just convert array to bytes straight away, otherwise leave the task to others.

Update Since you are creating a server, I would switch from event-based data handling to the structure that mimics IIS request handling via handlers: you can define an IHandler interface that allows customer to handle requests, and register a handler factory that can generate handler instance when you get a new client. That way you will call handler's method with payload and get response as a method result. Then you can serialise result and push it to the stream.

Update 2

Example of how I would tackle this task. First of all we have a number of classes describing our interaction in a typed way:

[XmlRoot("request_authenticate")]
public class AuthenticateRequest
{
    [XmlElement("userid")]
    public int UserId { get; set; }

    [XmlElement("password")]
    public string Password { get; set; }
}

[XmlRoot("response_authenticate")]
public class AuthenticateResponse
{
    [XmlElement("status")]
    public AuthenticateStatus Status { get; set; }
}

public enum AuthenticateStatus
{
    [XmlEnum("OK")]
    Ok,
    [XmlEnum("FAILED")]
    Failed
}

Then we need some base classes to handle these requests:

public interface IHandler
{
    Type RequestType { get; }
    Type ResponseType { get; }

    object Handle(object request);
}

public abstract class BaseHandler<TRequest, TResponse> : IHandler
{
    public Type RequestType { get { return typeof(TRequest); } }

    public Type ResponseType { get { return typeof(TResponse); } }

    public abstract TResponse Handle(TRequest request);

    object IHandler.Handle(object request)
    {
        if (!(request is TRequest))
            throw new ArgumentException("request is expected to be of type ", "request");

        return Handle((TRequest)request);
    }
}

public interface IHandlerFactory
{
    IHandler GetHandler(string elementName);
}

public class HandlerFactory : IHandlerFactory
{
    private readonly Dictionary<string, Func<IHandler>> _mapping = new Dictionary<string, Func<IHandler>>();

    public void RegisterHandler(string requestRootName, Func<IHandler> creator)
    {
        if (creator == null)
            throw new ArgumentNullException("creator", "Factory method should be provided");

        _mapping.Add(requestRootName, creator);
    }

    public IHandler GetHandler(string elementName)
    {
        Func<IHandler> creator;
        return _mapping.TryGetValue(elementName, out creator) ? creator() : null;
    }
}

We can define handlers as following:

public class AuthenticationHandler : BaseHandler<AuthenticateRequest, AuthenticateResponse>
{
    public override AuthenticateResponse Handle(AuthenticateRequest request)
    {
        var authenticateStatus = (request.UserId == 1 && request.Password == "123456")
            ? AuthenticateStatus.Ok
            : AuthenticateStatus.Failed;

        return new AuthenticateResponse { Status = authenticateStatus };
    }
}

And finally the "server":

public class MySocketTcp : IDisposable
{
    private const string LocalHost = "127.0.0.1";

    private readonly IHandlerFactory _handlerFactory;

    private readonly IPAddress _myAddress;
    private readonly int _myPortNumber;
    private readonly TcpListener _myTcpListener;

    private bool _stopping;
    private readonly ManualResetEvent _stopHandle = new ManualResetEvent(false);

    private Thread _mainThread;

    public MySocketTcp(IHandlerFactory handlerFactory) : this(handlerFactory, IPAddress.Parse(LocalHost), 58889) { }

    public MySocketTcp(IHandlerFactory handlerFactory, IPAddress hostAddress, int portNumber)
    {
        _handlerFactory = handlerFactory;
        _myAddress = hostAddress;
        _myPortNumber = portNumber;
        _myTcpListener = new TcpListener(_myAddress, _myPortNumber);
    }

    public void Start()
    {
        if (_stopping)
            return;

        _mainThread = new Thread(Listen);
        _mainThread.Start();
    }

    private void Listen()
    {
        _myTcpListener.Start();

        while (!_stopping)
        {
            var asyncResult = _myTcpListener.BeginAcceptTcpClient(HandleIncomingTcpClient, null);
            //blocks until a client has connected to the server or stopping has been signalled
            WaitHandle.WaitAny(new[] { _stopHandle, asyncResult.AsyncWaitHandle });
        }

        _myTcpListener.Stop();
    }

    private void HandleIncomingTcpClient(IAsyncResult ar)
    {
        using (TcpClient tcpClient = _myTcpListener.EndAcceptTcpClient(ar))
        using (NetworkStream networkStream = tcpClient.GetStream())
        {
            try
            {
                XElement xml = XElement.Load(networkStream);
                ProcessXmlRequest(networkStream, xml);
            }
            catch (XmlException ex)
            {
                //TODO:log deserialization exception, notify client about error in request etc
            }
            catch (Exception ex)
            {
                //TODO:log exception, notify client about error in request processing
            }
        }
    }

    private void ProcessXmlRequest(NetworkStream networkStream, XElement xml)
    {
        var handler = _handlerFactory.GetHandler(xml.Name.LocalName);
        if (handler == null)
        {
            //TODO: notify customer about missing handler, sort of 404 error
            return;
        }

        try
        {
            XmlSerializer serializer = new XmlSerializer(handler.RequestType);
            object requestObject = serializer.Deserialize(xml.CreateReader());
            object responseObject = handler.Handle(requestObject);
            serializer = new XmlSerializer(handler.ResponseType);
            serializer.Serialize(networkStream, responseObject);
        }
        catch (InvalidOperationException ex)
        {
            //TODO: add analysis of the error
        }
    }

    public void Close()
    {
        _stopping = true;
        _stopHandle.Set();
        _mainThread.Join();
    }

    public void Dispose()
    {
        //TODO: dispose all IDisposable properties
    }
}

Usage would look like:

void Main()
{
    HandlerFactory factory = new HandlerFactory();
    factory.RegisterHandler("request_authenticate", () => new AuthenticationHandler());
    //TODO: register all other handlers here

    MySocketTcp server = new MySocketTcp(factory);
    server.Start();
}

I haven't tested this code and can't guarantee it will work on WinMobile 6.5. The main purpose of it is to demonstrate how serialization/deserialization is abstracted from request handling, and how to decouple different request processing.

share|improve this answer
    
Thanks for the comments. I need to do this to call XML functions, afaik... I receive bytes, transform them into XML, process it and then Transform the reply XML into Bytes and send it. Is there an alternative to this? How should I make sure that the connection that created the response is the one I send the response back? If I don't expose NetworkStream? –  Adam Surfari Dec 21 '12 at 19:40
    
@AdamSurfari See updated answer –  almaz Dec 21 '12 at 21:07
    
can you provide me some source code that implements what you are explaining? I have 2 weeks experience with C#, and what I am implementing is indeed a server, but a local server to be run on a Windows Mobile 6.1 Pro device with a 624MHz processor and 102MB of Program memory... I will look for more info on Handlers, but if you can provide me with some links or source code that would be a huge help. Thanks again for updating the anwser! –  Adam Surfari Dec 22 '12 at 17:16
    
Ok, will try to sketch the implementation on Monday. In order to make it close to your needs, please describe: what do you need the server on mobile device for? Do you plan to have multiple "pages" on your server, or the logic is the same on all incoming requests? Can you provide samples of incoming/outgoing XML? –  almaz Dec 22 '12 at 17:26
    
Oh wow, that is much appreciated! The XML is along the lines of: ClientSend: <request_authenticate> <userid>23<userid> <password>7327372</password> </request_autenticate> Server:Parses, and calls DLL to verify user and pass are valid. Server sends: <response_authenticate> <status>OK</status> </response_authenticate> Server receives the request, usually under 150KB and send responses usually under 300KB Other request may have many fields, like product information, etc... but they are all along these lines. Client sends request, server parses and responds. –  Adam Surfari Dec 22 '12 at 17:34

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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