Попов Захар #257
Попов Захар #257
Conversation
BlizPerfect
changed the title
There was a problem hiding this comment.
Резюмируя
В целом для предпроверки - нормально, но код явно ещё нужнается в причёсывании. Хотелось бы видеть больше декомпозии на отдельные компонены, чтобы гига-методы с уровнем вложенности 4 стали проще!
Кроме того, хочется больше тестов, но думаю, ко второму дедлайну они будут доделаны. Простор для тестов тут, на самом деле, огромный. Не стесняйся тестировать даже то, что выходит за рамки основных классов: экстеншены, хелперы и всякие другие мелочи, которые есть.
Но как минимум, ожидаю увидеть тесты на все требования к раскладке
И не забывай обращать внимание на разнообразные предупреждения IDE, они помогают улучшить общую красоту кода ^_^
Про git могу сказать, круто, что теперь ветка отдельная, но название не несёт в себе какой-либо полезной информации. Хотелось бы, чтобы оно было понятнее.
На практике в названии ветки часто содержиться номер задачи или никнейм автора, а потом пара ключевых слов, о чём ветка, чтобы можно было их различать:
- task-1234/circular-visualizer
- BlizPerfect/render-engine
| var result = new Point[360]; | ||
| var step = 1; | ||
|
|
||
| for (int angle = 0; angle < 360; angle += step) |
There was a problem hiding this comment.
Почему в некоторых местах используется var, а в некоторых явное задание типа?
There was a problem hiding this comment.
Это мне когда студия сама подсказывает код, я там забываю поменять иногда тип данных, но всегда сам использую var)
There was a problem hiding this comment.
Понял. Но как и писал выше - хотелось бы видеть единобразный стиль во всём коде
There was a problem hiding this comment.
Я полностью с Вами согласен - тоже считаю, что всё должно быть единообразно.
Конкретно с var в циклах у меня забавная ситуация:
При решении задач на Leetcode я сначала руками всё писал через var, потом начал принимать подсказки студии, но менять int в цикле на var, но в итоге сдался и перестал это замечать)
Класс CircularCloudLayouterWorker был переписан как статичный для избежания создания экземпляра класса.
1. Добавлен интерфейс ICircularCloudLayouter. 2. Класс CircularCloudLayouter теперь реализует интерфейс ICircularCloudLayouter.
1. Рефакторинг класса CircularCloudLayouter. 2. Выделение сущности Circle, отвечающей за полчение координат на окружности.
1. Добавление новых тестов. 2. Общий рефакторинг кода.
cs/TagsCloudVisualization.Tests/CircularCloudLayouterMainRequirementsTests.cs
Outdated
Show resolved
Hide resolved
cs/TagsCloudVisualization.Tests/CircularCloudLayouterMainRequirementsTests.cs
Outdated
Show resolved
Hide resolved
cs/TagsCloudVisualization.Tests/CircularCloudLayouterMainRequirementsTests.cs
Outdated
Show resolved
Hide resolved
| [TestCase(0.55, ExpectedResult = true)] | ||
| [Repeat(10)] | ||
| public bool ShouldPlaceRectanglesTightly(double tight) | ||
| { | ||
| var boundingBoxSize = GetBoundingBoxSize(); | ||
| var boundingBoxArea = boundingBoxSize.Width * boundingBoxSize.Height; | ||
| var filledArea = _rectangles.Sum(r => r.Width * r.Height); | ||
| return ((double)filledArea / boundingBoxArea) >= tight; | ||
| } |
There was a problem hiding this comment.
Ой, тут, кажется, тоже квадрат пройдёт тест
There was a problem hiding this comment.
Видимо этот тест был убран в угоду ShouldPlaceCenterOfMassOfRectanglesNearCenter
resolved
| [TestCase(null, ExpectedResult = true)] | ||
| [TestCase(100, ExpectedResult = true)] | ||
| public bool Draw_CalculatesImageSizeCorrectly(int? padding) | ||
| { | ||
| var correctPadding = padding ?? _defaultPadding; |
There was a problem hiding this comment.
Дефолтное значение удобно применять в публичных интерфейсах (например библиотек), чтобы лишний раз не заморачиваться над передачей приемлемых по умолчанию параметров.
Как сейчас сделано в
Bitmap Draw(IList<Rectangle> rectangles, int? paddingPerSide = null)Но в тестах это лишнее. Можно null в TestCase заменить на 10 - будет нагляднее
| private Circle _arrangementСircle = new Circle(center); | ||
| private Random _random = new Random(); |
There was a problem hiding this comment.
Если ссылка никогда не модифицируется - можно смело делать её readonly
There was a problem hiding this comment.
В других местах тоже стоит исправлять комментарии
…al классов для тестов.
1. Тесты ShouldPlaceRectanglesInCircle и ShouldPlaceRectanglesTightly были заменены на более точный тест ShouldPlaceRectanglesInCircle, обьединяющий их функциональность. 2. ShouldPlaceRectanglesNearCenter переименован в ShouldPlaceCenterOfMassOfRectanglesNearCenter, так как предыдущее название вводило в заблуждение по функциональности теста. 3. Рефакторинг тестов.
|
|
||
| private bool[,] GetOccupancyGrid(int gridSize, double maxRadius, double step) | ||
| { | ||
| var result = new bool[gridSize, gridSize]; |
There was a problem hiding this comment.
Массив байт 1000x1000 это гарантированное попадание в Large Object Heap
If an object is greater than or equal to 85,000 bytes in size, it's considered a large object
Большие объекты дольше создаются, дольше удаляются (причём только при сборке 2-го поколения) и занимают много места в оперативной памяти. Подробнее можно почитать по ссылке выше.
Поэтому рекомендуется по возможности их избегать.
В данном случае, что было бы, если облако тегов содержало 2 диаметрально противоположных прямоугольнка на дальних координатах?
Им может банально не хватить оперативной памяти компьютера, чтобы "закрасить" этот grid.
Альтернативный вариант, который тут можно было реализовать: подсчёт площадей.
Мы можем легко посчитать сумму всех площадей прямоугольников: O(n) по времени, O(1) по памяти.
А также можем найти радиус описанной окружности всех прямоугольников. Что позволит посчитать площадь.
Отношение этих двух чисел будет показывать коэффициент плотности располагаемых прямоугольников.
| Math.Abs(rectangle.X - point.X), Math.Abs(rectangle.X + rectangle.Width - point.X)); | ||
| var dy = Math.Max( | ||
| Math.Abs(rectangle.Y - point.Y), Math.Abs(rectangle.Y + rectangle.Height - point.Y)); | ||
| return Math.Sqrt(dx * dx + dy * dy); |
There was a problem hiding this comment.
Функция встречается трижды в коде - хороший кандидат на выделение в метод
|
Так как дедлайн по этой задаче уже прошёл, а тест [TestCase(0.7, 1000)]
[Repeat(10)]
public void ShouldPlaceRectanglesInCircle(double expectedCoverageRatio, int gridSize)
{
var maxRadius = _rectangles.Max(r => r.GetMaxDistanceFromPointToRectangleAngles(_center));
var step = (2 * maxRadius) / gridSize;
var occupancyGrid = GetOccupancyGrid(gridSize, maxRadius, step);
var actualCoverageRatio = GetOccupancyGridRatio(occupancyGrid, maxRadius, step);
actualCoverageRatio.Should().BeGreaterThanOrEqualTo(expectedCoverageRatio);
}Выглядит легко, если особо не присматриваться :) 1 строкаactualCoverageRatio.Should().BeGreaterThanOrEqualTo(expectedCoverageRatio);Хорошая проверка, которая в случае расхождения показывает дельту. Это удобно при дебаге. 2 строкаvar actualCoverageRatio = GetOccupancyGridRatio(occupancyGrid, maxRadius, step);Уже интереснее, реализация такова: private double GetOccupancyGridRatio(bool[,] occupancyGrid, double maxRadius, double step)
{
var totalCellsInsideCircle = 0;
var coveredCellsInsideCircle = 0;
for (var x = 0; x < occupancyGrid.GetLength(0); x++)
{
for (var y = 0; y < occupancyGrid.GetLength(0); y++)
{
var cellCenterX = x * step - maxRadius + _center.X;
var cellCenterY = y * step - maxRadius + _center.Y;
var distance = Math.Sqrt(
Math.Pow(cellCenterX - _center.X, 2) + Math.Pow(cellCenterY - _center.Y, 2));
if (distance > maxRadius)
{
continue;
}
totalCellsInsideCircle += 1;
if (occupancyGrid[x, y])
{
coveredCellsInsideCircle += 1;
}
}
}
return (double)coveredCellsInsideCircle / totalCellsInsideCircle;
}Я бы сказал: "тут слишком много ответственности", это нарушает Single Responsibility Principle (S из SOLID), потому что:
Сразу скажу, я не особо вдавался в подробности что такое 1. Вырез сеткиИтак, у нас есть полотно, закрашенное чёрным и белым цветом: Мы могли бы написать функцию, которая наше чёрно-белое полотно превратит в такое-же чёрно-белое полотно, но с альфа-каналом (как png без фона). Где прозрачный фон это Реализация public interface IShape
{
bool Inside(Point point);
}
public static class GridExtensions
{
public static bool?[,] Carve(this bool[,] source, IShape shape)
{
var grid = new bool?[source.GetLength(0), source.GetLength(1)];
for (var i = 0; i < source.GetLength(0); i++)
{
for (var j = 0; j < source.GetLength(1); j++)
{
if (shape.Inside(new Point(i, j)))
grid[i, j] = source[i, j];
}
}
return grid;
}
}
Соблюли ли мы тут SRP (Single Responsibility Principle)? Как думаешь, какие ещё принципы из SOLID тут явно соблюдаются? 2. Математические вычисленияТеперь у нас есть функция вырезания фигур. Следующим этапом нам нужно завести фигуру public class Circle(Point center, double radius) : IShape
{
public bool Inside(Point point) => Math.Sqrt(Math.Pow(center.X - point.X, 2) + Math.Pow(center.Y - point.Y, 2)) <= radius;
}Хм... Будто бы тут что-то не так... public static class PointExtensions
{
public static double DistanceTo(this Point first, Point second)
=> Math.Sqrt(Math.Pow(first.X - second.X, 2) + Math.Pow(first.Y - second.Y, 2));
}Теперь и public class Circle(Point center, double radius) : IShape
{
public bool Inside(Point point) => point.DistanceTo(center) <= radius;
}3. Подсчёт точекТеперь, когда у нас есть вырезанная фигура, подсчёт общего количества точек не составит труда. var grid = occupancyGrid.Carve(new Circle(_center, maxRadius));
var total = grid.Cast<bool?>().Count(x => x != null); // пиксель не пустой
var colored = grid.Cast<bool?>().Count(x => x == true); // пиксель закрашен3 строкаvar occupancyGrid = GetOccupancyGrid(gridSize, maxRadius, step);Ага, понятно: создаёт изначальное полотно на котором окрашивает прямоугольники. private (int start, int end) GetGridIndexesInterval(
int rectangleStartValue,
int rectangleCorrespondingSize,
double maxRadius,
double step)
{
var start = (int)((rectangleStartValue - _center.X + maxRadius) / step);
var end = (int)((rectangleStartValue + rectangleCorrespondingSize - _center.X + maxRadius) / step);
return (start, end);
}
private bool[,] GetOccupancyGrid(int gridSize, double maxRadius, double step)
{
var result = new bool[gridSize, gridSize];
foreach (var rect in _rectangles)
{
var xInterval = GetGridIndexesInterval(rect.X, rect.Width, maxRadius, step);
var yInterval = GetGridIndexesInterval(rect.Y, rect.Height, maxRadius, step);
for (var x = xInterval.start; x <= xInterval.end; x++)
{
for (var y = yInterval.start; y <= yInterval.end; y++)
{
result[x, y] = true;
}
}
}
return result;
}1. Keep it simpleНачнём с Но его можно сделать проще: private (int start, int end) GetIndexesInterval(int position, int length)
=> (position, position + length - 1);Соответственно private bool[,] GetOccupancyGrid(int gridSize)
{
var result = new bool[gridSize, gridSize];
foreach (var rect in _rectangles)
{
var xInterval = GetIndexesInterval(rect.X, rect.Width);
var yInterval = GetIndexesInterval(rect.Y, rect.Height);
for (var x = xInterval.start; x <= xInterval.end; x++)
{
for (var y = yInterval.start; y <= yInterval.end; y++)
{
result[x, y] = true;
}
}
}
return result;
}2. Yet another decompositionЕсли присмотреться к методу
Это вполне себе отдельные методы, которые могут быть разделены: Метод, позволяющий закрасить прямоугольник в любом двумерном масивеpublic static T[,] Fill<T>(this T[,] source, Rectangle rectangle, T color)
{
// не создаю копию массива в целях производительности
// поэтому функция не чистая, а хотелось бы
var xInterval = GetIndexesInterval(rectangle.X, rectangle.Width);
var yInterval = GetIndexesInterval(rectangle.Y, rectangle.Height);
for (var i = xInterval.start; i <= xInterval.end; i++)
{
for (var j = yInterval.start; j <= yInterval.end; j++)
source[i, j] = color;
}
return source;
}
private static (int start, int end) GetIndexesInterval(int position, int length) => (position, position + length);Метод GetOccupancyGrid, закрашивающий все прямоугольникиprivate bool?[,] GetOccupancyGrid(int gridSize)
{
var result = new bool?[gridSize, gridSize];
result.Fill(new Rectangle(0, 0, gridSize, gridSize), false);
return _rectangles.Aggregate(result, (current, rectangle) => current.Fill(rectangle, true));
}4-5 строкаvar maxRadius = _rectangles.Max(r => r.GetMaxDistanceFromPointToRectangleAngles(_center));
var step = (2 * maxRadius) / gridSize;Тут, в целом, всё понятно. Круто, что вычисление максимального расстояния до точек вынесено в отдельный метод Пока поверим, что он возвращает правильные значения. Но было бы здорово видеть на него тесты! Рефакторим тест[TestCase(0.7, 1000)]
[Repeat(10)]
public void ShouldPlaceRectanglesInCircle(double expectedCoverageRatio, int gridSize)
{
var maxRadius = _rectangles.Max(r => r.GetMaxDistanceFromPointToRectangleAngles(_center));
var grid = new bool?[gridSize, gridSize].Fill(new Rectangle(0, 0, gridSize, gridSize), false);
grid = _rectangles
.Aggregate(grid, (current, rectangle) => current.Fill(rectangle, true))
.Carve(new Circle(_center, maxRadius));
var total = grid.Cast<bool?>().Count(x => x != null);
var covered = grid.Cast<bool?>().Count(x => x == true);
var actual = (double)covered / total;
actual.Should().BeGreaterThanOrEqualTo(expectedCoverageRatio);
}Выглядит понятнее но... не работает: IndexOutOfRange. Что-ж, кажется, мы затевали рефакторинг, чтобы вносить изменения было проще? Самое время попробовать. На ум приходит идея вычислить прямоугольник, описывающий все существующие прямоугольники и преобразовать координаты всех прямоугольников в новый описывающий прямоугольник со сторонами 1000x1000. var boundingBox = _rectangles.GetBoundingBox();
grid = _rectangles
.Select(x => x.Map(boundingBox, new Rectangle(0, 0, gridSize, gridSize)))
.Aggregate(grid, (current, rectangle) => current.Fill(rectangle, true))
.Carve(new Circle(_center, maxRadius));Реализации таковы: public static Rectangle GetBoundingBox(this IList<Rectangle> rectangles)
{
return new Rectangle(
rectangles.Min(r => r.X),
rectangles.Min(r => r.Y),
rectangles.Max(r => r.X + r.Width),
rectangles.Max(r => r.Y + r.Height));
}
public static Rectangle Map(this Rectangle rectangle, Rectangle from, Rectangle to)
{
return new Rectangle(
new Point(
rectangle.X.ConvertInterval(from.X, from.Width, to.X, to.Width),
rectangle.Y.ConvertInterval(from.Y, from.Height, to.Y, to.Height)
),
new Size(
(int)(rectangle.Width * GetStretch(from.X, from.Width, to.X, to.Width)),
(int)(rectangle.Height * GetStretch(from.Y, from.Height, to.Y, to.Height))
)
);
}
private static int ConvertInterval(this int value, int oldMin, int oldMax, int newMin, int newMax)
{
var oldRange = oldMax - oldMin;
var newRange = newMax - newMin;
return (value - oldMin) * newRange / oldRange + newMin;
}
private static double GetStretch(int oldMin, int oldMax, int newMin, int newMax)
{
var oldRange = oldMax - oldMin;
var newRange = newMax - newMin;
return (double)newRange / oldRange;
}Финально тест выглядит так public void ShouldPlaceRectanglesInCircle(double expectedCoverageRatio, int gridSize)
{
var maxRadius = _rectangles.Max(r => r.GetMaxDistanceFromPointToRectangleAngles(_center));
var grid = new bool?[gridSize, gridSize].Fill(new Rectangle(0, 0, gridSize, gridSize), false);
var boundingBox = _rectangles.GetBoundingBox();
grid = _rectangles
.Select(x => x.Map(boundingBox, new Rectangle(0, 0, gridSize, gridSize)))
.Aggregate(grid, (current, rectangle) => current.Fill(rectangle, true))
.Carve(new Circle(_center, maxRadius));
var total = grid.Cast<bool?>().Count(x => x != null);
var covered = grid.Cast<bool?>().Count(x => x == true);
var actual = (double)covered / total;
actual.Should().BeGreaterThanOrEqualTo(expectedCoverageRatio);
}У нас получилось исправить баг, не задевая внутренности написаных ранее методов. Должно быть, это хорошо. У меня на самом деле тут где-то в преобразованиях прямоугольников есть баг, но идея этого текста показать, как можно декомпозировать запутанную логику в отдельные компоненты, поэтому баг позволю себе вынести за скобки. В идеале, как уже говорил, тут можно было посчитать площади без матриц. |
@Folleach
Черновой вариант решения второй домашней работы.
Примеры работ с разными параметрами:
1.
Принцип расположения прямоугольников:
Прямоугольники ставятся на окружности увеличивающегося радиуса с центром в точке, совпадающей с центром первого поставленного прямоугольника.