Бахтин Pantera - 3 задания (шифровка, расшифровка, брутфорс)#19
Бахтин Pantera - 3 задания (шифровка, расшифровка, брутфорс)#19BakhtinD wants to merge 12 commits intodemologin:mainfrom
Conversation
ADD DecoderCommand ADD EncoderCommand ADD ExceptionCatcher ADD InvalidInputException ADD MenuCommand ADD MenuController ADD WrongCommandException
ADD CaesarParamReader ADD EncoderCommand ADD FileSystem FIX ALL
FIX CaesarParamReader.java FIX Console.java FIX DecoderCommand.java FIX EncoderCommand.java DEL Constants.java
FIX Caesar.java FIX CaesarParamReader.java FIX Console.java FIX FileSystem.java FIX MenuController.java
FIX ALL
demologin
left a comment
demologin
left a comment
There was a problem hiding this comment.
Написано консольное приложение с тремя методами и из четырёх возможных, есть некоторые замечания, но в целом все выглядит неплохо, поставил оценку б
|
|
||
| public class BruteForce implements MenuCommand { | ||
|
|
||
| private static final String COMMAND_NAME = "Брутфорс"; |
There was a problem hiding this comment.
Константы невозможно изменить поэтому они почти всегда статические и почти всегда публичные, во-первых тогда не нужен getter А во-вторых тогда все строковые константы можно разместить в удобные классы например в интерфейсы, так будет проще сделать предложение многоязычным в случае необходимости
| } | ||
| } | ||
|
|
||
| public void execute() throws IOException { |
There was a problem hiding this comment.
чем важнее метод тем выше он находится в теле класса, поэтому обычно сначала идут публичные а в конце идут приватные
| shiftOfMaxNumOfMatches = shift; | ||
| } | ||
| } | ||
| System.out.println("Правильный ключ: " + shiftOfMaxNumOfMatches); |
There was a problem hiding this comment.
плохо что вывод находится непосредственно внутри вычислительной функции, обычно эти вещи делают отдельными, это позволяет легко заменить реализацию выводы без изменения логики предложения, и наоборот
| Path encodedPath = caesarParamReader.getEncodedFromUser(); | ||
| Path decodedPath = caesarParamReader.getDecodedFromUser(); | ||
|
|
||
| for (int shift = -50; shift <= 50; shift++) { |
There was a problem hiding this comment.
почему 50 а не 55 например, когда в коде появляются такие константы их называют магическими из-за того, что невозможно определить почему они именно такие и на что повлияет их изменение
|
|
||
| public class CaesarParamReader { | ||
|
|
||
| private static final String KEY_INPUT_MESSAGE = "Введите ключ шифрования, по умолчанию - 1:"; |
There was a problem hiding this comment.
вот тут почти как надо. Но надо было сделать все константы публичными и выделить для них отдельный класс
|
|
||
| public class Application { | ||
| public static void main(String[] args) { | ||
| new Console().run(); |
There was a problem hiding this comment.
в этой точке разумнее было бы собрать все приложениесо всеми его компонентами, тогда не пришлось бы их рождать внутри методов, и его структура была бы более прозрачной
| import java.io.IOException; | ||
| import java.nio.file.Path; | ||
|
|
||
| public class Caesar { |
There was a problem hiding this comment.
У этого класса нет никакого изменяемого внутреннего состояния, поэтому достаточно родить его один раз и просто обращаться к методам этого экземпляра
|
|
||
| public class Console { | ||
|
|
||
| private final MenuController menuController = new MenuController(); |
There was a problem hiding this comment.
вот то о чем я говорил, все компоненты лучше создать в одной общей точке а потом на основе сконструировать приложение, передать эти компоненты в него можно через конструктор например
| import java.nio.file.Files; | ||
| import java.nio.file.Path; | ||
|
|
||
| public class FileUtil { |
There was a problem hiding this comment.
очевидно это вспомогательный класс, утилитный, в таких всегда делают приватный конструктор
| <version>3.6.1</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>junit</groupId> |
There was a problem hiding this comment.
не очень понял зачем нужна эта зависимость, Тестов вроде бы нет
3 participants
No description provided.