Дмитрий Леонтьев #248

Dary0302 · 2024-11-13T14:44:00Z

No description provided.

task 1 complete

Buskervil

Тэкс, ну раз тема tdd, давай про тесты поговорим).

Давай еще раз поглядим на требования:

Некоторые требования к раскладке:

Форма итогового облака должна быть близка к кругу с центром в точке center.
Прямоугольники не должны пересекаться друг с другом.
Облако должно быть плотным, чем плотнее, тем лучше.

Надо постараться проверить по требованиям:

раскладка получается круглой
центр облака находится в центре
раскладка плотная, насколько?
нет пересечений (это у тебя есть)

И добавь тесты на все классы, которые у тебя есть (экстеншны не протестировал)

Buskervil · 2024-11-13T15:10:03Z

cs/TagsCloudVisualization/CloudLayouter/CircularCloudLayouter.cs

+    {
+        while (newRectangle.IsNotIntersectOthersRectangles(Rectangles) && desiredPosition != currentPosition)
+        {
+            currentPosition += currentPosition < desiredPosition ? 1 : -1;


Размазал логику определения того, в какую сторону смещение происходит. Тут считаешь текущую позицию, а реальное направление смещения задано вне этого метода в 32 строке. Надо объединить 2 кусочка в один

В плане направления смещения логику подсобрал - стало лучше

Buskervil · 2024-11-13T15:23:02Z

cs/TagsCloudVisualization/RectangleExtension.cs

+        new(rectangle.Location.X + rectangle.Width / 2, rectangle.Location.Y + rectangle.Height / 2);
+
+    public static bool IsNotIntersectOthersRectangles(this Rectangle rectangle, IEnumerable<Rectangle> rectangles) => 
+        rectangles.All(rect => !rectangle.IntersectsWith(rect));


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

Buskervil · 2024-11-13T15:25:38Z

cs/TagsCloudVisualization/RectangleExtension.cs

+    public static Point Add(this Point selfPoint, Point otherPoint) =>
+        new(selfPoint.X + otherPoint.X, selfPoint.Y + otherPoint.Y);
+
+    public static Point Subtract(this Point selfPoint, Point otherPoint) =>


класс называется RectangleExtension, а расширение для точки

теперь в PointerExtensions лежит расширение для Rectangle)

Buskervil · 2024-11-13T15:43:08Z

cs/TagsCloudVisualization/CloudLayouter/CircularCloudLayouter.cs

+    {
+        var newRectangle = MoveRectangleAxis(rectangle, rectangle.GetCenter().X, center.X,
+            new(rectangle.GetCenter().X < center.X ? 1 : -1, 0));
+        newRectangle = MoveRectangleAxis(newRectangle, newRectangle.GetCenter().Y, center.Y,


Интересно будет посмотреть как это выглядит). Подозреваю, что по оси Х плотность будет выше)

Судя по демо рисункам, мои подозрения не оправдались)

Buskervil · 2024-11-13T15:49:02Z

cs/TagsCloudVisualization.Tests/SpiralTest.cs

+    public void GetNextPoint_WithCorrectParameters_ReturnsCorrectPoint()
+    {
+        var spiral = new Spiral(new(0, 0), 1);
+        spiral.GetNextPoint().Should().Be(new Point(0, 0));


Думаю проверку, что первая точка это "центр" можно вынести в отдельный тест

Buskervil · 2024-11-13T16:07:02Z

cs/TagsCloudVisualization.Tests/CircularCloudLayouterTest.cs

+
+        for (var i = 0; i < countRectangles; i++)
+        {
+            var size = new Size(random.Next(20, 100), random.Next(40, 100));


Ну вот, а если бы класс RectangleGenerator был написан удобно, ты бы использовал здесь его)

Buskervil · 2024-11-13T16:08:03Z

cs/TagsCloudVisualization.Tests/CircularCloudLayouterTest.cs

+        }
+        isIntersectsWith.Should().Be(countRectangles);
+
+        rectangles.Clear();


Зачем очистка, она тут только AAA нарушает. Надо написать так, чтобы тесты не аффектили друг друга.

Buskervil · 2024-11-13T16:18:15Z

cs/TagsCloudVisualization.Tests/CircularCloudLayouterTest.cs

