Conversation
…ayouter На первый вызов PutNextRectangle создает прямоугольник по центру
Прямоугольники должны лежать как можно ближе друг к другу
…ия прямоугольников
Облако растет относительно центра, а не какой-то рандомной точки Облако получается круглым, а не овальным Допускаем, что центр облака может немного сместиться, но не сильно
Теперь используется общий угол для спирали Спираль сделал более плотной Чуть-чуть поработал над улучшением читаемости
|
|
||
| [SetUp] | ||
| public void SetUp() | ||
| { |
There was a problem hiding this comment.
Давай не будем выносить это в setUp потому что пользы это не приносит, а читабельность ухудшает
| private const int VisualizationScale = 5; | ||
| private const int VisualizationPadding = 10; | ||
|
|
||
| private Point _center; |
There was a problem hiding this comment.
В тестах нет кейсов с разными center, хорошо бы добавить. Сейчас с помощью редактирования center в тесте это сделать не получится (layouter всегда создается с центром (0,0)). Давай уберем SetUp и общие поля
| var fileName = $"{DateTime.UtcNow.ToString(FileNameTimestampFormat)}_{Guid.NewGuid().ToString(GuidFormat)}.{FailureImagesFileExtension}"; | ||
| var filePath = Path.Combine(outDir, fileName); | ||
|
|
||
| new CloudVisualizer(_layouter.CreatedRectangles, filePath, VisualizationScale, VisualizationPadding).Visualize(); |
There was a problem hiding this comment.
CloudVisualizer.Visualize() освобождает disposable объекты?
There was a problem hiding this comment.
CloudVisualizer.Visualize() освобождает disposable объекты?
Да, в методе Visualize класса CloudVisualizer IDisposable объекты обернуты в using var
|
|
||
| [TearDown] | ||
| public void TearDown() | ||
| { |
There was a problem hiding this comment.
Давай вынесем логику визуализации в отдельный метод
| var rectangles = new Rectangle[count]; | ||
|
|
||
| for (var i = 0; i < count; i++) | ||
| rectangles[i] = _layouter.PutNextRectangle(DefaultRectangleSize); |
There was a problem hiding this comment.
Давай будем создавать прямоугольники разных размеров для точности тестов. Еще можно добавить тесты по типу "сначала маленькие, потом большие прямоугольники", "сначала большие, потом маленькие", "большие и маленькие вперемешку" и т.д.
|
|
||
| for (var i = 0; i < rectangles.Length; i++) | ||
| for (var j = i + 1; j < rectangles.Length; j++) | ||
| rectangles[i].IntersectsWith(rectangles[j]).Should().BeFalse(); |
There was a problem hiding this comment.
Упадет с неинформативной ошибкой "Expected boolean to be False, but found True.", стоит добавить больше информации о проблеме
| rectangles[i].IntersectsWith(rectangles[j]).Should().BeFalse(); | ||
| } | ||
|
|
||
| // Облако должно быть достаточно плотным |
There was a problem hiding this comment.
комментарии можно перенести в test description
| [Test] | ||
| public void PutNextRectangle_ManyRectangles_BoundingRectangleContainsCenter() | ||
| { | ||
| var rectangles = CreateRandomRectangles(CreatedRectanglesLimit); |
There was a problem hiding this comment.
Тест пройдет если центр будет в одном углу BoundingRectangle, а все прямоугольники в противоположном. Давай усилим проверку?
| [Test] | ||
| public void PutNextRectangle_ManyRectangles_ShapeHasCircularAspectRatio() | ||
| { | ||
| var rectangles = CreateRandomRectangles(CreatedRectanglesLimit); |
There was a problem hiding this comment.
Тест пройдет в любом кейсе если BoundingRectangle будет квадратом, но это не гарантирует что облако будет круглым. Например, в кейсе когда облако будет в форме креста тест не упадет. Давай усилим проверку
|
|
||
| var centerOffsetRatio = distanceFromRequiredCenter / maxSize; | ||
|
|
||
| centerOffsetRatio.Should().BeLessThanOrEqualTo(MaxAllowedCenterOffsetRatio); |
There was a problem hiding this comment.
20% кажется многовато на первый взгляд, почему именно такое значение?
There was a problem hiding this comment.
20% кажется многовато на первый взгляд, почему именно такое значение?
Скорее было выбрано экспериментально. Был выбор между 0.1 и 0.2. Решил остановиться на 0.2. В коммите 4c9542f заменил на 0.1
| // Минимум половина ограничивающего прямоугольника должна быть занята прямоугольниками | ||
| private const double ExpectedMinDensity = 0.5; | ||
|
|
||
| // Допустимое значение смещения центра облака (не более 20 процентов от размера облака) |
There was a problem hiding this comment.
комментарии можно убрать, они дублируют код
|
|
||
| namespace TagsCloudVisualization; | ||
|
|
||
| public class CircularCloudLayouterTests |
There was a problem hiding this comment.
Хорошо бы добавить тесты на пограничные кейсы
|
|
||
| public List<Rectangle> CreatedRectangles => _createdRectangles; | ||
|
|
||
| public Rectangle PutNextRectangle(Size rectangleSize) |
There was a problem hiding this comment.
Хорошо бы добавить валидацию входных данных
| { | ||
| while (true) | ||
| { | ||
| var pointOnSpiral = GetNextSpiralPoint(); |
There was a problem hiding this comment.
Если я правильно поняла алгоритм, то для прямоугольника (1000,1000) будет 1000 итераций. Давай регулировать GetNextSpiralPoint исходя из размера следующего прямоугольника
|
|
||
| private double _currentAngle; | ||
|
|
||
| public List<Rectangle> CreatedRectangles => _createdRectangles; |
There was a problem hiding this comment.
Лучше возвращать readonly коллекцию, т.к. в другом случае коллекцию можно изменить извне
|
|
||
| public class CircularCloudLayouter(Point center) | ||
| { | ||
| // Шага 0.1 радиан достаточно для плотного наполнения спирали прямоугольниками |
|
|
||
| private static Rectangle CreateRectangle(Size size, int centerX, int centerY) | ||
| { | ||
| var left = centerX - size.Width / 2; |
There was a problem hiding this comment.
Для нечетных размеров будет смещение?
There was a problem hiding this comment.
Для нечетных размеров будет смещение?
Да, для нечетных размеров центр сместится на 0.5 пикселя относительно заданной точки
cs/CloudLayouterVisualizer/README.md
Outdated
| @@ -0,0 +1,7 @@ | |||
| # 50 прямоугольников размером 30x20: | |||
|
|
|||
| <img width="2220" height="2190" alt="cloud_50_30x20" src="https://github.com/user-attachments/assets/149dc4a1-f847-4988-9ef5-169d69dd08dc" /> | |||
There was a problem hiding this comment.
Такие ссылки в readme лучше не использовать. 1 - они временные, 2 - не отображаются в превью, 3 - не посмотреть без интернета
cs/CloudLayouterVisualizer/README.md
Outdated
| @@ -0,0 +1,7 @@ | |||
| # 50 прямоугольников размером 30x20: | |||
There was a problem hiding this comment.
Хочется больше описания: какой алгоритм, какой центр и т.д.
| { | ||
| var center = DefaultCenter; | ||
|
|
||
| var sizes = Enumerable |
There was a problem hiding this comment.
Размеры формируются также как в PutNextRectangle_LargeThenSmallRectangles_DoNotIntersect
| graphics.Clear(BackgroundColor); | ||
|
|
||
| using var fillBrush = new SolidBrush(RectangleFillColor); | ||
| using var font = new Font(FontFamilyName, FontSize); |
@masssha1308