5
\$\begingroup\$

I'm trying to get started with network and parallel programming in C#. I'm almost a complete novice in programming, so I'm looking for feedback on a small script I'm writing.

The goal of the programming is to:

  1. Accept incoming connections and store them as users which are sent into Rooms. Users are expected to provide occasional input, maybe once every second.
  2. After a certain time, the input from all users in a room is collected and then sent back to all users in that room.

In total there are 3 classes:

  1. Server
  2. Room
  3. User

The below code is far from complete and, and I haven't decided on startup and close down mechanisms yet. As of now, I just want the connections and asynchronous to work.

class Server {
    private bool ServerIsRunning;
    private const int port = 12321;
    private TcpListener _listener;
    private List<Room> _rooms;
    private const int speed = 100;

    public Server() 
    {
        _rooms = new List<Room>();
        startServer();
    }

    // Start the server
    private void startServer() 
    {
        try {
            _listener = new TcpListener(IPAddress.Any, port);
            _listener.Start();
            ServerIsRunning = true;
            listenForClients();
        }
        catch (SocketException e) {
            Console.Write(e.Message);
        }

    }

    // Listen for clients while server is on, if found, send them to a room
    async private void listenForClients()
    {
        while (ServerIsRunning) {
            TcpClient client = await _listener.AcceptTcpClientAsync();
            new User(client, getARoom());
        }
    }

    // Get a room thats not full(less then 10 users)
    private Room getARoom() 
    {
        int index = _rooms.Count - 1;
        if ((index < 0) || (_rooms[index].NumberOfUsers > 9)){
            _rooms.Add(new Room(speed));
            index++;
        }
        return _rooms[index];
    }
}

class Room
{
    private Task _sending;
    private static Timer timer;
    private List<User> _users;
    private List<Task> _readers;
    public Task Sending { get { return _sending; } }
    public int NumberOfUsers { get { return _users.Count; } }

    public Room(int time) 
    {
        this._users = new List<User>();
        this._readers = new List<Task>();
        this._sending = new Task();
        setupTimer(time);
    }


    public void start()
    {
        timer.Start();
    }

    public void stop() 
    {
        timer.Stop();
    }

    // set up timer
    private void setupTimer(int time)
    {
        timer = new Timer(time);
        timer.Elapsed += new ElapsedEventHandler(sendNewOutput);
    }

    // Add user to room and start if full
    public void registerUser(User newUser) 
    {
        _users.Add(newUser);
        _readers.Add(newUser.Reading);
        if (_users.Count > 0) { start(); }
    }

    // remove user from room
    public void deRegisterUser(User user) 
    {
        _users.Remove(user);
        _readers.Remove(user.Reading);
    }

    // waits for all users to finish recieving and then send the total input back to all
    async private void sendNewOutput(object s, ElapsedEventArgs a) 
    {
        await Task.WhenAll(_readers); // Waits for all clients to read
        try {
            _sending = Task.Run(() => {
                sendOutput(interpretInput());
            });
        }
        catch (Exception e) { Console.WriteLine(e.Message); }

    }

    // gathers all input
    private List<byte[]> interpretInput() 
    {
        List<byte[]> input = new List<byte[]>();
        _users.ForEach((user) => { input.AddRange(user.Input); })
        return input;
    }

    // Sends out output
    private void sendOutput(List<byte[]> output) 
    {
        _users.ForEach((user) => { user.send(output); });
    }
}

class User {
    private TcpClient _connection;
    private NetworkStream _stream;
    private Room _room;
    private Task _reading;
    private List<byte[]> _input;

    public Task Reading { get { return _reading; } } // So that others can see if the task is complete
    public List<byte[]> Input { get { return _input; } } // making the input available

    public User(TcpClient connection, Room room) 
    {
        this._connection = connection;
        this._stream = connection.GetStream();
        this._room = room;
        this._reading = new Task();
        this._room.registerUser(this);
        listen();
    }

    // Listen for data
    private void listen() 
    {
        Task.Run(async () => {
            while (_connection.Connected) {
                if (_stream.CanRead && _stream.DataAvailable) {
                    _reading = read();
                    await _reading; // finish reading before starting again
                }
                Thread.Sleep(50); // i dont want to read all the time
            }
            cleanUp();
        });
    }

