Холстинин Егор #208

Yrwlcm · 2023-12-03T09:30:37Z

desolver · 2023-12-04T17:52:59Z

cs/TagsCloudVisualization/CircularCloudLayouter.cs

+        public CircularCloudLayouter(Point center)
+        {
+            CenterPoint = center;
+            spiralGenerator = new SpiralGenerator(center);
+        }
+
+        public CircularCloudLayouter(Point center, int radiusDelta, double angleDelta)
+        {
+            CenterPoint = center;
+            spiralGenerator = new SpiralGenerator(center, radiusDelta, angleDelta);
+        }


Давай посмотрим на это с точки зрения связности кода:

В обоих конструкторах мы жестко завязались на генерацию точек исключительно по спирали, а что, если мы захотим генерировать точки каким-нибудь другим алгоритмом? Как много изменений придется внести в код, чтобы подключить другой алгоритм? (Вспоминаем O из SOLID'a) Можно ли это побороть?

Второй конструктор используется только в тесте, но при этом существует в основном коде, дополнительно нагружая его собой. В продакшене лучше несколько раз подумать, чем втащить в и без того сложный код ещё больше сложности, которая не участвует в основной логике. Но возможно его и так не станет, когда ты сделаешь 1 пункт)

desolver · 2023-12-04T17:57:00Z

cs/TagsCloudVisualization/CircularCloudLayouter.cs

+            if (rectangleSize.Width < 0 || rectangleSize.Height < 0)
+                throw new ArgumentException("Rectangle can't have negative width or height");
+
+            // Необходимо чтобы значительно увеличить плотность, очень сильно жертвуем производительностью


Безусловно, при чтении кода в IDE такие комментарии помогают понять ход твоих мыслей и то, чего ты хотел достичь, они полезны

Просто обычно такие комментарии пишутся прямо в PR в комментариях (которые я пишу сейчас, например, тебе). Потому что нужны они только ревьюеру и только в конкретном PR. В коде такое держать точно нельзя

А ещё потому, что тебе в любом случае напишут удалить их из кода)) Что собственно и тут нужно сделать (не беспокойся, я все прочитал :D )

desolver · 2023-12-04T18:01:14Z

cs/TagsCloudVisualization/CircularCloudLayouter.cs

+            {
+                var nextPoint = spiralGenerator.GetNextPoint();
+
+                var locationForRect = new Point(nextPoint.X - rectangleSize.Width / 2,


В случаях, когда в переменную врываются предлоги, союзы и артикли, по возможности стараемся от них избавиться - либо просто выбрасыванием их из низвания, либо используем инверсию слов, чтобы уменьшить длину переменной без потери смысла - locationForRect = rectLocation

Схожие переменные (характеристики прямоугольника) - rectangleSize и locationForRect называются по-разному - в первом случае ты написал слово полностью, а во втором сократил. Читаемость круче маленького объема)

desolver · 2023-12-04T18:08:25Z

cs/TagsCloudVisualization/CircularCloudLayouter.cs

+                var locationForRect = new Point(nextPoint.X - rectangleSize.Width / 2,
+                    nextPoint.Y - rectangleSize.Height / 2);
+
+                var newRect = new Rectangle(locationForRect, rectangleSize);


Аналогичный комментарий про сокращение названия, жертвуя читаемостью

desolver · 2023-12-04T18:08:37Z

cs/TagsCloudVisualization/CircularCloudLayouter.cs

+                    nextPoint.Y - rectangleSize.Height / 2);
+
+                var newRect = new Rectangle(locationForRect, rectangleSize);
+                if (createdRectangles.Any(rectangle => rectangle.IntersectsWith(newRect))) continue;


Как считаешь, насколько оптимально для каждой новой точки заново обсчитывать абсолютно все уже расположенные прямоугольники?
На маленьких данных понятное дело мы это не заметим, а если захотим располагать миллионы фигур?

desolver · 2023-12-04T18:52:40Z

cs/TagsCloudVisualization/SpiralGenerator_Should.cs

+        {
+            var centerPoint = new Point(centerX, centerY);
+            var spiral = new SpiralGenerator(centerPoint, radiusLambda, angleLambda);
+            spiral.GetNextPoint().Should().NotBeNull().And.Be(new Point(centerX, centerY));


Первое условие не нужно, ведь второе условие и так подразумевает, что точка не null

desolver · 2023-12-04T18:54:42Z

cs/TagsCloudVisualization/SpiralGenerator_Should.cs

+                (new Point(9, 6), 0),
+                (new Point(8, 9), Math.PI/4),
+                (new Point(5, 10), Math.PI/2),
+                (new Point(2, 9), Math.PI*3/4),
+                (new Point(1, 6), Math.PI),
+                (new Point(2, 3), Math.PI*5/4),


Форматирование чуть поехало - цифры слиплись

desolver · 2023-12-04T18:57:02Z

cs/TagsCloudVisualization/TagsCloudVisualization.csproj

@@ -0,0 +1,23 @@
+<Project Sdk="Microsoft.NET.Sdk">


Понимаю, что это не входило в рамки задания, но вкину немного продакнш практик. Тесты и основной код обычно делятся на два отдельных проекта - основной код, который раскатается потом на прод, и тесты - которые отработают на этапе тестирования и всё.

Так сборка основного кода не получит лишнюю нагрузку в виде компиляции и рестора зависимостей тестов и будет чуть быстрее собираться, что может значительно повлиять на CI/CD пайплайн

К чему я это - давай и тут сделаем так же)

