Conversation
| private const double radiusStep = 1; | ||
| private const double angleStep = .01; | ||
|
|
||
| private readonly Point center = center; |
There was a problem hiding this comment.
Если вводится primary constructor, то нет смысла выделять поле с таким же именем, пусть оно и readonly. Либо тогда использовать стандартные конструкторы
| public Rectangle PutNextRectangle(Size rectangleSize) | ||
| { | ||
| if (rectangleSize.Width <= 0 || rectangleSize.Height <= 0) | ||
| throw new ArgumentException(null, nameof(rectangleSize)); |
There was a problem hiding this comment.
Исключения без текста ошибки очень плохо помогают понять, что именно идёт не так. Старайся всегда указывать причину, в данном случае подойдёт "Recatngle size should be greater then zero"
| throw new ArgumentException(null, nameof(rectangleSize)); | ||
|
|
||
| placedRectangles.Add(new(FindRectanglePosition(rectangleSize), rectangleSize)); | ||
| return placedRectangles[^1]; |
There was a problem hiding this comment.
Не очень хороший подход сначала положить в список, а потом извлекать у него из конца. Да, на списке над массивом это будет работать быстро (хотя на 0.5 ns медленнее), но в общем случае не гарантированно. Ты уже имеешь вполне определённый объект, который нужно вернуть. Давай его выделять в переменную и потом возвращать
| while (!CanPlaceRectangle(new(GetRectanglePositionWithCurrentState(size), size))) | ||
| { | ||
| while (angle <= 2 * PI) | ||
| { | ||
| angle += angleStep; | ||
| if (CanPlaceRectangle(new(GetRectanglePositionWithCurrentState(size), size))) | ||
| return PullRectangleToCenter(size); | ||
| } | ||
|
|
||
| angle = 0; | ||
| radius += radiusStep; | ||
| } | ||
|
|
||
| return PullRectangleToCenter(size); | ||
| } |
There was a problem hiding this comment.
Здесь у тебя одно и то же условие проверяется в двух местах. Простая операция превратилась в двойной вложенный цикл. while в целом непросто просчитывать, а двойной while тем более. Оставь в теле только работу с углами, а проверка на допустимость в условии while
| } | ||
|
|
||
| private bool CanPlaceRectangle(Rectangle rectangle) | ||
| => !placedRectangles.Any(rectangle.IntersectsWith); |
There was a problem hiding this comment.
Код конечно выглядит красиво, но всего для 3 квадратов он аллоцирует невероятные 250МБ памяти и считает аж 10 секунд. А всё дело в боксинге и линку. Поэтому иногда лучше воспользоваться старым добрым форыч. Особенно если работаешь со структурами.
There was a problem hiding this comment.
Я не могу сделать выводы по данным профилирования, которые на картинке. Также мне неизвестно какого размера были эти 3 квадрата, а ведь это напрямую влияет на время выполнения и необходимый объём памяти (я понимаю, что речь не про объём памяти в пике, а суммарный объём памяти за выбранный промежуток времени). Если размеры были выбраны на порядки больше, чем размера шага по радиусу, то можно ждать целую вечность, пока точка на конце радиуса выйдет за пределы первого прямоугольника, а в это время будет создаваться огромное количество прямоугольников, которые пересекаются с первым и будут выброшены на свалку. Кроме того, если профилирование проводилось на предмет потребляемой памяти, то время выполнения нельзя приравнивать к результату бенчмарка на время (имхо, т. к. я не сильно разбираюсь). Время выполнения должно страдать из-за накладных расходов на сбор статистики (+ возможна ситуация с неудачными входными данными). Я это к тому, что у меня тесты размещают 50 прямоугольников с адекватными размерами несколько раз, и вся группа тестов выполняется меньше, чем за 1 секунду
There was a problem hiding this comment.
Честно, мне не нравится такой ответ. Это больше похоже на "не буду ничего исправлять, ты мне не сказал как делать". Я всегда открыт для диалога в телеграм, можно уточнить результаты профилирования, узнать как и при каких данных оно проводилось и что такое боксинг. Размер квадратов специально подобран так, чтобы ярко проявить ловушку в коде, на маленьких данных любой алгоритм будет работать быстро. Теперь обратимся к выделяемой памяти и коду. Все создаваемые объекты - это стурктуры. Структуры выделяются на стеке и являются типами-значениями, а значит никакой кучи на них тратиться вообще не должно. Однако на скрине прекрасно видно 250МБ. Возникает логичный вопрос, откуда это. Внутри анализатор любезно покажет 125МБ объектов типа Func<Rectangle, bool> и 125MБ объектов типа Rectangle_DisplayClass0_30. Но для упрощения понимания прямо в комментарии я указал на боксинг и предложил решение в виде foreach. Обидно, что оно было проигнорировано. К вопросу о накладных расходах. Не спорю, что профилирование немного нагружает систему, но это далеко не 9 секунд. Всё время как раз уходит на выделение и очистку памяти. Никто не запрещает провести бенчмарк, который покажет ровно то же самое. Я бы не стал писать комментарий, если бы не был уверен, что исправление сделает лучше. И конечно я проводил тесты с foreach. Они при том же анализе памяти выполняются за 500ms с аллокацией 0. Суть курса - научиться писать качественный код с глубинным пониманием происходящего, а так же замечать узкие места наперёд, по началу это подсвечивают наставники, а в дальнейшем это ожидается и от самих студентов
| var rectangles = sizes.Select(layouter.PutNextRectangle).ToArray(); | ||
| var image = DrawTagsCloud(rectangles); | ||
|
|
||
| var directory = Directory.CreateDirectory(Path.Combine(Environment.CurrentDirectory, "FailedTestVizualizations")); |
| var image = DrawTagsCloud(rectangles); | ||
|
|
||
| var directory = Directory.CreateDirectory(Path.Combine(Environment.CurrentDirectory, "FailedTestVizualizations")); | ||
| var path = Path.Combine(directory.FullName, $"{TestContext.CurrentContext.Test.Name}-{Path.GetRandomFileName()}.png"); |
There was a problem hiding this comment.
Здесь достаточно было бы простого гуида, но можно и так
| public void PutNextRectangle_Should_ReturnRectangleWithSameSize() | ||
| { | ||
| var layouter = new CircularCloudLayouter(new Point()); | ||
| var sizes = GetTestSizes(); |
There was a problem hiding this comment.
sizes - это перечисление. И по умолчанию мы не можем считать, что пройдясь дважды по нему получим одинаковой результат
|
|
||
| var rectangles = sizes.Select(layouter.PutNextRectangle).ToArray(); | ||
| var boundingRectangle = GetBoundingRectangle(rectangles); | ||
| var cloudRadius = (boundingRectangle.Width + boundingRectangle.Height) / 2.0 / 2.0; |
There was a problem hiding this comment.
А почему радиус - это высота плюс ширина описывающего прямоугольника? Поскольку у нас именно Circular, то и предполагаемая фигура должна быть кругом. Или не согласен?
There was a problem hiding this comment.
Я как раз нахожу радиус круга. Проблема в методах оценки. Из доступных средств у меня есть ограничивающий прямоугольник. Чтобы получить радиус мне нужно выбрать длину или ширину. Но есть ситуации, когда они сильно отличаются (прямоугольник не близок к квадрату). Такой случай, например, в первой визуализации README, которая построена на тестовых размерах. Я не придумал ничего лучше, чем взять среднее между длиной и шириной
There was a problem hiding this comment.
Вот теперь понял, что имеется в виду. Не стесняйся оставлять подсказки по коду в таких неочевидных местах, команда скажет спасибо не раз
|
|
||
| actualCenterOffset.Should().BeLessThanOrEqualTo(maximumCenterOffset); | ||
| } | ||
|
|
There was a problem hiding this comment.
Не хватает теста, который бы проверял, что это вообще круг. Косвенно можно делать через проверку плотности
There was a problem hiding this comment.
Та или иная величина плотности ни о чём сказать не может. Нужно знать её распределение. По распределению плотности определить фигуру — задача совсем другого порядка, как мне кажется. А если фигура неидеальная, то как измерить её "близость" к идеалу? Кроме того, обычный тест на плотность уже имеется
There was a problem hiding this comment.
Грубо можно оценить и по плотности. Если для описанного круга плотность заполнения велика (выше эмпирически полученного значения), то образованная фигура - почти круг. Если низкая - то вероятно это не совсем круг и стоит смотреть на результаты. Если подавать прямоугольники с большой вариацией длин, то может получиться что-то не хорошее, но как ты указал ранее, у нас фиксированный набор адекватных размеров. Отдельный тест можно не делать в данном случае, если указать в тесте с плотностью пометку про это.
| /// <summary> | ||
| /// The method creates a rectangle at a distance from the cloud center to the circumscribed circle of a rectangle. | ||
| /// </summary> | ||
| /// <param name="center"></param> | ||
| /// <param name="angle"></param> | ||
| /// <param name="distance"></param> | ||
| /// <param name="size"></param> | ||
| /// <returns></returns> |
There was a problem hiding this comment.
Комментарии получились грязными, брошены все описания принимаемых и возвращаемых значений. Здесь нужно либо описывать то, что принимается на вход, либо их не указывать вовсе
|
|
||
| var rectangles = sizes.Select(layouter.PutNextRectangle).ToArray(); | ||
| var boundingRectangle = GetBoundingRectangle(rectangles); | ||
| var cloudRadius = (boundingRectangle.Width + boundingRectangle.Height) / 2.0 / 2.0; |
There was a problem hiding this comment.
Вот теперь понял, что имеется в виду. Не стесняйся оставлять подсказки по коду в таких неочевидных местах, команда скажет спасибо не раз
|
|
||
| actualCenterOffset.Should().BeLessThanOrEqualTo(maximumCenterOffset); | ||
| } | ||
|
|
There was a problem hiding this comment.
Грубо можно оценить и по плотности. Если для описанного круга плотность заполнения велика (выше эмпирически полученного значения), то образованная фигура - почти круг. Если низкая - то вероятно это не совсем круг и стоит смотреть на результаты. Если подавать прямоугольники с большой вариацией длин, то может получиться что-то не хорошее, но как ты указал ранее, у нас фиксированный набор адекватных размеров. Отдельный тест можно не делать в данном случае, если указать в тесте с плотностью пометку про это.
2 participants
@GlazProject