Зубков Андрей #210
Зубков Андрей #210
Conversation
| } | ||
|
|
||
| [Test] | ||
| public void CircularCloudLayoter_RectanglesListEmpty_WhenCreated() |
There was a problem hiding this comment.
обычно ты видишь либо полное имя теста, которое строится как <NameSpace>.<ClassName>.<MethodName>.<Casename>
либо ты видишь граф на котором все вышеуказанные пункты так же являются отдельными нодами, поэтому у тебя тут появляется дублирование: CircularCloudLayouter
TagCloudTests.CircularCloudLayouterTests.CircularCloudLayoter_RectanglesListEmpty_WhenCreated
| @@ -0,0 +1,46 @@ | |||
| namespace TagCloudTests | |||
| { | |||
| [TestFixture] | |||
cs/TagCloud/CircularCloudLayouter.cs
Outdated
| { | ||
| if (rectangleSize.Height <= 0 || rectangleSize.Width <= 0) | ||
| { | ||
| throw new ArgumentException(); |
There was a problem hiding this comment.
летит такой пользователь вертолета, и у него на экране вылетает ArgumentException, он не поймет чо произошло, упадет и разобьется
cs/TagCloud/CircularCloudLayouter.cs
Outdated
| { | ||
| public class CircularCloudLayouter : ICircularCloudLayouter | ||
| { | ||
| private readonly Point center; |
cs/TagCloud/CircularCloudLayouter.cs
Outdated
| private readonly Point center; | ||
| private readonly SpiralGenerator spiral; | ||
| private IList<Rectangle> rectangles; | ||
| public IList<Rectangle> Rectangles => rectangles.ToList().AsReadOnly(); |
cs/TagCloud/CloudDrawer.cs
Outdated
| layouter.PutNextRectangle(new Size(50 + random.Next(0, 100), 50 + random.Next(0, 100))); | ||
| } | ||
|
|
||
| var bitmap = new Bitmap(1920, 1080); |
There was a problem hiding this comment.
какой кошмар, а у тебя тут нет подсказок IDE что здесь что-то не так?
cs/TagCloud/CloudDrawer.cs
Outdated
|
|
||
| namespace TagCloud; | ||
|
|
||
| public class CloudDrawer |
There was a problem hiding this comment.
этот класс явно не Drawer, он 'я-все-делаю-er', давай декомпозируем, drawer тут явно выделяется в отдельную сущность
cs/TagCloud/CloudDrawer.cs
Outdated
| layouter.PutNextRectangle(new Size(50 + random.Next(0, 100), 50 + random.Next(0, 100))); | ||
| } | ||
|
|
||
| var bitmap = new Bitmap(1920, 1080); |
There was a problem hiding this comment.
что будешь делать когда прямоугольники не войдут на картинку?
cs/TagCloud/CloudDrawer.cs
Outdated
| layouter.PutNextRectangle(new Size(50 + random.Next(0, 100), 50 + random.Next(0, 100))); | ||
| } | ||
|
|
||
| var bitmap = new Bitmap(1920, 1080); |
There was a problem hiding this comment.
у тебя несколько раз повторяются эти константы, захочешь изменить - придется руками искать все места где они находятся и менять руками
| { | ||
| private const int Width = 1920; | ||
| private const int Height = 1080; | ||
| private SpiralGenerator spiral; |
There was a problem hiding this comment.
это не замечание, а скорее рекомендация
поле/переменную с сущностью которую тестят обычно называют sut (System Under Tests), это не правило, а скорее часто встречающийся подход
часто получается так что тестовый класс содержит в себе десятки других полей, когда ты называешь переменную/поле sut то визуально легко определить что в данном классе тестируется
cs/TagCloud/CircularCloudLayouter.cs
Outdated
| { | ||
| this.center = center; | ||
| Rectangles = new List<Rectangle>(); | ||
| spiral = new SpiralGenerator(center); |
There was a problem hiding this comment.
создавать зависимости руками в коде не является хорошей практикой, так у тебя cloudLayouter жестко начинает быть связан с конкретным spiralGenerator, а не с его публичным интерфейсом
классы должны зависеть от абстракций, давай будем работать с этой сущностью через абстракцию и принимать эту зависимость извне
cs/TagCloud/CircularCloudLayouter.cs
Outdated
| rectangle.Offset(-vector.Item1, 0); | ||
| } | ||
|
|
||
| private void ShiftRectangleInYDirection(ref Rectangle rectangle, (int, int) vector) |
There was a problem hiding this comment.
у тебя тут два метода с оптимизацией укладки, но есть целый ворох проблем:
-
сразу напрашивается вопрос о дублировании
-
оба метода получают целый вектор, а используют лишь только одну его ось, отчего метод начинает знать больше чем ему положено
-
старайся не использовать неименованные таплы, на таком простом примере возможно спустя полгода ты и быстро восстановишь контекст что такое item1 и item2, но проще либо именовать значения в тапле, либо все таки выделять какие-то контракты, к тому же не очень понятно чем не подошел
Point, те же(x, y), думаю в контексте координатной плоскости использование структурыPointдля описания вектора с именем переменнойvectorвполне понятно -
кажется что проверки на центр и 0 нужны только для первого прямоугольника (но тут я может чего не понял), и выполняются лишний раз для всех остальных
-
по сути у тебя сейчас получается очень жесткая неявная зависимость на генератор точек, если я реализую другую абстракцию, например: DinosaurPointGenerator - которая будет выдавать точки на плоскости в форме динозавра, то такая оптимизация все только сломает при подмене зависимости
-
кажется что производительность этой оптимизации будет сильно зависеть от выбраного шага для генератора, а результат не сильно то и стоит того, взгляни на разницу с оптимизацией и без нее
(справа с оптимизацией, слева - без)
ну и не забывай что у нас тут нет задачи сделать строго оптимальную укладку
cs/TagCloud/CloudDrawer.cs
Outdated
|
|
||
| public static class CloudDrawer | ||
| { | ||
| public static Bitmap DrawTagCloud(CircularCloudLayouter layouter, int border = 10) |
There was a problem hiding this comment.
почему у тебя drawer получает зависимость в методе извне, а не данные?
No description provided.