Сибогатов Ринат #177

sibogatovr · 2024-02-07T03:17:06Z

Описание проекта:

Проект представляет собой программу для генерации облака тегов из файла с набором слов. Программа осуществляет удаление скучных слов, перечень которых указан в отдельном файле, а также приведение всех слов к нижнему регистру для унификации.

Внесенные изменения:

Добавлена функциональность удаления скучных слов.
Реализовано приведение всех слов к нижнему регистру.
Реализовано изменение цвета ТОП слов
Реализована возможность изменить шрифт, цвет, размер
Добавлен CLI
Реализованы ридеры: *.doc, *.docx, *.txt
Добавлены соответствующие тесты для новой функциональности.
Внесены мелкие улучшения и исправления.

@pakapik

pakapik · 2024-02-07T05:55:23Z

TagsCloudContainer/TagsCloud/CircularCloudLayouter.cs

+
+                var currentRectangleResult = CreateNewRectangle(rectangleSize);
+
+                if (currentRectangleResult.IsSuccess)


nit: можно попробовать сделать более "функционально":

currentRectangleResult .OnSuccess(() => /*...*/) .OnFail(() => /*...*/);

pakapik · 2024-02-07T05:57:32Z

TagsCloudContainer/TagsCloud/CircularCloudLayouter.cs

+
+        private void ValidateRectangleSize(Size rectangleSize)
+        {
+            if (rectangleSize.Width < MinPositiveValue || rectangleSize.Height < MinPositiveValue)


Кажется, что саму проверку можно зашить в экстеншн, передавая туда MinPositiveValue для сравнения.

pakapik · 2024-02-07T06:01:34Z

TagsCloudContainer/TagsCloud/FontSizeCalculator.cs

+        {
+            return Math.Max(BaseFontSize, BaseFontSize + frequency * FontSizeMultiplier);
+        }
+        return DefaultFontSize;


Пустую строчку упустил)

pakapik · 2024-02-07T06:13:17Z

TagsCloudContainer/TagsCloud/FontSizeCalculator.cs