    // wait for the room to finish sending out data and then read the new thats waiting
    async private Task read() 
    {
        await _room.Sending; // Waits for the server to finish sending out info to all clients before reading in new
        int length;
        byte[] input = new byte[1024];
        while ((length = await _stream.ReadAsync(input, 0, 1024)) != 0) {
            byte[] data = new byte[length];
            Array.Copy(input, data, length);
            _input.Add(data);
        }
    }

    // sends output to client
    public void send(List<byte[]> output) 
    {
        output.ForEach((message) => {
            _stream.Write(message, 0, message.Length);
        });

    }

    // dergisters the user from the room and closes stream and connection
    private void cleanUp() 
    {
        _room.deRegisterUser(this);
        try {
            _stream.Close();
            _connection.Close();
        }
        catch (Exception e) {
            Console.WriteLine(e.Message);
        }
    }
}
\$\endgroup\$

closed as off-topic by svick, palacsint, Simon Forsberg, rolfl, Jamal Jan 26 '14 at 18:28

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Your question must contain working code for us to review it here. For questions regarding specific problems encountered while coding, try Stack Overflow. After getting your code to work, you may edit this question seeking a review of your working code." – svick, palacsint, Simon Forsberg, rolfl, Jamal
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ “I just want the connections and asynchronous to work.” Does that mean it doesn't work now? If that's the case, your question is not appropriate here, Core Review is for reviewing working code only. \$\endgroup\$ – svick Jan 26 '14 at 10:25
  • \$\begingroup\$ Apologies, I didnt know. \$\endgroup\$ – Tobias Jan 26 '14 at 14:55
4
\$\begingroup\$

A few things I noticed:

  1. Traditionally methods in C# follow CamelCase naming convention (as you can see everwhere in the .NET framework).

  2. You probably want to pass the port and speed as parameters to the server rather than hard coding them.

  3. speed - what is that? You have to actually go down and read the code and see that it's passed to a timer and you have to know that the timer expects the timeout in milliseconds in order to know what it means. Something like roomUpdateIntervalMs would be more descriptive and take the guesswork out of what unit the value is. Also consider making it a TimeSpan instead (in which case you can drop the Ms suffix).

  4. Use the power of Linq. This:

    List<byte[]> input = new List<byte[]>();
    _users.ForEach((user) => { input.AddRange(user.Input); })
    return input;
    

    can be rewritten as

    return _users.SelectMany(u => u.Input).ToList();
    
  5. This:

    byte[] input = new byte[1024];
     while ((length = await _stream.ReadAsync(input, 0, 1024)) != 0) {
    

    should be

    byte[] input = new byte[1024];
    while ((length = await _stream.ReadAsync(input, 0, input.Length)) != 0) {
    

    otherwise you have to change a constant in two places if you want to make the buffer larger.

  6. The comment here is misleading:

    // Add user to room and start if full
    public void registerUser(User newUser)
    

    It does start it if at least one user is in there (not empty rather than full). In fact it will call start() every time you add a user as the condition checks for count > 0.

  7. This new User(client, getARoom()); reads a bit strange. In a garbage collected language on first glance this reads like "nothing holds the reference so it will be garbage collected immediately". Only after digging into the code one will see that it's internally added to the room collection. I would suggest to remove the registration form the User constructor and make it an external call instead.

    var room = getARoom();
    var user = new User(client, room);
    room.registerUser(user);
    

    This makes it more explicit what if happening.

  8. AcceptTcpClientAsync can accept a client any time. In which case you can add a new user while a send operation is in progress which happens to iterate over the user list. You are bound to get a InvalidOperationException stating Collection was modified; enumeration operation may not execute.

  9. Last but not least probably the biggest issue:

    I admit that I'm not having a great deal of experience with the async framework however: Tasks are not synchronization primitives. They are an abstraction aimed to make asynchronous and parallel programming a bit easier and less painful.

    You are using them to synchronize read and send operations and I'm pretty sure this will be a source of pain. A ReaderWriterLock owned by the room would seem a better tool (n readers, 1 writer, don't write while reading, don't read while writing). Don't expose the lock though - this should be encapsulated. Each user should call something like _room.ObtainReadPermission() or so which internally enters the lock.

\$\endgroup\$
  • \$\begingroup\$ Thank You! Lots of good tips here and ill take a look at all of them. The last one in particular, way better solution. \$\endgroup\$ – Tobias Jan 26 '14 at 11:48
  • \$\begingroup\$ Ad. 4: I think you need SelectMany there, Select would be the equivalent of Add, not AddRange. \$\endgroup\$ – svick Jan 26 '14 at 15:11

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