Skip to content

Conversation

@slampy97
Copy link

Очень приятная архитектура, чтобы расширять, требовалось только написать свою команду и добавить ей в списке comandbuilder. Интерфейс команды тоже понятный, аналогичный моему, да и впринципе при вынесении всех команд в свою директорию максимально логично. Хоть у меня было написано на питоне, а здесь все на java, тоже не составило проблем что-то писать. Единтсвенное у меня не запустились тесты, но я думаю это из-за того, что я на windows это делал. Свои тесты запускались и проходили.

Copy link
Collaborator

@ottergottaott ottergottaott left a comment

Choose a reason for hiding this comment

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

Пока 5 баллов

@Override
public String run(List<String> args, String before) {
if (args.isEmpty()) {
System.setProperty("user.dir", System.getProperty("user.home"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Очень зря так меняете текущую директорию, как минимум сложно написать тесты на такой cd

Comment on lines 156 to 165
void testCdCommands() {
Command cd = new Cd();
ArrayList<String> lst = new ArrayList<>();
String res = System.getProperty("user.dir");
cd.run(lst, "");
Assertions.assertEquals(System.getProperty("user.home"), System.getProperty("user.dir"));
lst.add("main");
cd.run(lst, "");
Assertions.assertEquals(System.getProperty("user.dir"), res + "\\" + "main");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ну вот, собственно, если бы не поленились и добавили еще хотя бы один тест, то быстро бы обнаружили проблему

Copy link
Collaborator

Choose a reason for hiding this comment

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

Пишите больше тестов

}
public static Command getCommand(String commandName) {
if (commandName.equals("pwd")) {
return new Pwd();
Copy link
Collaborator

Choose a reason for hiding this comment

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

А что с форматированием стало? Почему 4 пробела вдруг на 2 заменились?

Copy link
Author

@slampy97 slampy97 Apr 11, 2021

Choose a reason for hiding this comment

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

в ксц на java другие требования, забываю в editor менять

File curDir = new File(".");
for (File file : Objects.requireNonNull(curDir.listFiles())) {
String name = file.getName();
if (!name.startsWith(".")) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

А почему тут только в одном месте пропускаете скрытые файлы, а ниже этого не делаете?

Вообще код здесь обобщается, мы же все равно в любом случае должны пройти директорию, даже если это просто ..

@slampy97 slampy97 requested a review from ottergottaott April 11, 2021 00:38
Copy link
Collaborator

@ottergottaott ottergottaott left a comment

Choose a reason for hiding this comment

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

8 баллов

Command ls = new Ls();
ArrayList<String> lst = new ArrayList<>();
String expected = "build build.gradle.kts gradle gradlew gradlew.bat README.md settings.gradle src testFiles";
String expected = ".git .github .gradle .idea build build.gradle.kts gradle gradlew gradlew.bat README.md settings.gradle src testFiles";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Вообще такие тесты плохие, т.к. содержимое корневой директории очевидно еще будет меняться

} else {
File file = new File(args.get(0));
System.setProperty("user.dir", file.getAbsolutePath());
var path = Paths.get(System.getProperty("user.dir"), args.get(1));
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.

2 participants