+    public void SetUp()
+    {
+        center = new Point(0, 0);
+        layouter = new CircularCloudLayouter(center);


При параллельном запуске перетираешь себе данные

Buskervil · 2024-11-13T16:48:18Z

cs/TagsCloudVisualization/CloudLayouter/GenerateRandomRectangles.cs

+
+namespace TagsCloudVisualization.CloudLayouter;
+
+public static class RectangleGenerator


Возможно, этот класс стоит в сборку с тестами унести

Buskervil · 2024-11-13T16:50:29Z

cs/TagsCloudVisualization/CloudLayouter/GenerateRandomRectangles.cs

+
+        for (var i = 0; i < count; i++)
+        {
+            var size = new Size(random.Next(20,100), random.Next(20, 80));


Вроде разброс по высоте меньше, чем по ширине, но все равно может быть такое, что ширина 20, а высота 79. Вряд ли есть слова, у которых высота в 4 раза больше ширины, так что тут алгоритм генерации не совсем соответствует предметной области.

Buskervil · 2024-11-13T16:56:24Z

cs/TagsCloudVisualization/CloudLayouter/CircularCloudLayouter.cs

+    public readonly List<Rectangle> Rectangles = new();
+    private readonly Spiral spiral = new(center, 2);
+
+    public Rectangle PutNextRectangle(Size sizeRectangle)


Уровень абстракции этого метода какой-то неравномерный. Он часть работы делает сам, часть делегирует.
По красоте оставить бы в нем что-нибудь такое
...
var rectangle = FindNextValidRectanglePosition(sizeRectangle);
rectangle = MoveRectangleCloserToCenter(rectangle);
rectangles.Add(rectangle);
...

…tangles

add more tests, separate the layouter logic and the GenerateRandomRec…

add task2 and task3

add tests for RectangleDraw

Buskervil · 2024-11-17T12:27:37Z

cs/TagsCloudVisualization.Tests/SpiralTest.cs

+{
+    [TestCase(0, 0, 1)]
+    [TestCase(2, 10, 2)]
+    public void Constructor_WithCorrectParameters_NotThrow(int x, int y, double step)


А отрицательные аргументы можно?

можна)

а вот отрицательный шаг, думаю, что нельзя

Buskervil · 2024-11-17T12:38:17Z

cs/TagsCloudVisualization/CloudLayouter/CircularCloudLayouter.cs

+        return newRectangle;
+    }
+
+    public void PutRectangles(IEnumerable<Rectangle> rectangles)


Старайся соблюдать порядок: сначала все публичные методы, затем все приватные

Buskervil · 2024-11-17T12:40:25Z

cs/TagsCloudVisualization/CloudLayouter/CircularCloudLayouter.cs

+        return newRectangle;
+    }
+
+    public void PutRectangles(IEnumerable<Rectangle> rectangles)


Вот это можно унести в Extension метод для ILayouter.

класс будет чище

метод можно будет использовать для разных Lyouter-ов (он по природе своей универсальный)

Buskervil · 2024-11-17T12:41:44Z

cs/TagsCloudVisualization/CloudLayouter/CircularCloudLayouter.cs

+
+public class CircularCloudLayouter(Point center) : ILayouter
+{
+    public readonly List<Rectangle> Rectangles = new();


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

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

На практике так заморачиваются не всегда, но хорошо инкапсулированные коллекции в классе выглядят вот так:

private readonly List<Rectangle> _rectangles = new(); public IReadOnlyList<Rectangle> Rectangles => _rectangles.AsReadOnly();

Здесь внутри класса доступны все операции, а снаружи можно только читать.

Можно еще публиковать коллекцию вот так
public IReadOnlyList<Rectangle> Rectangles => _rectangles
Почти то же самое, но тут мы отдаем объект листа как IReadOnlyList. Снаружи можно будет сделать каст к листу и снова портить инкапсуляцию. А в первом варианте так сделать не получится, потому что возвращатеся объект ReadOnlyCollection у которого кроме чтения методов нет и кастовать к листу уже не получится.

Buskervil · 2024-11-17T12:51:57Z

cs/TagsCloudVisualization/CloudLayouter/CircularCloudLayouter.cs

+    public readonly List<Rectangle> Rectangles = new();
+    private readonly Spiral spiral = new(center, 2);
+
+    public Rectangle PutNextRectangle(Size sizeRectangle)


Buskervil · 2024-11-17T16:38:17Z

cs/TagsCloudVisualization.Tests/CircularCloudLayouterTest.cs

+
+    private static double GetAreaOfCircle(double radius) => Math.PI * Math.Pow(radius, 2);
+
+    private static int GetAreaOfRectangles(CircularCloudLayouter layouter) =>


Выше у тебя везеде IEnumerable, а тут вдруг layouter. Перечисление лучше

Buskervil · 2024-11-17T16:44:50Z

cs/TagsCloudVisualization.Tests/CircularCloudLayouterTest.cs

+        double areaOfCircle,
+        double averageDistance,
+        double radius) =>
+        areaOfRectangles / areaOfCircle * (1 - averageDistance / radius);


