Skip to content

Conversation

@MikePuzanov
Copy link
Owner

No description provided.

Copy link
Collaborator

@yurii-litvinov yurii-litvinov left a comment

Choose a reason for hiding this comment

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

Тестов нет, а по условию нужны были

Microsoft Visual Studio Solution File, Format Version 12.00
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Test1.1", "Test1.1\Test1.1.csproj", "{E451040C-44C5-427F-9CF7-F83F90E54DFF}"
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Test1.1.Tests", "Test1.1.Tests\Test1.1.Tests.csproj", "{E628CC17-DD0A-4658-8561-CB4C496A798D}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Даже CI не одобряет отсутствия тестов

/// </summary>
public class Client
{
private TcpClient _client;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Если хоть одно поле у класса IDisposable, то и сам класс должен быть IDisposable, и у себя в Dispose вызывать Dispose всех своих IDisposable-полей. Иначе есть риск, что Dispose у них не вызовется до самого конца программы (что сродни утечке памяти).


public Client(string host, int port)
{
_client = new TcpClient(host, port);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Это сразу инициирует подключение, и если хост не ответит, приведёт к ожиданию таймаута, который в случае с TCP может быть довольно большим. И всё это время вызывающий будет заблокирован. Надо было использовать ConnectAsync, и не в конструкторе, а при старте.

await Task.Run(async () =>
{
var writer = new StreamWriter(stream) { AutoFlush = true };
while (true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Итого у Вас три (!) вложенных бесконечных цикла, тогда как работает (и нужен) только один

Task.Run(async () =>
{
while (true) {
await Writer(_client.GetStream());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Не-а, так Reader будет заблокирован. Надо было Task.WhenAny

}
else
{
var client = new Client(args[0], Int32.Parse(args[1]));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Никто не обещал, что будет хоть один аргумент, кстати

public class Server
{
private int _port;
private readonly CancellationTokenSource _cancellationToken = new CancellationTokenSource();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
private readonly CancellationTokenSource _cancellationToken = new CancellationTokenSource();
private readonly CancellationTokenSource _cancellationToken = new ();

Вы ж не на Java пишете :)

}
}
});
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Что-то копипаст, такое в клиенте уже было

else
{
var client = new Client(args[0], Int32.Parse(args[1]));
client.Working();
Copy link
Collaborator

Choose a reason for hiding this comment

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

await server.Working(); но блокирующий client.Working();, несимметрично и неправильно (клиент тоже может быть асинхронным, конечно)

Console.WriteLine(data + "\n");
if (data == "exit")
{
StopServer();
Copy link
Collaborator

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.

3 participants