Черных Илья #243
Черных Илья #243
Conversation
Так как изначально угол и радиус спирали равны нулю, то центр будет в прямоугольнике.
| addedRectangles = new List<Rectangle>(); | ||
| } | ||
|
|
||
| public Rectangle PutNextRectangle(Size rectangleSize) |
There was a problem hiding this comment.
Сейчас твой метод кладет прямоугольники всегда одним и тем же способом, но в идеале было бы хорошо алгоритм раскладки вынести куда-то в сторону и передавать его в конструктор, чтобы разные экземпляры CircularCloudLayouter могли раскладывать разными способами. Конкретно в этой задаче способ нужен только один, но на будущее лучше сделать класс гибче, и чтобы он почти ничего не знал о том, по какому алгоритму выбираются точки для прямоугольников
| currentAngleOfSpiral += Math.PI / 180; | ||
|
|
||
| // проверяем не прошли ли целый круг | ||
| if (currentAngleOfSpiral > 2 * Math.PI) |
There was a problem hiding this comment.
У тебя тут получилась не совсем спираль, т.к. ты сначала пытаешься расположить прямоугольники по периметру круга, и только если по периметру укладывать больше нельзя увеличиваешь радиус, тем самым создавая новый круг. Я не требую что-то менять в логике, но нейминг точно стоит поправить, потому что он не соответствует алгоритму
| rectangle = new Rectangle(new Point(x, y), rectangleSize); | ||
|
|
||
| // увеличиваем угол на 1 градус | ||
| currentAngleOfSpiral += Math.PI / 180; |
There was a problem hiding this comment.
Тут и в ифке ниже явно прослеживается какая-то константа, которую можно было бы вынести отдельно, чтобы не высчитывать каждый раз
|
|
||
| public class VisualizationCircularCloudLayout | ||
| { | ||
| public static void DrawAndSaveLayout(List<Rectangle> rectangles, string fileName, string filePath) |
There was a problem hiding this comment.
По названию можно заметить, что метод выполняет сразу 2 функции: генерирует графическое представление нашего облака и сохраняет его в какой-то файл по переданному пути и имени. Такого в идеале быть не должно, у одного метода должно быть одно назначение. Так что предлагаю разбить на две части: одна вернет графическое представление, другая - сохранит его в файл
| { | ||
| public static void DrawAndSaveLayout(List<Rectangle> rectangles, string fileName, string filePath) | ||
| { | ||
| const int padding = 10; |
There was a problem hiding this comment.
Эту константу можно передавать параметром или в метод или в конструктор, чтобы можно было отступ кастомизировать
| graphics.DrawRectangle(Pens.Black, shiftedRectangle); | ||
| } | ||
|
|
||
| bitmap.Save(Path.Combine(filePath, fileName), ImageFormat.Png); |
There was a problem hiding this comment.
Формат изображения тоже было бы хорошо передавать параметром, чтобы пользователь мог сам выбирать, в каком формате его сохранять
cs/Tests/Tests.csproj
Outdated
There was a problem hiding this comment.
Мелочь, но названия проектов лучше всегда делать понятными, даже для тестовых или учебных заданий, это развивает привычку использовать хорошие называния
| var testName = TestContext.CurrentContext.Test.Name; | ||
|
|
||
| VisualizationCircularCloudLayout.DrawAndSaveLayout(addedRectangles, $"{testName}.png", | ||
| pathImageStored); |
There was a problem hiding this comment.
Если переносишь один параметр в методе, то лучше переносить сразу все
| } | ||
|
|
||
|
|
||
| private static List<Size> GetGeneratedRectangleSizes(int countRectangles, int minSideLength, int maxSideLength) |
There was a problem hiding this comment.
Get тут не очень хорошо подходит, т.к. он подразумевает, что мы откуда-то что-то берем, когда на самом деле мы вычисляем (других методов тоже касается)
| } | ||
|
|
||
| return generatedSizes; | ||
| } |
There was a problem hiding this comment.
Как будто весь метод можно свернуть в компактную Linq-цепочку
|
|
||
| private bool IntersectWithAddedRectangles(Rectangle rectangle) | ||
| { | ||
| return addedRectangles.Any(addedRectangle => addedRectangle.IntersectsWith(rectangle)); |
There was a problem hiding this comment.
В этом классе нужно оставить только вычисление точек, а логику с прямоугольниками унести в CircularCloudLayouter
| // увеличиваем угол на 1 градус | ||
| currentAngleOfCircle += OneDegree; | ||
|
|
||
| // проверяем не прошли ли целый круг |
There was a problem hiding this comment.
Было бы круто, если бы шаг радиуса и шаг угла пользователь мог сам выбирать. Например, вводя эти параметры в конструкторе. Но чтобы не нужно было это всегда делать, можно значения по умолчанию поставить
| return CalculateDistanceBetweenPoints(centerRectangle1, centerRectangle2); | ||
| } | ||
|
|
||
| private static double CalculateDistanceBetweenPoints(Point point1, Point point2) |
There was a problem hiding this comment.
У тебя ту довольно много статических методов, которые можно вынести в отдельный статический класс-хелпер
| { | ||
| var random = new Random(); | ||
|
|
||
| var generatedSizes = Enumerable.Range(0, countRectangles) |
There was a problem hiding this comment.
Мелочь. В Linq цепочке лучше все элементы переносить на новую строчку, включая первый
| center, | ||
| stepIncreasingRadius: stepIncreasingRadius, | ||
| stepIncreasingAngle: stepIncreasingAngle | ||
| ); |
There was a problem hiding this comment.
Мелочь. Скобочка далеко уехала куда-то, ее не нужно отдельно переносить
|
|
||
| [TestCase(Math.PI / 4)] | ||
| [TestCase(Math.PI / 2)] | ||
| public void CalculateNextPoint_ShouldIncreaseAngle_WhenCalculateThreePoints(double stepIncreasingAngle) |
There was a problem hiding this comment.
Мне не очень нравится название теста.
Во-первых, тут есть магическое число 3, и без знания реализации алгоритма не понятно почему оно именно такое
Во-вторых, тут довольно слабые проверки на неравенство и больше/меньше. Мы по идее знаем, как должен работать алгоритм, соответственно мы можем написать более точные ассерты. Мне кажется, что раз есть возможность, то лучше сравнивать прям покоординатно точки, а возможность есть, т.к. мы шаг для радиуса и угла прям в конструктор в класса проверяемого передаем.
Т.е. мы можем так же получить три точки и проверить каждую на точное соответствие ожидаемой координате.
@GarageManager