Conversation
There was a problem hiding this comment.
Если смотреть на саму идею решения - она правильная к ней вопросов нет. Наверное её можно было бы оптимизировать, те же сдвиги для уплотнения, но кажется для учебного задания все хорошо
С точки зрения написания кода - не очень, он очень неконсистентный, тут пишешь так, там эдак. Лучше определится со стилистикой написания и писать в одном стиле, пусть даже неправильном, чем чередовать
| Point newCenter = new Point(x, y); | ||
| layouter = new CircularCloudLayouter(newCenter); | ||
| var newRecSize = new Size(30, 10); | ||
|
|
||
| Rectangle rectangle = layouter.PutNextRectangle(newRecSize); |
There was a problem hiding this comment.
А почему ты чередуешь var и явное указание типа? Хорошей практикой считается везде писать var-ы, за исключениями, когда компилятор сам тебе не даст написать var из-за того что не сможет определить тип
| }; | ||
| } | ||
|
|
||
| public Point GetRectangleCenter(Rectangle rectangle) |
There was a problem hiding this comment.
У нас этот метод дублируется в в Layout-ере и в тестах, и кажется не должен относится ни к тому ни к другому. Если в изначальном классе такого нет, можно написать экстеншен метод
| { | ||
| center = new Point(0, 0); | ||
| layouter = new CircularCloudLayouter(center); | ||
| imagePath = Path.Combine(TestContext.CurrentContext.TestDirectory, $"{TestContext.CurrentContext.Test.Name}_cloude.png"); |
There was a problem hiding this comment.
nit: Тут и в других местах поправь cloude на cloud
| <PackageReference Include="Microsoft.NETFramework.ReferenceAssemblies.net48" Version="1.0.3"> | ||
| <PrivateAssets>all</PrivateAssets> | ||
| <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets> | ||
| </PackageReference> |
|
|
||
| private bool IsIntersectingWithOtherRectangles(Rectangle rectangle) | ||
| { | ||
|
|
There was a problem hiding this comment.
Приберись в отступах в коде: в начале методов, в конце методов не должно быть пустых строк. Между методами должна быть 1 пустая строка. Да и я не смог вспомнить ситуаций, где две пустые строки подряд выглядели бы норм, их тоже стоит схлопнуть в одну
| for (int j = i + 1; j < rectangles.Length; j++) | ||
| { | ||
|
|
||
| rectangles[i].IntersectsWith(rectangles[j]).Should().BeFalse(rectangles[i] + " " + i + " and " + rectangles[j] + " " + j + " intersect"); |
There was a problem hiding this comment.
Строчка сильно вылазит за один экран, разбей на две
| public double GetTotalAreaOfRectangles(Rectangle[] rectangles) | ||
| { | ||
| int totalAreaOfRectangles = 0; | ||
| for (int i = 0; i < rectangles.Length; i++) |
| return result; | ||
| } | ||
|
|
||
| public void GenerateImage(String pathToSave) |
There was a problem hiding this comment.
Генерация картинок - не зона ответственности CircularCloudLayouter, должен быть другой класс, который умеет рисовать набор прямоугольников, CircularCloudLayouter должен только возвращать их набор.
Соответственно и тесты на генерацию нужно будет утащить в другой файл с тестами
| return new Point(x, y); | ||
| } | ||
|
|
||
| public Rectangle PutNextRectangle(Size rectangleSize) |
There was a problem hiding this comment.
Публичные методы должны располагаться над приватными
cs/tdd.sln.DotSettings
Outdated
There was a problem hiding this comment.
Такие файлы тоже лучше не коммитить, только если есть конвенты на это в команде (и то, скорее настройки конфигурации ide будут где-то лежать в другом командном месте месте)
2 participants
@Yrwlcm