Can anyone rate the code for a client-server chat based on tcp/ip?

Question

code review

Server - https://github.com/SuperDru/TcpChatServer.git
Client - https://github.com/SuperDru/TcpChatClient.git
I'm learning c#, it's quite difficult to figure out what exactly I'm doing wrong and don't want to write the following project making the same mistakes without being aware of it, so any constructive comments on the code would be greatly appreciated.

Answer 1

You can talk about client-server applications for a long time. I will dwell on only one aspect: how to make sure that this server does not lie down.
The solutions that you have applied will work well with a load of 1 person (since we cannot press buttons in two windows at the same time), but already 2-3 people can easily put down a server if they start sending messages often.
In this regard, a few wishes:
In the connection waiting loop, we make a call to AcceptTcpClient():
In this case, we can easily get a network exception related to the inability to connect a particular client. And our server will go down. Because of one single unsuccessful connection of the client (maybe his Internet has fallen off). And the server should continue to work. To do this, it makes sense to wrap the call to AcceptTcpClient () in a try-catch, log the exception and continue working.

try
{
    ServerClient client = new ServerClient(server.AcceptTcpClient());
    // ...
}
catch (Exception e) when (server.Active)
{
    // Логируем исключение и продолжаем работу
    // (если сервер не остановлен).
}

The static field Server.clients is used from different threads.
It turns out that if two requests come at the same time, we can get a situation where they can be called simultaneously in different threads:

clients.Add()
clients.ForEach()
clients.TrueForAll()
clients.Remove()

In practice, this will lead, at best, to unnecessary exceptions that the standard library will throw for us. And in the worst case, if you call clients.Add and clients.Remove simultaneously from different threads, the data in this list will be destroyed.
To prevent this from happening, it makes sense to use a lock for ANY access to the clients field.
For example, like this:

public static class Server
{
    public static object State = new object();

    public static void Listen(string ip)
    {
        //...
        // Кстати, цикл имеет смысл прервать, если сервер будет остановлен через Stop()
        while (server.Active)
        {
            //...
            lock(State)
            {
                clients.Add(client);
            }
        }
    }
}

public class ServerClient
{
    private void RemoveUser()
    {
        lock(Server.State)
        {
            Server.clients.Remove(this);
            Server.users = Server.users.Replace(name + "\n", "");
        }
    }
}

In this case, if one thread started doing something inside "lock(State)", the other thread will wait until the first one finishes its work at the entrance to this lock.
If we need to work with several shared resources at once, for example, "clients" and "users", it makes sense to place this work in a single lock(State) block - see the example above.
At a minimum, the "clients" field is still used in Server.BroadcastMessage(), Server.isUniqueName(), ServerClient.AddUser(). There should also be a lock.
In the ServerClient.SendMessage() method when called:
- an exception can easily occur due to a communication defect. In this case, our server will go down. To prevent this from happening, we do the same as in StartReceiving () - we wrap it in a try-catch.
In general, for a training project, it's pretty decent. You have demonstrated the ability to solve complex problems in simple ways with minimal code. This will be very useful in your career.

Answer 2

You must be joking. As part of the question on the toaster, offer to study the code of someone else's project?