Сахабутдинов Ильфир #240
Сахабутдинов Ильфир #240
Conversation
…sable. Теперь ITagsCloudVisualizer наследует IDisposable
| return rectangle; | ||
| } | ||
|
|
||
| public IReadOnlyCollection<Rectangle> GetRectangles() |
There was a problem hiding this comment.
Круто что используешь IReadonly*
nit: оборачивать в AsReadonly нет большой необходимости, достаточно того что произойдет сужение типа и уже только поэтому снаружи никто не сможет мутировать коллекцию. Конечно, технически все еще будет возможность привести тип обратно, но в таком случае это будет уже проблемой того самого разработчика. Ну и как следствие. можно перейти от функции к свойству
| Func<Rectangle, bool> checkNegativeMove, | ||
| Func<Rectangle, int, Rectangle> doMove) | ||
| { | ||
| if (checkPositiveMove(rectangle) && checkNegativeMove(rectangle) |
There was a problem hiding this comment.
nit: если не ошибаюсь, можно свернуть в checkPositiveMove(rectangle) == checkNegativeMove(rectangle)
| private Point GetRectangleLocation(Size rectangleSize) | ||
| { | ||
| var shiftFromCenter = -1 * rectangleSize / 2; | ||
| if (!rectangles.Any()) |
There was a problem hiding this comment.
nit: холиварный момент, но часто рекомендую явно проверять размер, когда это возможно и не создает накладных расходов, иначе внутри будет создан лишний итератор, который попробует взять первый элемент. Последние версии dotnet'а научились это решать, но тем не менее
|
|
||
| var x = center.X + layer * distanceLayersDifference * Math.Cos(angle); | ||
| var y = center.Y + layer * distanceLayersDifference * Math.Sin(angle); | ||
| var wholeX = Convert.ToInt32(x); |
There was a problem hiding this comment.
nit: можно и просто кастонуть wholeX = (int) x. Внутри Convert происходит примерно то же самое
| if (angle > fullCircleTurn) | ||
| { | ||
| UpdateLayer(); | ||
| angle = 0; |
There was a problem hiding this comment.
Думаю, надежнее делать angle %= fullCircleTurn или хотя бы angle -= fullCircleTurn на случай, если в fullCircleTurn будет нецелое количество betweenAngleDifference
|
|
||
| namespace TagsCloudVisualization; | ||
|
|
||
| public class CircularCloudLayouter : ICircularCloudLayouter |
There was a problem hiding this comment.
Ходят легенды, что если помечать классы sealed (особенно при большом количестве типов), можно получить неощутимые рост производительности. А если серьезно, то если класс не спроектирован под расширение, то лучше сразу запечатывать. Если понадобится, всегда можно будет открыть
| public CircularCloudLayouter(Point center) | ||
| { | ||
| this.center = center; | ||
| rectangles = new List<Rectangle>(); |
There was a problem hiding this comment.
nit: Возможно, это какая-то привычка, но всё это и ниже можно либо писать сразу напротив поля, либо и так значения по умолчанию
| isDisposed = false; | ||
| } | ||
|
|
||
| public void AddVisualizationRectangles(IEnumerable<Rectangle> rectangles) |
There was a problem hiding this comment.
nit: Выглядит норм, но кажется что перестарался с декомпозицией. Я бы сложил всё что касается graphics в одну функцию
| graphics.FillEllipse(Brushes.Red, centerRectangle); | ||
| } | ||
|
|
||
| ~TagsCloudVisualizer() |
|
|
||
| namespace TagsCloudVisualization.Tests; | ||
|
|
||
| internal static class CircularCloudLayouterTestCases |
There was a problem hiding this comment.
Есть вариант сделать этот класс и класс с тестами partial. Файл с кейсами назвать, например, CircularCloudLayouterTests.Cases.cs, чтобы было понятно, что 2 файла содержат общий тип. Тогда кейсы можно будет сделать приватными, а во втором классе можно будет избавиться от проксирования
@a_zhelonkin