Горбатов Александр #163
Горбатов Александр #163
Conversation
TagsCloudConsoleUI/app.config
Outdated
| <?xml version="1.0" encoding="utf-8" ?> | ||
| <configuration> | ||
| <appSettings> | ||
| <add key="BoringWordsFilterPath" value="../../../../TagsCloudCore/Resources/filter.txt"/> |
There was a problem hiding this comment.
Очень странно выносить эту настройку таким образом. Мы можем выбирать вводные слова, но не можем выбирать фильтр, не залезая в код?
Плюс, у меня не нашло этот файл по такому пути.
| if (!stringSizeResult.IsSuccess) | ||
| return Result.Fail<IReadOnlyDictionary<string, WordData>>(stringSizeResult.Error); | ||
|
|
||
| var newWord = new WordData(_cloudLayouter.PutNextRectangle(stringSizeResult.Value), frequency); |
There was a problem hiding this comment.
Здесь уже ничего не поделаешь, так как CloudLayouter упакован, но очень странно было принимать в нем Size из графической библиотеки. Size не для подобных алгоритмов придуман.
| public Result<IReadOnlyDictionary<string, WordData>> DistributedWords => DistributeWords(); | ||
|
|
||
| private Result<IReadOnlyDictionary<string, WordData>> DistributeWords() |
| { | ||
| if (!_words.IsSuccess) | ||
| return Result.Fail<IReadOnlyDictionary<string, WordData>>(_words.Error); |
There was a problem hiding this comment.
Честно говоря, видеть подобное в самом начале метода очень путает.
Во-первых, трудно понять значение _words.IsSuccess
Логика становится очень непрозрачной, ведь мы не знаем, что происходит за этим _words.IsSuccess и даже не имеем ни малейшего понятия, потому что ничего из написанного не дает нам этого понять. Приходится лезть в код глубже, чтобы хоть что-то понять.
Во-вторых, мы спрятали вообще всю логику куда-то. Что мы сделали в этом классе с _words, что там могло появится .IsSuccess?
| ICommonOptionsProvider commonOptionsProvider, | ||
| IDrawingOptionsProvider drawingOptionsProvider) | ||
| { | ||
| _words = processedWord.ProcessedWords; |
There was a problem hiding this comment.
Я понял, что особенно сильно путает. Вызов логики происходит в конструкторе, где метод спрятан за полем. Это очень неочевидно и затрудняет чтение кода.
| public Result<IReadOnlyDictionary<string, int>> ProcessedWords => ProcessWords(); | ||
|
|
||
| private Result<IReadOnlyDictionary<string, int>> ProcessWords() | ||
| { |
There was a problem hiding this comment.
Опять используется этот подход. Пока что он кажется больше путающим, чем полезным. Готов обсудить, если тебе кажется, что так лучше.
| var getWordsResult = provider!.GetWords(_wordProviderInfo.ResourceLocation); | ||
| if (!getWordsResult.IsSuccess) | ||
| return Result.Fail<IReadOnlyDictionary<string, int>>(getWordsResult.Error); |
There was a problem hiding this comment.
Сейчас не замечание, а просто коммент.
Вот здесь понятно, откуда у getWordsResult берется IsSuccess и что он означает. Видно, над чем мы исполнили действия, что передавали в методы, какой результат ожидаем все это понятно из одной строчки.
А если мы переделаем этот пример под то, как ты писал в некоторых местах, будет
var words = provider.Words;
if (!words.IsSuccess)
И здесь вообще не понятно, что произошло, как так произошло, почему слова могут зафейлиться? Слова - это слова, это не логика, они не могут выкинуть ошибку. А тут мы как будто у слов проверяем они успешные или нет.
| _distributedWords = cloudDistributorProvider.DistributedWords; | ||
| _drawingOptions = drawingOptionsProvider.DrawingOptions; |
There was a problem hiding this comment.
Вот посмотрев на этот код, не видишь тут никакой скрытой логики. А потом переходишь в метод DrawImage и первая строчка if (!_distributedWords.IsSuccess), и она шокирует, пугает, будоражит воображение.
Дальше уже даже не страшно увидеть if (!_drawingOptions.IsSuccess), потому что из контекста конструктора кажется, что они объекты, которые ведут себя более-менее одинаково.
| public Result<Bitmap> DrawImage(WordColorerAlgorithm colorerAlgorithm) | ||
| { | ||
| if (!_distributedWords.IsSuccess) | ||
| return Result.Fail<Bitmap>($"Cannot draw the image. {_distributedWords.Error}"); |
There was a problem hiding this comment.
Уже написал несколько комментов по этому поводу.
Но ещё немного дополню.
Заходим в метод и первым же действием проверяем что-то из приватных полей класса. А значит, мы делали какую-то логику в конструкторе над приватными полями класса. И это немного путает. Потому что мы ещё ничего не сделали с этим классом, а уже могут быть ошибки? Получается, все сломалось ещё на моменте инициализации класса, а мы тут вот кидаем ошибку. И пугает то, что в конструкторе лежит какая-то логика в принципе, помимо инициализации объектов.
@MrTimeChip