Большаков Николай #256
Большаков Николай #256
Conversation
|
Проекты не добавились в солюшн, давай поправим |
| namespace TagsCloud_Tests; | ||
|
|
||
| [TestFixture] | ||
| public class CircularCloudLayouter_Constructor_Tests |
There was a problem hiding this comment.
Как правило 1 класс на который пишутся тесты = 1 класс с тестами
Давай объединим классы с тестами на конструктор и на метод
|
|
||
| [TestFixture] | ||
| public class CircularCloudLayouter_Constructor_Tests | ||
| { |
There was a problem hiding this comment.
Принято использовать UpperCamelCase для именования классов
| private readonly SortedRectanglesList _rectangles; | ||
| private readonly List<Rectangle> _rectanglesSpiralStack; | ||
|
|
||
| public SpiralCircularCloudLayouter( |
| Center = center; | ||
| } | ||
|
|
||
| public Point Center { get; } |
| Point center) | ||
| { | ||
| _rectangles = new SortedRectanglesList(); | ||
| _rectanglesSpiralStack = []; |
There was a problem hiding this comment.
можно инициализировать при объявлении (на мой вкус так более читабельно)
| { | ||
| [Test] | ||
| [Description("В конструкторе указывается центр первого прямоугольника")] | ||
| public void Constructor_SetsCenterOfFirstRectangle() |
There was a problem hiding this comment.
Описание теста не соответствует тому что в нем делается. Т.е. в нем тестируется не только конструктор, но и метод PutNextRectangle
Чтобы протестировать конструктор (хотя спорный вопрос нужно ли его вообще тестировать, в идеальном мире в конструкторе не должно быть логики которая нуждается тестах, только присвоение) нужно проверить что он корректно сеттит center, другой логики в нем нет :)
| [Description("Прямоугольники не имеют пересечений")] | ||
| public void PutNextRectangle_ReturnsNonIntersectingWithEachOtherRectangles() | ||
| { | ||
| var count = 1_000; |
There was a problem hiding this comment.
Пишу как разделитель для тысяч. Иногда с ними удобнее читать, мне кажется, особенно, когда числа большие, но я все время так пишу. Не стоит так делать?
There was a problem hiding this comment.
вкусовщина, мне кажется :) кому как, можно оставить
| for (var i = 0; i < count; i++) | ||
| { | ||
| var size = new Size(random.Next(2, 100), random.Next(2, 100)); | ||
| var putRectangle = PutNextRectangle(size); |
There was a problem hiding this comment.
а почему такой нейминг? этот метод возвращает rectangle, логичнее назвать rectangle (или что-то подобное)
| rectangles[i] = putRectangle; | ||
| } | ||
|
|
||
| for (var i = 0; i < count; i++) |
| Rectangle rectangle, | ||
| Comparer<Rectangle>? comparer) | ||
| { | ||
| Debug.Assert(rectangles != null); |
| Size rectangleSize, | ||
| out Rectangle result) | ||
| { | ||
| var (rectangle, direction) = GetInitialRectangleAndRotationDirection( |
There was a problem hiding this comment.
тут каждый раз вычисляется в каком направлении предыдущий прямоугольник к предпредыдущему и кажется на момент когда мы добавляем прямоугольник мы знаем в каком направлении он к предыдущему -> двойная работа. давай запоминать в каком прямоугольник к предыдущему чтобы не считать дважды
| /// стоящий на границе с previousRectangle с той же стороны, что и prePreviousRectangle, | ||
| /// и изначальное направление поиска подходящей для него позиции. | ||
| /// </summary> | ||
| private static (Rectangle, Direction) GetInitialRectangleAndRotationDirection( |
There was a problem hiding this comment.
давай поделим на 2 метода, нарушается srp
| direction = Direction.Left; | ||
| } | ||
|
|
||
| var rectangle = new Rectangle(position, rectangleSize); |
There was a problem hiding this comment.
еще не поняла идею, почему прямоугольник от которого все считается берется именно в location предпредыдущего?
There was a problem hiding this comment.
Для нахождения такой позиции, чтобы прямоугольник был наиболее прижат к остальным. Нужно, чтобы новый прямоугольник был, если возможно, внутри спирали или на ней. У меня расположение прямоугольников идет по часовой стрелке и, если предполагать, что предыдущий и предпредыдущий оба находятся на спирали, обходя предыдущий прямоугольник против часовой стрелки от позиции предпредыдущего, мы в первую очередь проверяем позиции внутри спирали.
| var target = GetTarget(rectangleSize, _rectanglesSpiralStack[^1], direction); | ||
| var hasIntersection = _rectangles.HasIntersection(rectangle, direction, 0, out var j); | ||
| var isTargetOvershot = false; | ||
|
|
There was a problem hiding this comment.
давай 82 и 83 строку не будем вычислять отдельно вне цикла, внесем в цикл (мб придется поправить методы и/или заменить на do while)
| /// Вспомогательная структура для хранения прямоугольников, отсортированных по координатам сторон, | ||
| /// с возможностью получения прямоугольника по индексу в отсортированном списке. | ||
| /// </summary> | ||
| public class SortedRectanglesList |
There was a problem hiding this comment.
1 - давай перепишем с использованием sortedList
2 - не совсем поняла задумку: почему один и тот же прямоугольник храним четырежды? кажется все взаимодействие с этим классом сводится к проверке пересечения прямоугольников (но это можно проверить и без дублирования листов)
There was a problem hiding this comment.
Я когда это решение писал, думал про SortedList, но почему-то подумал, что в нем взятие по индексу происходит за O(log(n)), что мне не подходило. Оказывается, нет.
Эта структура нужна только для оптимизации, чтобы избавиться от постоянной проверки пересечения со всеми прямоугольниками сразу. Она хранит прямоугольники, отсортированные по каждой из сторон, и при проверке на пересечение возвращает еще и индекс пересечения, чтобы потом, когда сдвинем прямоугольник, не проверять все прямоугольники сначала, а с этого индекса.
| // 0: первую сторону в первый раз обходит только от изначальной позиции; | ||
| // 1-3: 3 другие стороны полностью; | ||
| // 4: первую сторону обходит от угла прямоугольника). | ||
| for (var i = 0; i < 5; i++) |
There was a problem hiding this comment.
кажется здесь i соответствует смене направления? давай заменим связку i + RotateCounterclockwise итерацией по направлениям
| Debug.Assert(_rectangles.Count == 1 | ||
| && _rectangles.Get(Direction.Left, 0) == _rectanglesSpiralStack[0]); | ||
|
|
||
| return GetSecondRectangle(rectangleSize, _rectanglesSpiralStack[0]); |
There was a problem hiding this comment.
кажется нет необходимости выделять получение 2-го прямоугольника в отдельный метод, давай в общем методе получения учтем случай когда непонятно направление
instead always put it on top of the first rectangle
@masssha1308