Conversation
|
|
||
| public static class Program | ||
| { | ||
| public static void Main() |
There was a problem hiding this comment.
Вся эта логика построения в идеале, конечно, не в Main должна лежать. Вынеси в какой-нибудь класс (или группу классов) и вызывай из Main
| public IEnumerable<Point> GetNextPoint() | ||
| { | ||
| yield return _start; | ||
| while (true) |
There was a problem hiding this comment.
while (true) потенциально может быть опасен. Давай зададим какое-нибудь большое, но разумное ограничение
| [assembly: InternalsVisibleTo("TagsCloudVisualizationTests")] | ||
| namespace TagsCloudVisualization; | ||
|
|
||
| public class CircularCloudLayouter |
| private readonly IPointGenerator _pointGenerator; | ||
| private const int Density = 1; | ||
| private Point _center; | ||
| private bool isEnumerating; |
There was a problem hiding this comment.
Мотивация понятна, но решение использовать логику с isEnumerating для меня выглядит избыточным. Даже если бы Rectangle был ссылочным типом, метод GetLayout() всё равно возвращает элементы через yield, а значит перечисление заморожено на момент старта и yield не замечает возможных последующих изменений соурса - _prevRectangles. Плюс Rectangle - struct, так что возвращаются копии объектов. В многопоточном коде такая защита бы не помогла (ибо сама коллекция-источник не блокируется), а в однопоточном всё итак нормально, поэтому я бы логику эту выпилил
| isEnumerating = false; | ||
| } | ||
|
|
||
| public Rectangle PutNextRectangle(Size rectangleSize) |
There was a problem hiding this comment.
Методы PutNextRectangle и PressRectangleToCenter имеют похожие участки кода, посмотри, может получится что-то вынести в отдельные методы, чтобы не нарушать принцип DRY
| public static void Main() | ||
| { | ||
| // Раскладка фиксированных прямоугольников | ||
| var center = new Point(400, 400); |
There was a problem hiding this comment.
Напишу здесь, но относится ко всему проекту: многие параметры (вроде координат или настроек размера) можно вынести в константы, сделай это
| .Sum(x => x.Size.Width * x.Size.Height); | ||
| var areaRatio = rectanglesArea / potentialCircleArea; | ||
|
|
||
| // В среднем сейчас точность около 75%, как повысить (и надо ли, или уже норм) - вопрос хороший, хотелось бы на этот счёт посоветоваться |
| // Этот тест перестал работать из-за уменьшения шага в генераторе спирали, так что теперь точки ближе => прямоугольники | ||
| // не влезают так, как раньше | ||
| [Test] | ||
| public void ArchimedeanSpiralGenerator_ShouldGeneratePoints_ByArchimedeanSpiral() |
There was a problem hiding this comment.
Для себя подметил, что тест был написан и был зеленый. Сейчас в целом можно выпилить, если он красный
There was a problem hiding this comment.
Хорошо, оставлю его для демонстрации того, что 3 пункт задачи работает (это тот, в котором говорится о генерации изображения раскладки на упавшем тесте)
2 participants
@OvchinnikovNikita