Timoshenko. 4 tasks complete. Rady CryptoAnaliz!#18
Timoshenko. 4 tasks complete. Rady CryptoAnaliz!#18FunnyDaniik wants to merge 2 commits intodemologin:mainfrom
Conversation
FunnyDaniik
changed the title
demologin
left a comment
demologin
left a comment
There was a problem hiding this comment.
Над дизайном приложения определённо стоит ещё немного поработать, однако в целом все что мы изучали здесь применяется а также реализованы четыре варианта из четырёх необходимых, поэтому поставил оценку а
|
|
||
| if (isRussian) { | ||
| expectedFrequencies = new double[]{ | ||
| 0.0801, 0.0159, 0.0454, 0.0170, 0.0298, 0.0845, 0.0094, 0.0165, 0.0735, |
There was a problem hiding this comment.
Данный массив выглядит как магия, посторонний человек никогда не догадается сразу что означает каждое из этих цифр, если эти частоты относятся к определённым буквам то лучше бы было сделать карту, тогда все было бы понятно, а размер этой карты как раз указал бы количество букв
| letterCount = 32; | ||
| } else { | ||
| expectedFrequencies = new double[]{ | ||
| 0.0812, 0.0149, 0.0271, 0.0432, 0.1202, 0.0230, 0.0203, 0.0592, 0.0731, |
| return calculateFrequencySimilarity(text, sampleText); | ||
| } | ||
|
|
||
| private double calculateLetterFrequency(String text) { |
There was a problem hiding this comment.
Дизайн этого метода неудачный, практически всегда если число строк в методе превышает один скролл, а это примерно 25 строк, его разбивают на подметоды каждому из которых дают осмысленное названия
| }; | ||
| letterCount = 32; | ||
| } else { | ||
| expectedFrequencies = new double[]{ |
There was a problem hiding this comment.
тут ещё одна странность, при каждом обращении к методу оба этих массива будут всякий раз рождаться заново, и всякий раз с одними и теми же значениями, намного лучше было бы вынести их в статическое поле и там проинициализировать. Это важно для сборки мусора
| import java.util.Map; | ||
|
|
||
| public class CaesarCipher { | ||
| public static final char[] RUSSIAN_ALPHABET = { |
There was a problem hiding this comment.
по смыслу это конечно константа, однако технически это массив. А значение массива всегда можно изменить. Поэтому в общем случае лучше не обозначать массивы константными именами
| try { | ||
| Files.write(Paths.get(filePath), content.getBytes()); | ||
| } catch (IOException e) { | ||
| throw new CipherException("Ошибка записи в файл: " + filePath, e); |
| private double calculateFrequencyScore(int[] freq1, int[] freq2) { | ||
| double score = 0; | ||
| for (int i = 0; i < freq1.length; i++) { | ||
| if (freq2[i] > 0) { |
There was a problem hiding this comment.
в именах переменных лучше сокращений не использовать никогда
| return frequency; | ||
| } | ||
|
|
||
| private double calculateFrequencyScore(int[] freq1, int[] freq2) { |
There was a problem hiding this comment.
суффиксы один и два не говорят мне о назначении этих массивов абсолютно ничего, expectedFrequency и actualFrequency было бы намного информативнее
| import com.javarush.timoshenko.exception.CipherException; | ||
| import java.util.Scanner; | ||
|
|
||
| public class MainApp { |
There was a problem hiding this comment.
данный класс выполняет сразу две функции с одной стороны он собирает все компоненты приложения вместе, а с другой стороны отвечает пользовательский ввод, реализацию вызова расчётной функциональности, и вывод результатов. Очевидно что его нужно разбить как минимум на несколько классов, по одному на каждое направление ответственности
| } | ||
| } | ||
|
|
||
| public static void main(String[] args) { |
There was a problem hiding this comment.
самые важные методы идут самыми первыми, а второстепенные опускаются в конец класса, так проще понимать логику его работы
2 participants
No description provided.