А первый множитель расчитан неправильно. Перепроверь пожалуйста.
Вообще я посмотрел. У тебя плотность на твоих тестовых данных выше 75% везде. Это хороший результат 🔥

Buskervil · 2024-11-17T16:50:24Z

cs/TagsCloudVisualization.Tests/CircularCloudLayouterTest.cs

+    }
+
+    [Test]
+    public void PutNextRectangle_CheckIntersectRectangles_ReturnFalse()


плохое название

PutNextRectangle_CheckIntersectOfRectangles тоже плохо. Название теста всегда должно что-то утверждать о коде.
В данном случае, что пересечений не будет сколько прямоугольников ты бы не положил.
Хорошее название: PutNextRectangle_CreateLayoutWithoutIntersetions

Buskervil · 2024-11-17T16:50:32Z

cs/TagsCloudVisualization.Tests/CircularCloudLayouterTest.cs

+    [TestCase(50)]
+    [TestCase(100)]
+    [TestCase(1000)]
+    public void PlacedRectangles_WhenCorrectNotIntersects_ReturnTrue(int countRectangles)


плохое название

Buskervil · 2024-11-17T16:52:30Z

cs/TagsCloudVisualization.Tests/CircularCloudLayouterTest.cs

+    [TestCase(-100, 100, 10, Description = "Center in the fourth quadrant of the coordinate axis")]
+    [TestCase(-100, 100, 100, Description = "Center in the fourth quadrant of the coordinate axis")]
+    [TestCase(-100, 100, 1000, Description = "Center in the fourth quadrant of the coordinate axis")]
+    public void RectanglesDensity_WithDifferentNumberRectanglesAndDifferentCenters(


Вот тут как раз не хватает 3й части в названии. Надо что-то типа такого:
PutNextRectangle_CreateLaoyoutWithOver75PercentDencity_OnRandomNumberRectangles

… the logic of the draftsman was divided

add more tests, the tests have been improved, add RectangleExtension,…

Buskervil · 2024-11-18T16:19:06Z

cs/TagsCloudVisualization.Tests/CircularCloudLayouterTest.cs

+    }
+
+    [Test]
+    public void PutNextRectangle_ReturnsFirstRectangleThatIsInCenterOfCloud()


Название хорошее, но не правдивое. Действительно возвращает, действительно в центре, но только когда это первый прямоугольник. Лучше PutNextRectangle_PlaceFirstRectangleAtCenter

Buskervil · 2024-11-18T16:19:55Z

cs/TagsCloudVisualization.Tests/CircularCloudLayouterTest.cs

+    [TestCase(50)]
+    [TestCase(100)]
+    [TestCase(1000)]
+    public void PutNextRectangle_CheckIntersectOfRectangles(int countRectangles)


Плохое название. Название теста должно утверждать что-то о поведении класса.

Buskervil · 2024-11-18T16:20:47Z

cs/TagsCloudVisualization.Tests/PointExtensionTest.cs

+        return point1.Add(point2);
+    }
+
+    private static IEnumerable<TestCaseData> PointerSumTestCases()


Потренировался - молодец)

Buskervil · 2024-11-18T16:21:45Z

cs/TagsCloudVisualization.Tests/RectangleDraftsmanTest.cs

+    }
+
+    [TestCase(null)]
+    public void CreateImage_OnInvalidParameters_ThrowsArgumentException(string filename)


Ввыкинь тесты на сохранение. Это не ответственность класса, который рисует

Buskervil · 2024-11-18T16:24:17Z

cs/TagsCloudVisualization.Tests/RectangleGeneratorTest.cs