После разделения проверь пакеты - чтобы ничего лишнего не попало в тот или иной проект

desolver · 2023-12-04T18:58:51Z

cs/TagsCloudVisualization/CircularCloudLayouter_Should.cs

+        [TestCase(-1, 0, TestName = "Negative width")]
+        [TestCase(0, -1, TestName = "Negative height")]
+        [TestCase(-5, -5, TestName = "Negative width and height")]
+        public void PutNextRectangle_ThrowsArgumentException_WhenNegativeParameters(int rectWidth, int rectHeight)


Конвент именования немного потерял в этом тесте

desolver · 2023-12-04T19:00:07Z

cs/TagsCloudVisualization/CircularCloudLayouter.cs

+using System.Threading.Tasks;
+
+namespace TagsCloudVisualization
+{


Хорошая практика - использовать file-scoped неймспейсы, лишяя вложенность ни к чему, без нее код становится проще читать.

Давай пройдемся по всем файлам в проекте и сделаем это (в отдельном коммите)

Yrwlcm · 2023-12-05T19:19:32Z

cs/TagsCloudVisualization/CloudLayouter.cs

+                nextPoint.Y - rectangleSize.Height / 2);
+
+            var newRectangle = new Rectangle(rectangleLocation, rectangleSize);
+            if (createdRectangles.Any(rectangle => rectangle.IntersectsWith(newRectangle))) continue;


Вроде зарефакторил все из запрошенного, кроме этого. Особенно в условиях DI не приходит в голову как это можно оптимизировать, хоть и понимаю, что потенциально убийственная строчка.

Yrwlcm · 2023-12-05T19:22:05Z

cs/TagsCloudVisualizationTests/CloudLayouter_Should.cs

+            var minimalDistanceToClosestRectangle = rectanglesWithoutCurrent
+                .Min(existingRectangle => CalculateDistanceBetweenRectangles(currentRectangle, existingRectangle));
+
+            minimalDistanceToClosestRectangle.Should().BeLessThan(2 * squareSide);


Тут дистанция взята из головы, по схождению звезд и с учетом гороскопа. Может конечно какую-то более оптимальную можно подобрать, но лень

desolver · 2023-12-06T14:30:20Z

cs/TagsCloudVisualization/LayoutDrawer.cs

+            var bitmap = new Bitmap(imageWidth, imageHeight);
+
+            var graphics = Graphics.FromImage(bitmap);
+            graphics.Clear(Color.Wheat);
+
+            var blackPen = new Pen(Color.Black);


Все ли ресурсы освобождены в этих трех объектах?

desolver · 2023-12-06T14:30:48Z

cs/TagsCloudVisualization/LayoutDrawer.cs

+            graphics.DrawRectangles(blackPen, offsettedRectangles);
+            bitmap.Save(filePath + @$"\{fileName}.png", ImageFormat.Png);
+
+            //Console.WriteLine($"Image is saved to {filePath}" + @$"\{fileName}.png");


Я писал в том плане, чтобы прям раскомментить её и оставить в качестве логирования

desolver · 2023-12-06T14:31:55Z

cs/TagsCloudVisualization/SpiralGenerator.cs

+
+public class SpiralGenerator : IPointGenerator
+{
+    private readonly Point center = new(0, 0);


Сейчас пропала возможность задать центр спирали (из условий задачи)

desolver · 2023-12-06T14:33:49Z

cs/TagsCloudVisualization/SpiralGenerator.cs

+    private int radius;
+    private double angle;
+    private const int RadiusDelta = 1;
+    private const double AngleDelta = Math.PI / 60;


Зависит от конвентов в команде конечно, но обычно вот такие простые поля (не считая констант) делаются свойствами, так удобнее регулировать правила доступа, которые в будущем могут измениться

Приватными полями делаются зависимости классов (как у тебя сделано с IPointGenerator в лейаутере)

desolver · 2023-12-06T14:34:16Z

cs/TagsCloudVisualization/TagsCloudVisualization.csproj

+<Project Sdk="Microsoft.NET.Sdk">
+
+  <PropertyGroup>
+    <TargetFramework>net6.0-windows10.0.22621.0</TargetFramework>


Какая-то жесть пролезла во фреймворк (во втором проекте тоже)

desolver · 2023-12-06T14:34:50Z

cs/TagsCloudVisualization/TagsCloudVisualization.csproj

+  </PropertyGroup>
+
+  <ItemGroup>
+    <PackageReference Include="coverlet.collector" Version="3.2.0" />


Вот эта штука нужна для проверки покрытия кода тестами, по идее нужна только для проекта с тестами, тут можно дропнуть

desolver · 2023-12-06T14:35:12Z

cs/TagsCloudVisualizationTests/CloudLayouter_Should.cs

+        rectangleCreation.Should().Throw<ArgumentException>();
+    }
+
+    private static IPointGenerator[] Generators =


