Skip to content

TEST3 MyChat#9

Open
khusainovilas wants to merge 2 commits intomainfrom
test3-mychat
Open

TEST3 MyChat#9
khusainovilas wants to merge 2 commits intomainfrom
test3-mychat

Conversation

@khusainovilas
Copy link
Owner

No description provided.

Assert.That(serverReceived, Is.EqualTo(clientMessage));
Assert.That(clientReceived, Is.EqualTo(clientMessage));
});
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Удивительно, но клиент и сервер в тесте даже не участвуют, тестируются TcpListener и TcpClient из стандартной библиотеки

/// <summary>
/// Represents parsed command-line arguments for the chat application.
/// </summary>
public class Arguments

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Думаю, что можно было сделать эту штуку record-ом или хотя бы сделать основной конструктор

{
if (args.Length == 1)
{
return new Arguments(int.Parse(args[0]), null);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Стоило бы заодно проверить, принадлежит ли номер порта разрешённому диапазону, чтобы выдать пользователю понятную диагностику.

/// </summary>
/// <param name="arguments">Arguments containing port and optional IP address.</param>
/// <returns>A <see cref="Task"/> representing the async operation.</returns>
public static async Task RunAsync(Arguments arguments)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Очеь желательно всё, что Async, делать отменяемым (принимать CancellationToken опциональным параметром, чтобы кому надо — могли операцию отменить). Тут и в методах ниже.

/// <returns>Representing the asynchronous server operation.</returns>
public static async Task RunServerAsync(int port)
{
var listener = new TcpListener(IPAddress.Any, port);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TcpListener IDisposable, поэтому лучше его объявлять с using. Иначе из-за какого-нибудь исключения listener.Stop может не исполниться и порт останется занят до конца работы программы.

var client = await listener.AcceptTcpClientAsync();
lock (ConnectedClients)
{
ConnectedClients.Add(client);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

По условию клиент всего один, так что это зря :)


Console.WriteLine($"Client connected: {endpoint}");

_ = Task.Run(async () =>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А так Вы теряете ссылку на запущенную задачу, следовательно не можете её ни прервать, ни дождаться её завершения

if (ConnectedClients.Count == 0)
{
Console.WriteLine("No clients connected. Server shutting down.");
Environment.Exit(0);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Это вообще не стоит использовать — вдруг сервер захотят переиспользовать в большем приложении

{
while (true)
{
var line = Console.ReadLine();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Это тоже надо было бы сделать отменяемым. Иначе клиент уже отключился, а мы сидим тут и ждём, пока пользователь что-то введёт, хотя уже давно должны были закрыть программу. У Вас эта проблема решается с помощью Environment.Exit, но это всё равно что выключатель света в доме сломался, мы дом снесли к чертям.

}
});

await Task.WhenAny(consoleTask, networkTask);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

По-хорошему надо корректно остановить и дождаться завершения обеих задач

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants