Савицких Антон by Xineev · Pull Request #210 · kontur-courses/fp

Xineev · 2025-12-29T16:54:10Z

SquirrelLeonid

Правки, не связанные на прямую с переходом на Result
Точно можно покрыть больше сценариев с помощью Result
Для закрытия второго пункта задачи нужно внимательно посмотреть используемые методы в твоих проектах и покрыть возможные исключения Result

SquirrelLeonid · 2025-12-30T08:55:52Z

TagCloudConsoleClient/ConsoleClient.cs

+using TagCloudGenerator.Infrastructure.Readers;
+using static System.Runtime.InteropServices.JavaScript.JSType;


Лишние using. В других местах тоже проверь

Проверил, постарался убрать все лишние

SquirrelLeonid · 2025-12-30T08:58:03Z

TagCloudConsoleClient/ConsoleClient.cs

+            string inputFile = opts.InputFile;
+            string outputFile = opts.OutputFile;


string => var

Исправил

SquirrelLeonid · 2025-12-30T09:01:03Z

TagCloudConsoleClient/ConsoleClient.cs

+                .Then(reader => reader.TryRead(inputFile))
+                .Then(words => _normalizer.Normalize(words))
+                .Then(words => _generator.Generate(words, canvasSettings, textSettings, _filters))
+                .Then(image => Result.OfAction(() => image.Save(outputFile, ImageFormat.Png), "Failed to save output image"))


Аргументы для метода OfAction стоит разбить по строкам (лямбда и сообщение в случае ошибки). Так будет легче читать.

Поменял

SquirrelLeonid · 2025-12-30T09:03:53Z

TagCloudGenerator/Infrastructure/Readers/ReaderRepository.cs

+                return Result.Fail<IFormatReader>("File path is empty");
+            var reader = _readers.FirstOrDefault(r => r.CanRead(filePath));


Для читаемости добавить отступ между return и инструкцией вне if'а. Так ты дополнительно разграничишь небольшие кусочки логики. Можно мыслить это как запятую или точку в предложении :)

Исправил

SquirrelLeonid · 2025-12-30T09:05:32Z

TagCloudGenerator/Infrastructure/Readers/ReaderRepository.cs

+                return Result.Fail<IFormatReader>(
+                    $"No reader found for file '{filePath}'");


А здесь разбивка ни к чему. Все и так влезает в рекомендуемую ширину строки.

Хотя возможно это актуально для моего монитора (120 символов)

Исправил

SquirrelLeonid · 2025-12-30T10:18:41Z

TagCloudGenerator/Infrastructure/Sorterers/FrequencyDescendingSorterer.cs

@@ -0,0 +1,16 @@
+using TagCloudGenerator.Core.Interfaces;
+
+namespace TagCloudGenerator.Infrastructure.Sorterers


Sorterers => Sorters

Исправил

SquirrelLeonid · 2025-12-30T10:20:07Z

TagCloudGenerator/Infrastructure/Result.cs

+        }
+    }
+
+    public struct Result<T>


У меня есть несколько моментов, которые я бы переделал в этой реализации Result. Но т.к. она взята из примера к задаче, то пусть будет как есть.

Напиши отдельно если интересно, что на мой взгляд здесь не так

Был бы очень рад, по личным ощущениям эту тему хуже остальных освоил, попробовать написать другую реализацию думаю будет полезно

SquirrelLeonid · 2025-12-30T10:29:33Z

TagCloudGeneratorTests/CloudGeneratorTests.cs

+                    It.IsAny<IEnumerable<CloudItem>>(),
+                    It.IsAny<CanvasSettings>(),
+                    It.IsAny<TextSettings>()))
+                .Returns(new Bitmap(1, 1));


На этот bitmap нужно вызывать dispose

SquirrelLeonid · 2025-12-30T10:35:42Z

TagCloudConsoleClient/ConsoleClient.cs

+                .Then(reader => reader.TryRead(inputFile))
+                .Then(words => _normalizer.Normalize(words))
+                .Then(words => _generator.Generate(words, canvasSettings, textSettings, _filters))
+                .Then(image => Result.OfAction(() => image.Save(outputFile, ImageFormat.Png), "Failed to save output image"))


image нужно задиспоузить после сохранения

SquirrelLeonid · 2025-12-30T10:37:13Z

TagCloudGenerator/Algorithms/BasicTagCloudAlgorithm.cs

+
+namespace TagCloudGenerator.Algorithms
+{
+    public class BasicTagCloudAlgorithm : ITagCloudAlgorithm


Здесь тоже переход на Result нужен

Обновил

SquirrelLeonid

Поправить еще пару моментов с using и немного перетащить логику из FrequencyDescendingSorter. В остальном ок, следующие правки, вероятно, будут завершающими

SquirrelLeonid · 2026-01-08T15:15:51Z

TagCloudGenerator/Algorithms/BasicTagCloudAlgorithm.cs

+            return Result.Ok(PlaceNextRectange(rectangleSize));
+        }
+
+        private Rectangle PlaceNextRectange(Size rectangleSize)


Опечатка

Исправил

SquirrelLeonid · 2026-01-08T15:29:46Z

TagCloudGenerator/Core/Services/CloudGenerator.cs

+
+            var initializedItems = InitializeCloudItems(sortedWords.Value, textSettings).ToList();
+
+            return _renderer.Render(initializedItems, canvasSettings, textSettings);


Предлагаю вынести шаг по рендеру из CloudGenerator. На то две причины:

Класс должен генерировать облако, но вот его отрисовка - уже ответственность другого модуля. Грубо говоря - одну и ту же раскладку мы должны уметь скармливать разным рендерам.

На то, что упоминание BItmap в этом классе излишне меня натолкнули строчки выше, где возвращается Result.Fail. Если провалиться в класс Result, то увидим, что в этом случае полю Value присвоится значение по умолчанию для Generic-типа. В данном случае это будет Null. Проблема в том, что это не говорит внешнему коду о том, что вызывать Dispose на Bitmap в этом случае не нужно. а попытка его вызова приведет к NullReferenceException.

Если сказать коротко - утащив отсюда упоминание Bitmap мы разгрузим класс. Ему не нужно будет брать ответственность за отрисовку картинки - только за генерацию раскладки.

Вынес шаг с рендером в классы клиентов

SquirrelLeonid · 2026-01-08T15:31:57Z

TagCloudGenerator/Core/Services/CloudGenerator.cs

+
+            foreach (var word in words)
+            {
+                foreach (var filter in filters)


Здесь все еще возможно множественное перечисление по filters. Вообще на всей цепочке фильтров уместно передавать / получать ICollection или лист / массив. Это следует из вполне разумных предположений:
а) Количество фильтров конечно.
б) Это количество почти наверняка будет невелико

Обновил

SquirrelLeonid · 2026-01-08T15:34:28Z

TagCloudGenerator/Core/Services/CloudGenerator.cs

+            if (!wordsWithFreq.IsSuccess || wordsWithFreq.Value == null)
+                return Result.Fail<Bitmap>("Analyzed words collection is null");
+
+            var sortedWords = _sorter.Sort(wordsWithFreq.Value);
+            if (!sortedWords.IsSuccess || sortedWords.Value == null)
+                return Result.Fail<Bitmap>("Sorted words collection is null");


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

Обновил

SquirrelLeonid · 2026-01-08T15:35:43Z

TagCloudGenerator/Infrastructure/Analyzers/WordsFrequencyAnalyzer.cs

+            if (words == null)
+                return Result.Fail<Dictionary<string, int>>("Missing words list to analyze");


И еще один момент, в котором отсутствие слов не должно являться исключительным состоянием. Вероятно ты имел ввиду, что коллекция не должна быть null, но на мой взгляд в данном случае Null и Count = 0 это одно и то же

Заменил на Result.Ok(null)

У тебя там Result.Ok не от Null, а с конкретным объектом. Вообще, так даже лучше, только MaxFreq и MinFreq я бы указал по нулям.

Сначала также хотел, но дело в том что minFreq и maxFreq дальше используются для вычисления frequencyRatio и там их разность берется как делитель, если оставить их нулями то получим деление на 0 поэтому решил изначально оставлять такими значениями, в любом случае это ситуация при которой мы ничего не рендерим поэтому полученные там коэффициенты при текущих частотах ни на что не влияют

Пересмотрел свой код, понял что это не должно ничего сломать

Указал по нулям

SquirrelLeonid · 2026-01-08T16:25:01Z

TagCloudGenerator/Infrastructure/Renderers/PngRenderer.cs

+                return Result.Fail<Bitmap>("Invalid canvas size");
+
+            var bitmap = new Bitmap(canvasSettings.CanvasSize.Width, canvasSettings.CanvasSize.Height);
+            using (Graphics graphics = Graphics.FromImage(bitmap))


Graphics => var. В других подобных местах аналогично

Обновил

SquirrelLeonid · 2026-01-08T16:26:17Z

TagCloudGenerator/Infrastructure/Renderers/PngRenderer.cs

+            if (items == null)
+                return Result.Fail<Bitmap>("Cloud items are null");


Здесь ошибочное состояние уже более уместно. Но все равно можно подумать, не заменить ли ошибку рисованием пустого холста

Заменил на создание пустого листа

SquirrelLeonid · 2026-01-08T16:39:43Z

TagCloudGenerator/Infrastructure/Renderers/PngRenderer.cs

+                ConfigureGraphics(graphics);
+                graphics.Clear(canvasSettings.BackgroundColor);
+                var (offsetX, offsetY) = CalculateOffset(itemsList, canvasSettings);
+                using (SolidBrush brush = new SolidBrush(textSettings.TextColor))
+                {
+                    using (StringFormat stringFormat = new StringFormat())
+                    {
+                        stringFormat.Alignment = StringAlignment.Center;
+                        stringFormat.LineAlignment = StringAlignment.Center;
+
+                        using (Pen pen = new Pen(textSettings.TextColor, 1))
+                        {
+                            pen.DashStyle = System.Drawing.Drawing2D.DashStyle.Dash;
+
+                            return Result.Of(() =>
+                            {
+                                foreach (var item in itemsList)
+                                    DrawCloudItem(graphics, item, offsetX, offsetY, canvasSettings, textSettings, brush, pen, stringFormat);
+                                return bitmap;
+                            });
+                        }
+                    }


Здесь аналогичная проблема.

graphics, brush, pen, stringFormat используются в лямбде, но уничтожаются вне ее. Лучше тоже отступить на полшага назад и завернуть все внутрь одного ResultOf. Просто разбей это на два шага.

var result = ResultOf...

return result;

Вроде как исправил

SquirrelLeonid · 2026-01-08T16:41:10Z

TagCloudGenerator/Infrastructure/Sorters/FrequencyDescendingSorter.cs

+            var minFreq = int.MaxValue;
+            var maxFreq = int.MinValue;


Этому классу точно не нужно заниматься подсчетом максимальной / минимальной частоты. Эту логику нужно унести в WordsFrequencyAnalyzer.

Вынес логику в WordsFrequencyAnalyzer

SquirrelLeonid · 2026-01-08T16:43:29Z

TagCloudGeneratorTests/CloudGeneratorTests.cs

+                    It.IsAny<IEnumerable<CloudItem>>(),
+                    It.IsAny<CanvasSettings>(),
+                    It.IsAny<TextSettings>()))
+                .Returns(new Bitmap(1, 1));


Все еще актуально. Вынеси Bitmap(1,1) чуть выше и заверни его в using

Это из тестов ушло

SquirrelLeonid · 2026-01-11T16:27:04Z

Оставшиеся замечания учтены, задачу засчитываю на полный балл

Xineev added 3 commits December 29, 2025 19:36

Import solution from previous task

eed1d8a

Implemented Result<T> pattern

bda3418

Updated tests with new exceptions handling pattern

0a21c47

SquirrelLeonid reviewed Dec 30, 2025

View reviewed changes

Xineev added 3 commits January 3, 2026 21:44

Implemented Result<T> pattern for more classes and added fixes

52f7aed

Made critical adjustments for classes

57591ca

Updated tag cloud tests

a1fdfe0

SquirrelLeonid reviewed Jan 8, 2026

View reviewed changes

Xineev added 7 commits January 10, 2026 23:16

Updated CloudGenerator and Infrastructure

0ba2794

Moved renderer to console client

db85b57

Moved renderer to UI client

a28b2b6

Updated tests

ca24a76

Updated generator, docxReader, renderer and analyzer

264f1a5

Updated generator and analyzer tests

8a4bf9b

Updated DocxReader

239700a

		using TagCloudGenerator.Infrastructure.Readers;
		using static System.Runtime.InteropServices.JavaScript.JSType;

		string inputFile = opts.InputFile;
		string outputFile = opts.OutputFile;

		return Result.Fail<IFormatReader>("File path is empty");
		var reader = _readers.FirstOrDefault(r => r.CanRead(filePath));

		return Result.Fail<IFormatReader>(
		$"No reader found for file '{filePath}'");

		@@ -0,0 +1,16 @@
		using TagCloudGenerator.Core.Interfaces;

		namespace TagCloudGenerator.Infrastructure.Sorterers


		var initializedItems = InitializeCloudItems(sortedWords.Value, textSettings).ToList();

		return _renderer.Render(initializedItems, canvasSettings, textSettings);

		if (words == null)
		return Result.Fail<Dictionary<string, int>>("Missing words list to analyze");

		if (items == null)
		return Result.Fail<Bitmap>("Cloud items are null");

Conversation

Xineev commented Dec 29, 2025

Uh oh!

SquirrelLeonid left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SquirrelLeonid left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment