базовая реализация структуры, алгоритма распределения и визуализации #247
Conversation
There was a problem hiding this comment.
В целом по структуре критичных вопросов нет, балл за предпроверку я поставлю
Круто, что ты использовал интерфейсы, это очень правильный подход - завязываться на абстракции вместо реализаций.
Правда основной целью задания было написание логики с использованием TDD, а тут тестов совсем нет, значит и принцип этот использован, к сожалению, не был.
cs/TagsCloudVisualization/TagCloud/PositionCalculator/SpiralPositionCalculator.cs
Outdated
Show resolved
Hide resolved
|
|
||
| private Rectangle RectangleFromParams(Size nextRectangleSize) | ||
| { | ||
| var x = (int)(center.X + currentOffset * Math.Cos(currentAngle)); //надо центровать? |
There was a problem hiding this comment.
По желанию. Я бы центровал, так, на мой взгляд, "правильнее", но это не принципиально
cs/TagsCloudVisualization/TagCloud/PositionCalculator/SpiralPositionCalculator.cs
Outdated
Show resolved
Hide resolved
cs/TagsCloudVisualization/TagCloud/PositionCalculator/SpiralPositionCalculator.cs
Outdated
Show resolved
Hide resolved
cs/TagsCloudVisualization/TagCloud/PositionCalculator/SpiralPositionCalculator.cs
Outdated
Show resolved
Hide resolved
cs/TagsCloudVisualization/TagCloud/CloudDrawer/CircularCloudDrawer.cs
Outdated
Show resolved
Hide resolved
cs/TagsCloudVisualization/TagCloud/CloudDrawer/CircularCloudDrawer.cs
Outdated
Show resolved
Hide resolved
|
Еще хочу добавить, что желательно выделять отдельные коммиты для разных логически отделимых частей задачи. Это помогает понять ход твоих мыслей, помогло бы при написании с использованием TDD, а так же в целом формирует привычку более структурного подхода к написанию кода |
cs/TagsCloudVisualization/TagCloud/PositionCalculator/SpiralPositionCalculator.cs
Outdated
Show resolved
Hide resolved
cs/TagsCloudVisualization/TagCloud/PositionCalculator/SpiralPositionCalculator.cs
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| private Rectangle MakeRectangleFromSize(Size nextRectangleSize) |
There was a problem hiding this comment.
По-хорошему логика с прямоугольниками должна была лежать в CircularCloudLayouter. Классу CircularPositionCalculator по идее не нужно ничего знать о прямоугольниках. либо о каких-то других фигурах, которые мы захотим размещать на окружностях. Если отталкиваться от S в SOLID, то ответственность класса CircularPositionCalculator заключается в нахождении следующей точки, на которую потенциально можно поставить прямоугольник или любую другую фигуру, а сами фигуры и операции над ними должны лежать на уровень выше - в CircularCloudLayouter.
Текущая реализация создает несколько проблем:
- При добавлении другого алгоритма расстановки фигур нам потребуется не просто алгоритм подсчета точки написать в новом классе, но и продублировать всю логику с прямоугольниками.
- Если мы захотим размещать, допустим, ромбы, а не прямоугольники, то нам придется вносить изменения прямо в CircularPositionCalculator, т.к. он сильно завязан на сами фигуры.
- Покрывать тестами этот класс сложнее, чем если бы он отвечал только за получение точек.
| namespace BaseCloudDrawer_Tests; | ||
|
|
||
| [TestFixture] | ||
| public class BaseCloudDrawer_Tests |
There was a problem hiding this comment.
На классы, которые формируются какие-то файлы, лучше всего писать Approval-тесты, обычные unit тут не очень хорошо подходят
| var fileName = "test_image"; | ||
| var format = ImageFormat.Png; | ||
|
|
||
| drawer.SaveToFile(bitmap, fileName, testDirectory, format); |
There was a problem hiding this comment.
На этот метод тесты можно было не писать, т.к. он внутри себя не содержит какой-то логики, а просто вызывает стандартный метод из шарповой библиотеки, тесты на который должны писать разработчики этой библиотеки. Мы, если ее используем, ей доверяем и дополнительные тесты можем не писать
|
|
||
| var result = layouter.PutNextRectangle(size); | ||
|
|
||
| result.IntersectsWith(layouter.Rectangles[0]).Should().BeFalse(); |
There was a problem hiding this comment.
Мы для тестов поле Rectangles сделали публичным, что не очень хорошо, т.к. оно стал публичным не только для тестов, но и для любого пользователя нашего класса. Я бы тут предложил самостоятельно складывать прямоугольники в коллекцию в самом тесте и проверять в конце, что они не пересекаются.
No description provided.