Приватные методы должны стоять в классе после публичных

desolver · 2023-12-06T14:38:13Z

cs/TagsCloudVisualizationTests/CloudLayouter_Should.cs

+        new SpiralGenerator()
+    };
+
+    [TestCaseSource(nameof(Generators))]


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

Сейчас мы можем лишь предположить, что может измениться, если вдруг поменяется раскладка - самый яркий параметр тот, который проверяется в .Should, ты как раз о нем писал https://github.com/kontur-courses/tdd/pull/208/files#r1416175977

Давай прокинем его в аргументы теста, мотивацию я думаю ты понял

desolver · 2023-12-06T14:38:34Z

cs/TagsCloudVisualizationTests/CloudLayouter_Should.cs

+                .Min(existingRectangle => CalculateDistanceBetweenRectangles(currentRectangle, existingRectangle));
+
+            minimalDistanceToClosestRectangle.Should().BeLessThan(2 * squareSide);
+            rectanglesWithoutCurrent.Add(currentRectangle);


Не находишь эту строчку странной с точки зрения нейминга?)

desolver · 2023-12-06T14:39:33Z

cs/TagsCloudVisualization/CloudLayouter.cs

+
+public class CloudLayouter
+{
+    private readonly IPointGenerator pointsGenerator;


Не критично совсем, но в конструкторе ты принимаешь pointGenerator, а поле в классе называется pointS Generator

This reverts commit 4a9fea2, reversing changes made to 0a8f385.

Yrwlcm added 11 commits November 30, 2023 13:19

Created new project

604fc9f

Now can create first rectangle

b2e80d0

Minor CircularCloudLayouter test changes

f0f7176

Added SpiralGenerator class with tests

efecb27

CloudLayouter now uses SpiralGenerator to place rectangles

5e152bf

Added image creating feature with examples

b67eb7a

Now generates image after failed tests

45e399f

Merge commit '45e399fd6a5d1d6dbac254764de86c75ab62fa24'

eb4250b

Now comments are readable

6eada19

Added readme to TagsCloudVisualization

1b6ba8a

Now readme contains images

90875cd

desolver suggested changes Dec 4, 2023

View reviewed changes

Yrwlcm added 11 commits December 5, 2023 12:18

Removed unnecessary comments

a5ad440

Improved readability mostly variable refactoring

24ed218

File scoped namespaces

b351ece

Refactored SpiralGenerator

aca8f81

Refactored classes using IPointGenerator

6757a95

CloudLayouter now works with DI of IPointGenerator

dd52ffe

Layout image is now created with separate class

08506b8

Now tests in separate project also specified os version

a580398

New test for IPointGenerator implementations

cbf2bd0

Minor changes

c701e2c

Merge commit 'c701e2c98eaf2420a183825775b3dc705cacbc74'

132af71

Yrwlcm commented Dec 5, 2023

View reviewed changes

desolver suggested changes Dec 6, 2023

View reviewed changes

Yrwlcm added 4 commits December 8, 2023 14:15

More refactoring

0430a81

More refactoring

0a8f385

Merge branch 'master' of github.com:Yrwlcm/tdd

4a9fea2

Revert "Merge branch 'master' of github.com:Yrwlcm/tdd"

7682cd4

This reverts commit 4a9fea2, reversing changes made to 0a8f385.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Холстинин Егор #208

Холстинин Егор #208

Yrwlcm commented Dec 3, 2023

desolver Dec 4, 2023

desolver Dec 4, 2023

desolver Dec 4, 2023

desolver Dec 4, 2023

desolver Dec 4, 2023

desolver Dec 4, 2023

desolver Dec 4, 2023

desolver Dec 4, 2023

desolver Dec 4, 2023

desolver Dec 4, 2023

Yrwlcm Dec 5, 2023

Yrwlcm Dec 5, 2023 •

edited

Loading

desolver Dec 6, 2023

desolver Dec 6, 2023

desolver Dec 6, 2023

desolver Dec 6, 2023

desolver Dec 6, 2023 •

edited

Loading

desolver Dec 6, 2023

desolver Dec 6, 2023

desolver Dec 6, 2023

desolver Dec 6, 2023

desolver Dec 6, 2023

Холстинин Егор #208

Are you sure you want to change the base?

Холстинин Егор #208

Conversation

Yrwlcm commented Dec 3, 2023

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

Yrwlcm Dec 5, 2023 • 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

desolver Dec 6, 2023 • 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

Yrwlcm Dec 5, 2023 •

edited

Loading

desolver Dec 6, 2023 •

edited

Loading