Шмаков Данил #207

Reltig · 2023-12-02T17:19:49Z

No description provided.

Reltig · 2023-12-02T17:23:50Z

gisinka · 2023-12-05T16:19:27Z

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

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

Reltig · 2023-12-07T09:55:23Z

cs/TagCloud/SpiralCloudShaper.cs

+        this.deltaAngle = deltaAngle;
+        currentAngle = 0;
+    }
+    public Rectangle GetNextPossibleRectangle(Size size)


Предполагается, что некоторые Shaper'ы могут использовать Size для просчёта следующей точки, но ладно, убрал.

Reltig · 2023-12-07T09:55:36Z

cs/TagCloud/ICloudShaper.cs

+
+namespace TagCloud;
+
+public interface ICloudShaper


Сделал

Reltig · 2023-12-07T09:57:09Z

cs/TagCloudTests/CloudTests_Should.cs

+    }
+
+    [TestCase(5, 10, 10, 20, 100)]
+    public void AlwaysFail(int widthMin, int widthMax, int heightMin, int heightMax, int count)


Этот тест вызывался для проверки, что провалившиеся тесты сохраняются в папку Fails, убрал.

Reltig · 2023-12-07T09:57:21Z

cs/TagCloud/SpiralCloudShaper.cs

+
+    public SpiralCloudShaper(Point center, double coefficient = 1, double deltaAngle = 0.1)
+    {
+        this.center = center;


Сделано

Считаю, что конструктор должен быть максимально простым, никаких проверок внутри не должно быть, его задача - инициализировать объект, если хочешь сделать проверку на входные параметры, то лучше сделать статичный метод который это будет проверять, и потом сам вызовет тебе приватный конструктор,
Ну и декомпозиция, все дела
Да и тесты так как то приятнее писать когда вызываешь какой нибудь метод Create вместо new

Как вариант валидацию выносить в отдельный приватный метод

Сделал через Create

Так же это работает и для других классов

Reltig · 2023-12-07T09:57:33Z

cs/TagCloudTests/CloudTests_Should.cs

+    private const string RelativePathToFailDirectory = @"..\..\..\Fails";
+
+    private CircularCloudLayouter layouter;
+    private Point center = new Point(0, 0);


gisinka · 2023-12-05T16:14:12Z

cs/TagCloudTests/Fails/TagCloudTests.CloudTests_Should.AlwaysFail(5,10,10,20,100).jpg

Судя по картинке есть пустые места, по условию задачи "Облако должно быть плотным, чем плотнее, тем лучше."
Нужно как-то сжимать

Для этого, по крайней мере для SpiralCloudShaper, придётся для каждого прямоугольника возвращать возможные позиции начинаю с центральной точки, а не с последней вычисленной (т.е. функция становиться квадратичной по времени)

Разве в условии есть ограничения на сложность вычислений?

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

Кажется, это требование должно быть приоритетнее

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

Потести на квадратиках и посмотри, какая картинка получится, должно что-то похожее на это получится

Уплотнил, пример в TagCloudTest/Examples

Reltig · 2023-12-07T10:06:43Z

cs/TagCloudTests/HasIntersectedRectangleTests_Should.cs

+    [Test]
+    public void ThrowException_ThenEmptyOrConainsOneElement()
+    {
+        Assert.Throws<ArgumentException>(() => Array.Empty<Rectangle>().HasIntersectedRectangles());


Разделил

Reltig · 2023-12-07T10:07:14Z

cs/TagCloudTests/CloudTests_Should.cs

+
+    private void SaveIncorrectResultsToJpg()
+    {
+        var rectangles = layouter.Rectangles;


Вынес в TagCloudDrawer

Reltig · 2023-12-07T10:07:23Z

cs/TagCloudTests/CloudTests_Should.cs

+        layouter.PutNextRectangle(new Size(1, 1));
+        layouter.PutNextRectangle(new Size(1, 1));
+        layouter.Rectangles.Count().Should().Be(2);
+        NotIntersectedAssetration();


Исправил

Reltig · 2023-12-07T10:08:37Z

cs/TagCloud/CircularCloudLayouter.cs

+        shaper = new SpiralCloudShaper(this.center);
+    }
+
+    public Rectangle PutNextRectangle(Size rectangleSize)


Сегодня узнал, что возможно создавать Size с отрицательными width и height. Тесты добавил в CloudTests_Should

Reltig · 2023-12-07T10:08:48Z

cs/TagCloudTests/HasIntersectedRectangleTests_Should.cs

+{
+    #region TestData
+
+    public static IEnumerable<TestCaseData> intersectedRectanglesTestCases =


Reltig · 2023-12-07T10:09:39Z

cs/TagCloudTests/CloudTests_Should.cs

+        NotIntersectedAssetration();
+    }
+
+    [TestCase(5, 10, 2, 4, 100)]


Добавил новые кейсы

А в чем различие тесткейсов? Неочевидно, тогда уж разницу надо в TestName объяснить

Оставил один

Reltig · 2023-12-11T16:27:40Z

cs/TagCloudTests/TagCloudDrawer.cs

+
+        using var bitmap = new Bitmap(maxX - minX + 2, maxY - minY + 2);
+        using var graphics = Graphics.FromImage(bitmap);
+        graphics.DrawRectangles(new Pen(Color.Red, 1),


Создал интерфейс, реализовал два класса с постоянным цветом и случайно генерируемым

gisinka · 2023-12-10T14:37:46Z

cs/TagCloudTests/SpiralCloudShaper_Should.cs

+    [TestCase(-1, 1, TestName = "NegativeCoefficient")]
+    [TestCase(1, -1, TestName = "NegativeDeltaAngle")]
+    [TestCase(-1, -1, TestName = "NegativeDeltaAngleAndCoefficient")]
+    public void Throw_OnCreationWith(double coefficient, double deltaAngle)


А тестирование валидных сценариев?

Не хватает тестов на уникальность точек, увеличение радиуса

Добавил тесты

Не увидел тесты на уникальность точек

Reltig · 2023-12-11T16:28:38Z

cs/TagCloudTests/TestData/IntersectedRectanglesTestCases.cs

+
+public class IntersectedRectanglesTestCases : IEnumerable
+{
+    private static TestCaseData[] data = new[]


Сделал, хотя предполагаю, что не совсем правильно понял идею

Да, вот так норм с тесткейссорс, правда имя последовательности можно было понятное дать, но в целом ок

gisinka · 2023-12-10T14:48:09Z

cs/TagCloudTests/CloudTests_Should.cs

+        layouter.PutNextRectangle(new Size(1, 1));
+        layouter.PutNextRectangle(new Size(1, 1));
+        layouter.Rectangles.Count().Should().Be(2);
+        NotIntersectedAssertation();


Assertion тогда уж, но тут лучше оставить layouter.Rectangles.HasIntersectedRectangles().Should().BeFalse();

Благодаря FluentAssertions данная проверка читается легко, как предложение + неявная передача данных через поля ухудшает читаемость и поддержку

Переименовал, но оставил метод, потому что DRY и логика работы NotIntersectedAssertion понятна из названия

Reltig · 2023-12-11T16:31:41Z

cs/TagCloudTests/TagCloudDrawer.cs

+                .ToArray());
+
+        var pathToFile = @$"{RelativePathToFailDirectory}\{TestContext.CurrentContext.Test.FullName}.jpg";
+        var absolutePath = Path.GetFullPath(pathToFile);


Декомпозировал

gisinka · 2023-12-10T14:56:11Z

cs/TagCloudTests/TagCloudDrawer.cs

+        var pathToFile = @$"{RelativePathToFailDirectory}\{TestContext.CurrentContext.Test.FullName}.jpg";
+        var absolutePath = Path.GetFullPath(pathToFile);
+        bitmap.Save(pathToFile);
+        Console.WriteLine($"Tag cloud visualization saved to file {absolutePath}");


Если ты про задание 3, то такой код будет писать в консоль сохранение любой раскладки, а там просилось сохранение раскладок упавших тестов

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

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

Reltig · 2023-12-11T16:34:08Z

cs/TagCloudTests/TagCloudDrawer.cs

+
+namespace TagCloud;
+
+public static class TagCloudDrawer


Сделал через интерфейс

Reltig · 2023-12-11T16:34:19Z

cs/TagCloudTests/TagCloudDrawer.cs

+{
+    private const string RelativePathToFailDirectory = @"..\..\..\Fails";
+
+    public static void Draw(CircularCloudLayouter layouter)


Исправил

gisinka · 2023-12-10T14:59:32Z

cs/TagCloudTests/TagCloudDrawer.cs

+            rectangles.Select(rect => rect with { X = -minX + rect.X, Y = -minY + rect.Y })
+                .ToArray());
+
+        var pathToFile = @$"{RelativePathToFailDirectory}\{TestContext.CurrentContext.Test.FullName}.jpg";


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

Комментировал выше

cs/TagCloudTests/CloudTests_Should.cs

gisinka · 2023-12-10T15:25:59Z

cs/TagCloudTests/CloudTests_Should.cs

+    [TestCase(1, 1, 100, TestName = "WithSquareShape")]
+    [TestCase(20, 10, 100, TestName = "WithRectangleShape")]
+    public void AddManyRectangles_WithConstantSize(int width, int height, int count)
+    {


Так же нужны тесты на плотность раскладки, форму - круг

Добавил дополнительный Assertation в методы с большим количество создаваемых прямоугольников

А на плотность?

cs/TagCloudTests/CloudTests_Should.cs

cs/TagCloudTests/SpiralCloudShaper_Should.cs

gisinka · 2023-12-12T15:07:45Z

cs/TagCloudTests/Extensions.cs

+    {
+        return rectangles
+            .SelectMany(
+                (x, i) => rectangles.Skip(i + 1), 


Слишком сложно, rectangles.Any(rectangle.IntersectsWith) для проверки пересечения текущего прямоугольника с последовательностью или пересечение пар layout.Any(x => layout.Any(y => x.IntersectsWith(y) && x != y));

gisinka · 2023-12-12T15:08:04Z

cs/TagCloudTests/Extensions.cs

+
+public static class Extensions
+{
+    public static IEnumerable<Tuple<T, T>> CartesianProduct<T>(this IEnumerable<T> source)


Неиспользуемое - можно удалить

gisinka · 2023-12-12T15:12:29Z

cs/TagCloud/CircularCloudLayouter.cs

+    {
+        this.center = center;
+        rectangles = new List<Rectangle>();
+        shaper = SpiralCloudShaper.Create(this.center);


Зачем делать интерфейс и жестко привязываться к реализации в конструкторе? Через конструктор можно принимать ICloudShaper

gisinka · 2023-12-12T15:44:58Z

cs/TagCloudTests/Examples/TagCloudTests.CircularCloudLayouterTests.WithSquareShape.jpg

Чет шакалит, но лан

Reltig added 4 commits November 30, 2023 11:14

create cloud layouter

eab9d69

add method to detect intersected rectangles in collection

39f073b

add cloud shaper, randomized tests and error logging

780f1bf

small fixes

d693b70

gisinka suggested changes Dec 5, 2023

View reviewed changes

gisinka reviewed Dec 5, 2023

View reviewed changes

first fix

6dbc999

gisinka reviewed Dec 10, 2023

View reviewed changes

cs/TagCloudTests/CloudTests_Should.cs Outdated Show resolved Hide resolved

gisinka reviewed Dec 10, 2023

View reviewed changes

cs/TagCloudTests/CloudTests_Should.cs Outdated Show resolved Hide resolved

gisinka reviewed Dec 10, 2023

View reviewed changes

cs/TagCloudTests/SpiralCloudShaper_Should.cs Outdated Show resolved Hide resolved

second fix

0d4d766

gisinka reviewed Dec 12, 2023

View reviewed changes

cs/TagCloudTests/Examples/TagCloudTests.CircularCloudLayouterTests.WithSquareShape.jpg

Copy link

gisinka Dec 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Чет шакалит, но лан

Шмаков Данил #207

Are you sure you want to change the base?

Шмаков Данил #207

Conversation

Reltig commented Dec 2, 2023

Reltig commented Dec 2, 2023

gisinka commented Dec 5, 2023 • edited Loading

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

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

This comment was marked as resolved.

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

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

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

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gisinka Dec 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

gisinka Dec 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gisinka Dec 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gisinka Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gisinka Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gisinka commented Dec 5, 2023 •

edited

Loading

gisinka Dec 10, 2023 •

edited

Loading

gisinka Dec 10, 2023 •

edited

Loading

gisinka Dec 10, 2023 •

edited

Loading

gisinka Dec 12, 2023 •

edited

Loading

gisinka Dec 12, 2023 •

edited

Loading