Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HW4 #6

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

HW4 #6

wants to merge 9 commits into from

Conversation

konnnst
Copy link
Owner

@konnnst konnnst commented Oct 19, 2023

Simple FTP program realisation

Copy link

@ArtyomKopan ArtyomKopan left a comment

Choose a reason for hiding this comment

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

5 баллов. Нужно больше тестов, в особенности таких, где читаются не текстовые, а бинарные файлы

HW04FTP/CLIENT/Network.cs Outdated Show resolved Hide resolved
/// <returns>Response string</returns>
public string GetServerResponse(string query)
{
using (var client = new TcpClient(_address, _port))

Choose a reason for hiding this comment

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

так мы получим синхронное подключение. Нужно использовать конструктор TcpClient() без параметров и затем использовать метод ConnectToAsync. И лучше создавать client в конструкторе, а не здесь. Так получается, что для обработки каждого запроса от сервера мы создаём новый клиент, а это накладные расходы

/// </summary>
/// <param name="query">Raw command string</param>
/// <returns>Response string</returns>
public string GetServerResponse(string query)

Choose a reason for hiding this comment

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

этот метод (как и все остальные методы клиента и сервера) должен быть асинхронным

HW04FTP/CLIENT/UI.cs Outdated Show resolved Hide resolved
HW04FTP/CLIENT/Network.cs Outdated Show resolved Hide resolved
HW04FTP/SERVER/Executor.cs Outdated Show resolved Hide resolved
HW04FTP/SERVER/Server.cs Outdated Show resolved Hide resolved

Choose a reason for hiding this comment

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

хотелось бы увидеть больше тестов (чтение текстовых и бинарных файлов, и обработка некорректных запросов)

HW04FTP/SERVER.Tests/ExecutorTests.cs Outdated Show resolved Hide resolved
HW04FTP/CLIENT/Network.cs Show resolved Hide resolved
@konnnst konnnst requested a review from ArtyomKopan December 27, 2023 17:11
Copy link

@ArtyomKopan ArtyomKopan left a comment

Choose a reason for hiding this comment

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

В принципе всё хорошо, 9/10. Только проверь while (true) в файле Server/Network.cs, а то у меня сомнения насчёт него. Возможно, лучше тоже заменить его на CancellationToken. И если пишешь несколько assert'ов в одном тесте, то лучше объединить их с помощью Assert.Multiple

Choose a reason for hiding this comment

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

а зачем пустой класс?

Comment on lines +37 to +43
public ClientNetwork(string address)
{
_port = 1337;
_address = address;
_client = GetTcpClient();

}

Choose a reason for hiding this comment

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

Suggested change
public ClientNetwork(string address)
{
_port = 1337;
_address = address;
_client = GetTcpClient();
}
public ClientNetwork(string address)
{
_port = 1337;
_address = address;
_client = GetTcpClient();
}

лишняя пустая строка

Comment on lines +52 to +58
public ClientNetwork(int port, string address)
{
_port = port;
_address = address;
_client = GetTcpClient();

}

Choose a reason for hiding this comment

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

Suggested change
public ClientNetwork(int port, string address)
{
_port = port;
_address = address;
_client = GetTcpClient();
}
public ClientNetwork(int port, string address)
{
_port = port;
_address = address;
_client = GetTcpClient();
}

var givenSize = Convert.ToInt32(correctResponse.Split(' ')[0]);
var realSize = correctResponse.Split(' ').Count<string>() - 1;


Choose a reason for hiding this comment

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

опять лишняя пустая строка

public class CommandsTests
{
[TestMethod]
public async Task IsGetTest()

Choose a reason for hiding this comment

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

не очень удачное название для теста

var listener = new TcpListener(IPAddress.Any, _port);
listener.Start();

while (true)

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