Савенко Дмитрий#258
Conversation
There was a problem hiding this comment.
Круто, что сразу разделил на два отдельных проекта - рабочая практика, чтобы тесты собирались и гонялись отдельно от сборки основного кода проекта
There was a problem hiding this comment.
Название файлика не совпадает с названием класса внутри
| } | ||
|
|
||
| var rectangles = layouter.GetRectangles(); | ||
| rectangles.Count.Should().Be(100); |
There was a problem hiding this comment.
Эта проверка тянет на отдельный тест, ведь текущий, судя по названию, должен проверять лишь круговое расположение, но не то, что все переданные прямоугольники смогли расположиться
| } | ||
|
|
||
| [Test] | ||
| public void GenerateLayout_ShouldCreateCircularCloud() |
There was a problem hiding this comment.
А нужен ли тест выше, если текущий почти полностью повторяет его работу за исключением размеров прямоугольников?
| private double GetDistanceFromCenter(Point point) | ||
| { | ||
| var dx = point.X - center.X; | ||
| var dy = point.Y - center.Y; | ||
| return Math.Sqrt(dx * dx + dy * dy); | ||
| } |
There was a problem hiding this comment.
Один и тот же метод дублируется дважды, тянет на вынесение в экстеншн
| using System.IO; | ||
| using TagsCloudVisualization; | ||
|
|
||
| public class CircularCloudLayouter |
There was a problem hiding this comment.
Кажется, что в этом классе смешались две ответственности - он и располагает прямоугольники, и умеет с файловой системой работать. Давай оставим один лейаутер и отдельную зависимость, которая будет уметь сохранять файл
| namespace TagsCloudVisualizationTest; | ||
|
|
||
| [TestFixture] | ||
| public class RectangleLayouterTests |
There was a problem hiding this comment.
Давай ещё напишем а-ля нагрузочный тест, вставим оооочень много прямоугольников и проверим, что тест отработает за адекватное время
| [TestFixture] | ||
| public class LayoutGeneratorTests |
There was a problem hiding this comment.
Только после ревью основного проекта понял, что тесты на этот класс особо и не были нужны, ведь он просто вызывает классы и методы, которые ты уже тестируешь в соседних тест-классах, то есть не имеет собственной бизнес-логики
Но раз уж они есть, то пусть остаются))
|
|
||
| public Rectangle ShiftRectangleToCenter(Rectangle rectangle, List<Rectangle> rectangles) | ||
| { | ||
| if (rectangles.Any(r => r.IntersectsWith(rectangle))) |
There was a problem hiding this comment.
Перед вызовом метода ShiftRectangleToCenter ты в цикле генерировал точки, пока не добрался до возможности вставить прямоугольник без пересечения с другими прямоугольниками. То есть к моменту вызова текущего метода мы точно уверены, что прямоугольник ни с чем уже не пересекается и ветка внутри if точно не будет выполнена. Так что, если у тебя нет планов на этот код, можешь его смело грохать))
| if (rectangles.Any(r => r.IntersectsWith(nextRectangle))) | ||
| break; |
There was a problem hiding this comment.
На каждой итерации цикла сдвига ближе к центру эта проверка затрачивает серьезные ресурсы. Конечно, можно сейчас подзапариться за оптимизацию, но хочется делать это только после того как будет уверенность, что этот метод вообще нужен
Ты проверял, он правда более плотное облако генерирует? Может быть даже без шифтера получалось достаточно плотное облако?
Если проверял, давай обсудим на двух конкретных примерах - расположение облака с большим количеством прямоугольников с шифтером и без
2 participants
@desolver