+    {
+        var wordFrequencies = new Dictionary<string, int>();
+
+        foreach (var word in words)


Можно применить LINQ - сгруппировать и посчитать

pakapik · 2024-02-07T06:14:57Z

TagsCloudContainer/TagsCloud/ImageSettings.cs

+
+        public Font GetFont()
+        {
+            return new Font("Verdana", 20);


Зачем каждый раз возвращать новый объект шрифта?

Font реализует IDisposable - выше по колстеку это не учитывается

pakapik · 2024-02-07T07:00:50Z

TagsCloudContainerTests/FileReaderTests.cs

+            {
+                foreach (var word in words)
+                {
+                    Console.WriteLine(word);


Зачем выводить слова на консоль?

pakapik · 2024-02-07T07:02:36Z

TagsCloudContainerTests/FileReaderTests.cs

+
+        private IFileReader GetFileReader(string filePath)
+        {
+            if (Path.GetExtension(filePath).Equals(".docx", StringComparison.OrdinalIgnoreCase))


Кажется, что чуть лучше было бы создать словарь

pakapik · 2024-02-07T07:04:44Z

TagsCloudContainerTests/FileReaderTests.cs

+        [Test]
+        public void WithSpecificContent_ReturnsExpectedWords()
+        {
+            var filePath = "src/boring_words.txt";


nit: подобный хардкод делает тесты весьма хрупкими. Мало ли как всё изменится в процессе рефакторинга? И когда-нибудь попадешь в ситуацию "хотел сделать лучше, а упала половина тестов")

pakapik · 2024-02-07T07:05:51Z

TagsCloudContainerTests/SpiralTests.cs

+        public void Constructor_ShouldNotThrow_WithValidArguments()
+        {
+            Action constructorAction = () => new Spiral(center, 0.1, 0.2);
+            constructorAction.Should().NotThrow();


В одном случае у тебя было Assert.DoesNotThrow, тут уже .Should().NotThrow();. Стоит делать единообразно.

pakapik · 2024-02-07T07:12:58Z

TagsCloudContainerTests/TagCloudGeneratorTests.cs

+        private ITagCloudGenerator tagCloudGenerator;
+
+        [SetUp]
+        public void SetUp()


SetUp стоит использовать тогда, когда в нем есть необходимость. Необходимость в нем может появиться тогда, когда каждому тесту нужен новый объект sut, причем все объекты идентичны по своим параметрам. Если хоть одно правило не выполняется - SetUp может начать приносить больше боли, чем пользы.
Прочитай, пожалуйста, пункты с 3.2 до 3.4 (ссылку пришлю в телеге)

Кстати, ты уже использовал токен sut. Почему бы и тут это не сделать?)

pakapik · 2024-02-08T11:22:58Z

TagsCloudContainer/TagsCloud/TagCloudApp.cs

+        private const double DefaultRadiusStep = 0.04;
+        private const int HalfWidthHeight = 2;
+
+        public string FontName { get; set; }


Теперь непонятно, зачем тебе _fontName и FontName одновременно)

pakapik · 2024-02-08T11:31:00Z

TagsCloudContainer/TagsCloud/ImageSettings.cs

+            Height = height;
+        }
+
+        ~ImageSettings()


Почему именно деструктор, а не реализация типом интерфейса IDisposable?

pakapik · 2024-02-08T11:32:58Z

TagsCloudContainer/Readers/TxtReader.cs

+            {
+                var lines = File.ReadAllLines(filePath);
+
+                var words = lines.SelectMany(line => line.Split(' '));


nit: Если не ошибаюсь, то Split по умолчанию делит строки по пробелу ' '

pakapik · 2024-02-08T11:33:29Z

TagsCloudContainer/TagsCloud/TagCloudGenerator.cs

+                graphics.Clear(imageSettings.BackgroundColor);
+                Font font = imageSettings.GetFont();
+
+                int OffsetY = 0;


pakapik · 2024-02-08T11:35:47Z

TagsCloudContainer/Utility/Visualizer.cs

+                var word = uniqueWords.ElementAt(i).ToLower(); 
+
+                var fontSize = fontSizes[i];
+                var font = new Font(fontName, fontSize, FontStyle.Regular);


nit: using. Ещё можно подумать над тем, как бы постоянно не выделить память для font и brush

pakapik · 2024-02-08T11:37:08Z

TagsCloudContainerTests/FileReaderTests.cs

+        {
+            readerCreators = new Dictionary<string, Func<IFileReader>>(StringComparer.OrdinalIgnoreCase)
+        {
+            { ".docx", () => new DocxReader() },


nit: можно подумать над тем, как с FileType использовать

pakapik · 2024-02-08T11:37:44Z

TagsCloudContainerTests/WordPreprocessorTests.cs

+
+            foreach (var word in processedWords)
+            {
+                Console.WriteLine(word);


Снова - зачем тут вывод в консоль?

pakapik · 2024-02-08T11:38:50Z

TagsCloudContainer/TagsCloud/TagCloudApp.cs

+                }
+                else
+                {
+                    Console.WriteLine($"Error generating tag cloud image: {tagCloudImageResult.Error}");


Всё ещё остался вывод в консоль. Я не думаю, что тут место для класса Console. Вся прилага автоматически стала привязана к консоли, это не оч хорошо

pakapik · 2024-02-09T10:50:35Z

TagsCloudContainer/TagsCloud/ImageSettings.cs

+{
+    public class ImageSettings : IImageSettings, IDisposable
+    {
+        private readonly Font font;


Зачем для этого поля сделан самописный геттер?

Для чего инициализация поля вынесена в конструктор?

Теперь расскажу, в чем суть придирок.

Ты захламляешь код ненужными деталями. Чтоб этого не делать и были введены автосвойства. Стоит их и использовать.

Если есть возможность что-то инициализировать сразу в полях - так и надо сделать. Компилятор всё равно подставит инициализацию поля внутрь конструктора. Однако у тебя сразу появится полезная привычка, т.к. иногда конструкторами (если их более одного) злоупотребляют, делая копипаст. Так что мини-совет на все случаи жизни:
а. инициализируй поля на месте объявления, если это возможно
б. не делай копипаст в конструкторах, вместо этого стоит использовать конструкцию : this()

pakapik · 2024-02-09T11:05:30Z

TagsCloudContainer/TagsCloud/TagCloudApp.cs

+            }
+        }
+
+        private string GetMostPopularWord(Dictionary<string, int> wordFrequencies)


Думаю, можно обойтись Linq, даже не выделяя метод

pakapik · 2024-02-09T11:10:38Z

TagsCloudContainer/TagsCloud/TagCloudApp.cs

+                    ? Result.Ok(new CircularCloudLayouter(center, spiralResult.Value))
+                    : Result.Fail<CircularCloudLayouter>($"Error creating CircularCloudLayouter: {spiralResult.Error}");
+            }
+            catch (Exception ex)


На самом деле, обычно рекомендуют указывать минимально допустимый тип исключения, а уже далее по нарастающей, вплоть до базового Exception. Хотя на практике это делается редко, только если их надо по-разному обрабатывать. Но если не знал, то вот)

pakapik · 2024-02-09T11:27:43Z

TagsCloudContainer/Utility/Spiral.cs

+        {
+            try
+            {
+                var points = GeneratePointsOnSpiral().ToList();


Исключительно странная строка) У тебя есть потенциально бесконечный набор точек, создаваемый GeneratePointsOnSpiral. После этого ты аллоцируешь коллекцию в памяти вызовом ToList, после чего отдаешь сгенерированный лист как IEnumerable. В чем суть этого всего?)

Мало того, что аллоцируешь всю коллекцию в памяти - ты это делаешь многократно, т.к. конечный размер коллекции, который выдаст GeneratePointsOnSpiral неизвестен, а значит создаваемый List будет расширяться (путем копирования старого листа в новый лист, размер которого в 2 раза больше предыдущего) n раз до тех пор, пока не сможет вместить в себя все точки исходной коллекции.

Ради интереса можешь заняться расчетами, сколько в итоге сожрет памяти многократная аллокация коллекций структур)

pakapik · 2024-02-09T11:29:48Z

TagsCloudContainer/Utility/Visualizer.cs

+            EnsureDirectoryExists(pathToDirectory);
+
+            var safeFileName = GetSafeFileName(fileName);
+            bitmap.Save(Path.Combine(pathToDirectory, safeFileName), ImageFormat.Png);


nit: не стеняйся выделять переменные.
По-моему, запись

var safeFileName = GetSafeFileName(fileName); var savePath = Path.Combine(pathToDirectory, safeFileName); bitmap.Save(savePath, ImageFormat.Png);

намного легче читается

pakapik · 2024-02-09T11:30:35Z

TagsCloudContainer/Utility/Visualizer.cs

+
+        private static void EnsureDirectoryExists(string directory)
+        {
+            if (!Directory.Exists(directory))


Кажется, что если переписать без отрицания, то будет чуть нагляднее

pakapik · 2024-02-09T11:38:44Z

TagsCloudContainerTests/CircularCloudLayouterTests.cs

+
+            secondRectangleResult.OnSuccess(secondRectangle =>
+            {
+                var expectedRectangleCenter = new Point(


nit: В итоге, выделяли мы или нет константу Half... всё равно есть деление на 2, пусть и в другом месте. Повод задуматься о том, чтоб научить точку саму по себе становиться центром прямоугольника / либо вынести эту ответственность ещё куда-то.

pakapik · 2024-02-09T11:40:16Z

TagsCloudContainerTests/CircularCloudLayouterTests.cs

+
+                secondRectangleResult.OnSuccess(secondRectangle =>
+                {
+                    secondRectangle.IntersectsWith(firstRectangle).Should().BeFalse();


К комментарию выше: тут всё весьма прилично, мы не руками реализовываем поиск пересечения, а используем уже написанный метод. Было бы круто с точкой то же самое провернуть.

pakapik · 2024-02-09T11:43:27Z

TagsCloudContainerTests/WordPreprocessorTests.cs

+        [SetUp]
+        public void SetUp()
+        {
+            preprocessor = new WordPreprocessor(new List<string>());


SetUp не несет никакой пользы. Если бы preprocessor как-то конфигурировался - ок, но тут можно сделать поле вычисляемым, и всё будет работать точно так же, т.к. каждый тест будет обращаться к новому объекту препроцессора.

sibogatovr added 3 commits February 3, 2024 01:11

feat: Result<T>

3a0777e

refactor: Refactor tests

ad788f8

feat: Readme.md and minor improvements

b2db053

pakapik reviewed Feb 7, 2024

View reviewed changes

refacator: Addressed all issues in pull request

0c8072a

pakapik reviewed Feb 8, 2024

View reviewed changes

refactor: issues in pull request (2)

75639dd

pakapik reviewed Feb 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Сибогатов Ринат #177

Сибогатов Ринат #177

sibogatovr commented Feb 7, 2024

pakapik Feb 7, 2024 •

edited

Loading

pakapik Feb 7, 2024

pakapik Feb 7, 2024

pakapik Feb 7, 2024

pakapik Feb 7, 2024 •

edited

Loading

pakapik Feb 7, 2024

pakapik Feb 7, 2024

pakapik Feb 7, 2024 •

edited

Loading

pakapik Feb 7, 2024

pakapik Feb 7, 2024

pakapik Feb 8, 2024

pakapik Feb 8, 2024

pakapik Feb 8, 2024

pakapik Feb 8, 2024

pakapik Feb 8, 2024

pakapik Feb 8, 2024

pakapik Feb 8, 2024

pakapik Feb 8, 2024

pakapik Feb 9, 2024

pakapik Feb 9, 2024

pakapik Feb 9, 2024

pakapik Feb 9, 2024 •

edited

Loading

pakapik Feb 9, 2024

pakapik Feb 9, 2024

pakapik Feb 9, 2024

pakapik Feb 9, 2024

pakapik Feb 9, 2024


		var currentRectangleResult = CreateNewRectangle(rectangleSize);

		if (currentRectangleResult.IsSuccess)

Сибогатов Ринат #177

Are you sure you want to change the base?

Сибогатов Ринат #177

Conversation

sibogatovr commented Feb 7, 2024

Описание проекта:

Внесенные изменения:

pakapik Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakapik Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakapik Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakapik Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakapik Feb 7, 2024 •

edited

Loading

pakapik Feb 7, 2024 •

edited

Loading

pakapik Feb 7, 2024 •

edited

Loading

pakapik Feb 9, 2024 •

edited

Loading