Алешев Руслан #215
Алешев Руслан #215
Conversation
Реализованы базовые требования. Сделана сохранение изображений при ошибке. Добавлен README.md и изображения с примерами.
| public class CircularCloudLayouter | ||
| { | ||
| private readonly Point center; | ||
| private List<Rectangle> cloud; |
There was a problem hiding this comment.
Лучше IList, а то и вовсе ICollection. Это позволит не привязываться к реализации коллекции и поменять ее в любой момент
| int i = 0; | ||
| float it = (float)Math.PI / 21; | ||
| float ri = 50; |
There was a problem hiding this comment.
Нужны говорящие имена, модификаторы доступа, мб еще какие-нибудь. Можно даже summary, поясняющее физический смысл полей
| { | ||
| if (center.X <= 0 || center.Y <= 0) | ||
| throw new ArgumentException("Central point coordinates should be in positive"); | ||
| cloud = new List<Rectangle>(); |
There was a problem hiding this comment.
В CreateCloud всё равно будет создан новый лист. Мб тогда здесь это не нужно
|
|
||
| while (true) | ||
| { | ||
| bool findPlace = true; |
There was a problem hiding this comment.
Здесь и во многих других местах - var
| return rect; | ||
| } | ||
|
|
||
| public List<Rectangle> CreateCloud(List<Size> rectangleSizes) |
There was a problem hiding this comment.
Тут 2 варианта:
- Либо менять возвращаемый тип на
IReadonlyCollection, чтобы никто снаружи при получении списка прямоугольников не мог на него как-то влиять (очистить, например) - Либо не возвращать здесь вообще ничего, завести отдельную проперть и использовать ее во всех необходимых местах
Еще, мне как потенциальному пользователю твоего класса не ясна его роль. Например, что если я вызову этот метод 2 раза? Список старых прямоугольников очистится, но функция продолжить работать со старого индекса, получится не оч. Нужно решить как оно должно себя вести - пересоздавать или дополнять и дать соответствующее название
| return image; | ||
| } | ||
|
|
||
| private Point GetNextPoint() |
There was a problem hiding this comment.
Если знаком с перечислениями, было бы наглядно переписать на IEnumerable<Point> с yield return. Таким образом получится спрятать неприятный бесконечный цикл в этой небольшой функции
| { | ||
| cloud = new List<Rectangle>(); | ||
|
|
||
| foreach (var rectangleSize in rectangleSizes) |
There was a problem hiding this comment.
Если знаком с LINQ, можно схлопнуть всю функцию буквально в одну строку
| foreach (var previous in cloud) | ||
| { | ||
| if (rect.IntersectsWith(previous)) | ||
| { | ||
| findPlace = false; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Если знаком с LINQ, можно схлопнуть всю функцию буквально в одну строку
| [TestCase(123)] | ||
| public void CreateCloud_ReturnCorrectNumberOfRectangles(int rectCount) | ||
| { | ||
| var layouter = new CircularCloudLayouter(new Point(50, 50)); |
There was a problem hiding this comment.
- Давай явно обозначим комментариями где какая часть AAA
- Объект тестируемого класса часто называются
sut(system under testing). Это помогает быстрее ориентироваться в тесте
| [TestCase(10)] | ||
| [TestCase(20)] | ||
| [TestCase(200)] | ||
| public void CreateCloud_RectanglesShouldNotIntersect(int rectCount) |
There was a problem hiding this comment.
Если это попытка отрисовать облако при падении теста, то кажется что ты несколько неправильно понял задачу. Тебе нужен некий кусок кода, который вызовется при невыполнении любого условия из тестов выше, откуда-то возьмет входные данные теста и попробует отрисовать
Создание точек вынесено в отдельный класс с интерфейсом IPointsProvider При падении тестов на раскладку сохраняется файл с изображением Поправлены замечания
Реализованы базовые требования.
Сделана сохранение изображений при ошибке.
Добавлен README.md и изображения с примерами.