+public class RectangleGeneratorTest
+{
+    [Test]
+    public void GenerateRandomRectangles_CheckNumberRectangles_NumberRectanglesGeneratedMatchesRequested()


Просто утверждение о поведении. CheckNumberRectangles лишнее. Лучше GenerateRandomRectangles_ReturnGeneratedRectanglesNumberMatchesRequested

Buskervil · 2024-11-18T16:25:08Z

cs/TagsCloudVisualization.Tests/SpiralTest.cs

+{
+    [TestCase(0, 0, 1)]
+    [TestCase(2, 10, 2)]
+    public void Constructor_WithCorrectParameters_NotThrow(int x, int y, double step)


а вот отрицательный шаг, думаю, что нельзя

Buskervil · 2024-11-18T16:25:50Z

cs/TagsCloudVisualization.Tests/SpiralTest.cs

+namespace TagsCloudVisualizationTests;
+
+[TestFixture]
+public class SpiralTest


В этом классе все имена тестов хорошие

Buskervil · 2024-11-18T16:27:38Z

cs/TagsCloudVisualization/Program.cs

+using TagsCloudVisualization.Extension;
+using TagsCloudVisualization.RectangleGenerator;
+
+var center = new Point(0, 0);


Обсуждали, что лучше бы сделать сборку class library

В принципе можешь это потом сделать. Сейчас забьем

Buskervil · 2024-11-18T16:31:07Z

cs/TagsCloudVisualization/RectangleGenerator/RectangleGenerator.cs

+{
+    public static IEnumerable<Rectangle> GenerateRandomRectangles(int countRectangles)
+    {
+        var rectangles = new List<Rectangle>(countRectangles);


Отдаешь IEnumerable, но под капотом держишь list. Можно сделать ленивую генерацию

Buskervil · 2024-11-18T16:52:57Z

cs/TagsCloudVisualization.Tests/CircularCloudLayouterTest.cs

+[TestFixture]
+public class CircularCloudLayouterTest
+{
+    private List<Rectangle> rectanglesForCrashTest = new();


Надо не забывать во всех тестах писать сюда данные)

Может возникать проблема при параллельном запуске тестов (ты уже встречался с этим). Для решения этой проблемы можно сделать поле статическим и обернуть его в ThreadLocal. Тогда для каждого потока rectanglesForCrashTest будет свой

можно в общем-то забить. Просто отметил на подумать

update tests raundlayout

Dary0302 and others added 2 commits November 13, 2024 19:36

task 1 complete

0397783

Merge pull request #1 from Dary0302/task-1

25ef8f2

task 1 complete

Buskervil reviewed Nov 13, 2024

View reviewed changes

Dary0302 and others added 6 commits November 16, 2024 21:08

add more tests, separate the layouter logic and the GenerateRandomRec…

da0a95b

…tangles

Merge pull request #2 from Dary0302/task1-bug-fixes

9023ff6

add more tests, separate the layouter logic and the GenerateRandomRec…

add task2 and task3

1ed59c7

Merge pull request #3 from Dary0302/task1-bug-fixes

ab4bc98

add task2 and task3

add tests for RectangleDraw

c467e75

Merge pull request #4 from Dary0302/task1-bug-fixes

f9f8199

add tests for RectangleDraw

Buskervil reviewed Nov 17, 2024

View reviewed changes

Dary0302 and others added 2 commits November 18, 2024 19:03

add more tests, the tests have been improved, add RectangleExtension,…

f241aed

… the logic of the draftsman was divided

Merge pull request #6 from Dary0302/task1-bug-fixes

f33f6c7

add more tests, the tests have been improved, add RectangleExtension,…

Buskervil reviewed Nov 18, 2024

View reviewed changes

Dary0302 and others added 2 commits November 18, 2024 22:31

update tests raundlayout

5caad72

Merge pull request #7 from Dary0302/task1-bug-fixes

fe86382

update tests raundlayout


		namespace TagsCloudVisualization.CloudLayouter;

		public static class RectangleGenerator


		private static double GetAreaOfCircle(double radius) => Math.PI * Math.Pow(radius, 2);

		private static int GetAreaOfRectangles(CircularCloudLayouter layouter) =>

Дмитрий Леонтьев #248

Are you sure you want to change the base?

Дмитрий Леонтьев #248

Conversation

Dary0302 commented Nov 13, 2024

Buskervil left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment