Брайденбихер Виктор Николаевич #238
Conversation
cs/BowlingGame/BowlingGame.csproj
Outdated
| <PackageReference Include="FireSharp" Version="2.0.4" /> | ||
| <PackageReference Include="FluentAssertions" Version="5.10.3" /> | ||
| <PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.7.1" /> | ||
| <PackageReference Include="Newtonsoft.Json" Version="12.0.3" /> | ||
| <PackageReference Include="Newtonsoft.Json" Version="13.0.3" /> | ||
| <PackageReference Include="NUnit" Version="3.12.0" /> | ||
| <PackageReference Include="NUnit.Console" Version="3.11.1" /> | ||
| <PackageReference Include="NUnit3TestAdapter" Version="3.17.0" /> |
There was a problem hiding this comment.
Старайся чтобы в мр попадало только то, что действительно нужно. Скорее всего проект BowlingGame вы рассматривали во время занятия, но к домашке он никак не относится.
| public Rectangle PutNextRectangle(Size rectangleSize) | ||
| { | ||
| if (rectangleSize.Width <= 0 || rectangleSize.Height <= 0) | ||
| throw new ArgumentException("Not valid size should be positive"); |
There was a problem hiding this comment.
Текст ошибки недостаточно понятен, лучше как то переформулировать)
| using TagsCloudVisualization.CloudClasses; | ||
| using TagsCloudVisualization.Visualisation; | ||
|
|
||
| namespace TagsCloudVisualization.Tests |
There was a problem hiding this comment.
Я бы разделил на два проекта: текущий, где будет находится вся логика и проект где будут тесты на логику.
Это сделает код более понятным и удобным - логика остается чистой, а тесты находятся отдельно и не мешают.
There was a problem hiding this comment.
Плюс артефакты из тестов, будут складываться рядом с тестами, а не посреди логики
There was a problem hiding this comment.
Я первоначально так и хотел, но когда я прочитал формулировку задания в репозитории
"Сделай форк этого репозитория. Добавь проект TagsCloudVisualization. Выполняй задания в этом проекте."
Это заставило меня передумать и создать один проект, но я понимаю, что лучше было бы это разделить
There was a problem hiding this comment.
Понял, немного от формулировки задания, можно отходить если считаешь как можно сделать лучше
| if (radiusStep <= 0 || angleStep <= 0) | ||
| throw new ArgumentException("radiusStep and angleStep should be positive"); | ||
|
|
||
| Rectangles = new List<Rectangle>(); |
There was a problem hiding this comment.
Не обязательно выносить создание коллекций в конструктор класса — их можно сразу инициализировать прямо в полях.
| Angle += deltaAngle; | ||
| } | ||
|
|
||
| public Point GetEndPoint => new Point |
| @@ -0,0 +1,63 @@ | |||
| using System.Drawing; | |||
|
|
|||
| namespace TagsCloudVisualization.Visualisation | |||
There was a problem hiding this comment.
Как то набросано всё в папке Visualisation (Visualization) - может получится разделить всё красиво по папкам
| public Bitmap CreateNewBitmap(Size bitmapSize, CircularCloudLayouter layouter, | ||
| Func<IEnumerable<Size>> configurationFunc) | ||
| { | ||
| var newBitmap = new Bitmap(bitmapSize.Width, bitmapSize.Height); | ||
|
|
||
| foreach (var size in configurationFunc()) | ||
| { | ||
| Visualiser.DrawRectangle(newBitmap, layouter.PutNextRectangle(size)); | ||
| } | ||
|
|
||
| return newBitmap; |
There was a problem hiding this comment.
По сути у тебя layouter не используется как layouter. Что я имею в виду:
Ты сделал отдельный класс, который сохраняет состояния (Rectangles), но по факту используется только один метод PutNextRectangles. Получается так, что всё остальное и не нужно. Можно сделать в layouter какой-нибудь метод, который будет возвращать конкретно шаблон облака (в нашем случае - это List) и на основе этого шаблона уже делать конечное изображение.
| [TearDown] | ||
| public void Teardown() | ||
| { | ||
| if (TestContext.CurrentContext.Result.Outcome == ResultState.Failure && _currentBitmap != null) |
There was a problem hiding this comment.
Чтобы не делать проверку на null, можно создавать bitmap прямо в этом методе и не хранить его отдельно
| "radiusStep and angleStep should be positive"); | ||
| } | ||
|
|
||
| [TestCase(5)] |
There was a problem hiding this comment.
Можно заменить просто на Test, раз уж тест всего один
| } | ||
|
|
||
| [TestCase(1, 2, 3)] | ||
| public void CircularCloudLayouter_WhenAddFewButWasBefore_ShouldHaveCorrectCount(int countBefore, int add, int countAfter) |
There was a problem hiding this comment.
Честно скажу не понимаю что значит "WhenAddFewButWasBefore"
|
Для создания изображения добавил возможность использовать состояние CircularCloudLayouter(Rectangles) для создания изображения этот способ я применил в Program. Также оставил и предыдущий способ построения изображения. |
| if (rectangleSize.Width <= 0 || rectangleSize.Height <= 0) | ||
| throw new ArgumentException("The height and width of the Rectangle must be greater than 0"); | ||
|
|
||
| var rectangle = Rectangle.Empty; |
There was a problem hiding this comment.
можно убрать и создавать внутри цикла
|
|
||
| public List<Rectangle> Rectangles => rectangles; | ||
|
|
||
| public CircularCloudLayouter(ISpiralRayMover rayMover) |
There was a problem hiding this comment.
Можно сделать интерфейс IRayMover и наследовать от него будущие RayMover'ы, как SpiralRayMover. И сюда передавать IRayMover, чтобы в случае изменения логики распределения прямоугольников просто подменить реализацию RayMover'a
| catch (Exception e) | ||
| { | ||
| var fileName = $"{TestContext.CurrentContext.Test.MethodName + Guid.NewGuid()}.png"; | ||
|
|
||
| BitmapSaver.SaveToFail(currentBitmap, fileName); | ||
| currentBitmap.Dispose(); | ||
|
|
||
| throw; | ||
| } |
There was a problem hiding this comment.
Эту логику можно прописать в TearDown. Тогда не придется писать одинаковый код на сохранение фейла в каждом тесте. Можно использовать в нем проверку TestContext.CurrentContext.Result.Outcome == ResultState.Failure чтобы понять что тест провалился
There was a problem hiding this comment.
Поправил, использовал создание изображения, достав список добавленных изображений из _layouter.Rectangles.
И избавился от создания изображения внутри тестов.
@ksamnole