Skip to content

4. Simple FTP - #4

Open
ArtemNikit1n wants to merge 13 commits into
mainfrom
04-simple-ftp
Open

4. Simple FTP#4
ArtemNikit1n wants to merge 13 commits into
mainfrom
04-simple-ftp

Conversation

@ArtemNikit1n

Copy link
Copy Markdown
Owner

No description provided.

/// <exception cref="SocketException">Thrown when network connection fails.</exception>
/// <exception cref="ObjectDisposedException">Thrown when client is disposed.</exception>
/// <returns>A <see cref="Task"/> representing the asynchronous operation.</returns>
public async Task ConnectAsync()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Неподключённый клиент вроде как не нужен, так что по идее после конструктора он уже должен быть подключен и готов к работе (иначе после конструктора объект в неконсистентном состоянии — вроде как жив, но ни одной операции содержательно не исполнить, и можно забыть вызвать ConnectAsync). Однако конструктор не может быть асинхронным. Проблему можно решить статическим асинхронным методом, который вызывает конструктор и сразу ConnectAsync. Сам конструктор тогда можно сделать private, чтобы никто его случайно не вызвал.

public async Task ConnectAsync()
{
this.client = new TcpClient();
await this.client.ConnectAsync(host, port);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Асинхронные операции лучше сразу делать все прерываемыми. Тем более потенциально длительные. Принимайте CancellationToken в любой async-метод и передавайте его в любой библиотечный async-метод, который его принимает.

await this.writer.WriteLineAsync($"2 {path}");
await this.writer.FlushAsync();

var sizeLine = await this.reader.ReadLineAsync();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

По условию все разделители внутри сообщения — пробел, но ок, это можно не править

}
catch (Exception ex)
{
return new GetResult { Success = false, ErrorMessage = $"Error downloading file: {ex.Message}" };

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Вообще, поскольку это по идее класс, относящийся к бизнес-логике системы, он не должен терять информацию об ошибке. Вместо Success = false я бы выкидывал исключения (за исключением тех случаев, когда никакой ошибки не произошло, а ошибка вызвана просто чем-то ожидаемым, типа на сервере файл удалили). А то представьте, "Error downloading file: Null Reference Exception" и без информации о стеке вызовов. И тогда удачной отладки :)

/// <summary>
/// Represents the result of a file download operation.
/// </summary>
public class GetResult

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Сделали бы его record-ом

/// Gets a value indicating whether indicates whether this entry is a directory (true) or file (false).
/// </summary>
public bool IsDirectory { get; init; }
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

А то прямо как в Java. Надо было ещё тесты на DTOшки написать, они же public-классы, что уж.

[Test]
public async Task ProcessRequest_Should_HandleMultipleClientsSimultaneously_When_DownloadingFiles()
{
_ = this.server.RunAsync();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Сделали бы это тоже в SetUp, в каждом тесте есть

/// </summary>
/// <param name="host">Server hostname or IP address (default: localhost).</param>
/// <param name="port">Server port number (default: 8080).</param>
public class Client(string host = "localhost", int port = 8080) : IDisposable

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Клиенту тоже было бы нелишне консольное приложение, раз у сервера есть.

socket.Close();
});
this.currentTasks.Add(task);
this.currentTasks.RemoveAll(t => t.IsCompleted);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍

while ((bytesRead = await fileStream.ReadAsync(buffer)) > 0)
{
await stream.WriteAsync(buffer.AsMemory(0, bytesRead));
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

У стримов есть CopyToAsync, который примерно это и делает

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