Cli application - #1
Conversation
257b79b to
ba9a5ab
Compare
ba9a5ab to
65c717b
Compare
| @@ -0,0 +1,68 @@ | |||
| package ru.ifmo.cli_application; | |||
|
|
|||
| import javafx.util.Pair; | |||
There was a problem hiding this comment.
javafx ? Зачем? Тем более, что класс для такой пары уже написан --- ParserRule
| Scanner scanner = new Scanner(inputStream); | ||
|
|
||
| while (!ctx.isClosed()) { | ||
| List<IToken> tokens = parser.parseTokens(scanner.nextLine()); |
There was a problem hiding this comment.
Парсеру становится плохо на строчках вида echo '1" | 2'. Возможно, стоит еще подумать над регулярным выражением. И хорошо бы к нему написать разные сложные тесты
| import java.util.Map; | ||
|
|
||
| public class Context { | ||
| private Map<String, String> variables; |
There was a problem hiding this comment.
Зачем отдельно писать конструктор, если мапу можно и здесь инициализировать?
| } | ||
| List<IToken> tokensToExecute = new ArrayList<>(); | ||
| IExecutable executableCommand = null; | ||
| for (IToken token : tokens) { |
There was a problem hiding this comment.
Предлагаю ввести еще одну абстракцию <команда c параметрами>
| List<String> splitedCommand = splittingFunction.apply(command); | ||
| List<IToken> outputTokens = new ArrayList<>(splitedCommand.size()); | ||
| for (String commandToken : splitedCommand) { | ||
| if (commandToken.isEmpty() || commandToken.equals("\n")) { |
There was a problem hiding this comment.
Мы же вроде по строчке считаем, может ли это условие когда-либо выполниться?
| StringBuilder resultString = new StringBuilder(); | ||
| for (IToken fileName : args) { | ||
| try { | ||
| Scanner scanner = new Scanner(new FileInputStream(fileName.toString())); |
There was a problem hiding this comment.
Files.lines(Paths.get(fileName.toString())) ?
| StringBuilder resultString = new StringBuilder(); | ||
| for (IToken fileName : args) { | ||
| try { | ||
| Scanner scanner = new Scanner(new FileInputStream(fileName.toString())); |
| public class PwdCommand implements IToken, IExecutable { | ||
| @Override | ||
| public String execute(List<IToken> args, String inputStream) { | ||
| return System.getProperty("user.dir"); |
There was a problem hiding this comment.
Мы можем перемещаться по папкам, а pwd все user.dir будет выдавать
There was a problem hiding this comment.
А, это уже hw4, данный комментарий учитывать не нужно
| package ru.ifmo.cli_application.tokens; | ||
|
|
||
| public interface IToken { | ||
| String toString(); |
There was a problem hiding this comment.
Мб по-другому как-нибудь назвать? Так-то любой объект имеет метод toString()
| public void start() throws IOException { | ||
| Scanner scanner = new Scanner(inputStream); | ||
|
|
||
| while (!ctx.isClosed()) { |
There was a problem hiding this comment.
Лучше добавить обработку вообще всех ошибок
ff5a9dd to
9ad1e4d
Compare
yurii-litvinov
left a comment
yurii-litvinov
left a comment
There was a problem hiding this comment.
В целом всё ок, но хочется ещё немного доработок
| import java.util.List; | ||
| import java.util.Scanner; | ||
|
|
||
| public class Application { |
There was a problem hiding this comment.
Хочется больше комментариев. Хотя бы к каждому классу и паблик-методу (кроме геттеров-сеттеров и прочей мелочи)
| } | ||
|
|
||
| public void start() throws IOException { | ||
| Scanner scanner = new Scanner(inputStream); |
There was a problem hiding this comment.
Это же современная Java, тут лучше писать var в случаях, если тип переменной очевиден из правой части присваивания, дабы не писать дважды одно и то же
| import java.util.regex.Pattern; | ||
|
|
||
| public class SimpleParser extends Parser { | ||
| private static Pattern splittingPattern = Pattern.compile("[^\\s\"']+|\"[^\"]*\"|'[^']*'"); |
There was a problem hiding this comment.
Так оно не может раскрыть "12$x" --- думает, что это строка, тогда как $x должна интерпретироваться как подстановка
|
|
||
| import java.util.List; | ||
|
|
||
| public interface IExecutable { |
There was a problem hiding this comment.
Особенно интерфейсы следует документировать
| } | ||
|
|
||
| dependencies { | ||
| testCompile group: 'junit', name: 'junit', version: '4.12' |
There was a problem hiding this comment.
Я бы рекомендовал на JUnit 5 переходить, он более идеологически правильный. Но это можно не править
| import java.util.HashMap; | ||
| import java.util.Map; | ||
|
|
||
| public class Context { |
There was a problem hiding this comment.
Я бы не сказал, что с комментариями стало прям сильно лучше
|
|
||
| public class Variable implements IToken { | ||
| String variable; | ||
| Context ctx; |
There was a problem hiding this comment.
Поля с пакетной видимостью не нужны, дыра в инкапсуляции же.
3 participants
No description provided.