Мажирин Александр #252
Мажирин Александр #252
Conversation
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="FluentAssertions" Version="6.12.2"/> |
There was a problem hiding this comment.
Точно нужна эта зависимость здесь?
|
|
||
| public CircularCloudLayouter(SKPoint center) | ||
| { | ||
| rectangles = []; |
There was a problem hiding this comment.
Да, но так лучше не делать.
- IDE младше 2024 это не распознают - плохо
- Создаём вроде список, а инициализируем около массив (стилистическая придирка)
Я бы рекомендовал всё-таки явно использовать new List<SKRect>()
| public CircularCloudLayouter(SKPoint center) | ||
| { | ||
| rectangles = []; | ||
| positionGenerator = new SpiralLayoutPositionGenerator(center); |
There was a problem hiding this comment.
А вот здесь уже важно. Это получается жёсткая связь с использование генератора. Такое необходимо пробрасывать через конструктор в виде интерфейса. Блок по DI у вас будет позже, но я бы рекомендовал уже сейчас это посмотреть. Пример статьи про di в целом https://habr.com/ru/companies/hh/articles/783002/
Сейчас достаточно просто вынести интерфейс в конструктор, а при создании класса явно передавать в него new SpiralLayoutPositionGenerator
There was a problem hiding this comment.
Поправлю. Сделал так, потому что по условию в HomeExercise.md сигнатура конструктора - CircularCloudLayouter(Point center).
|
|
||
| public interface IPositionGenerator | ||
| { | ||
| public SKPoint GetNextPosition(); |
There was a problem hiding this comment.
У методов интерфейсов не принято писать модификатор доступа, по умолчанию он всегда public
|
|
||
| namespace TagsCloudVisualization.PositionGenerator; | ||
|
|
||
| public class SpiralLayoutPositionGenerator(SKPoint center, double step = 0.01) : IPositionGenerator |
There was a problem hiding this comment.
Интересная конструкция )
Наблюдение: не 2024 райдер такое не поддерживает
Важный момент 1: в данном случае мы не можем добавлять модификатор доступа к этим полям. Очевидно, что center и step должны быть readonly, так как их нельзя изменять по ходу работы. Но здесь они просто компилируются как private. Потенциальное место для ошибки в большом проекте.
Важный момент 2: рекомендуется использовать primary конструктор только в классах, которые не содержать ничего больше в себе. Например в DataClasses. В противном случае высок риск ошибки
| [TestCase(2, 0.019601332f, 0.0039733867f)] | ||
| [TestCase(50, 0.1418311f, -0.47946215f)] | ||
| [TestCase(100, -0.8390715f, -0.5440211f)] | ||
| public void GetNextPosition_ReturnsCorrectPosition_OnSubsequentCalls(int iterations, float x, float y) |
There was a problem hiding this comment.
Лучше x и y назвать expectedX и expectedY
| public void GetNextPosition_ReturnsCorrectPosition_WithNonZeroCenter() | ||
| { | ||
| var center = new SKPoint(10, 10); | ||
| var generator = new SpiralLayoutPositionGenerator(center, 0.1); | ||
|
|
||
| var position = generator.GetNextPosition(); | ||
|
|
||
| position.Should().Be(new SKPoint(10, 10)); | ||
| } |
There was a problem hiding this comment.
Выглядит как копия теста 1. Можно в него добавить просто несколько кейсов
| public void GetNextPosition_ReturnsCorrectPosition_WithNonZeroCenter() | ||
| { | ||
| var center = new SKPoint(10, 10); | ||
| var generator = new SpiralLayoutPositionGenerator(center, 0.1); | ||
|
|
||
| var position = generator.GetNextPosition(); | ||
|
|
||
| position.Should().Be(new SKPoint(10, 10)); | ||
| } |
There was a problem hiding this comment.
Выглядит как копия теста 1. Можно в него добавить просто несколько кейсов
| } | ||
|
|
||
| [Test] | ||
| public void GetNextPosition_ReturnsCorrectPosition_WithDifferentStep() |
There was a problem hiding this comment.
Давай лучше укажем Specified, different не очень понятен
| } | ||
|
|
||
| [Test] | ||
| public void GetNextPosition_ReturnsCorrectPosition_WithNonZeroCenter_OnSusequentCalls() |
| if (TestContext.CurrentContext.Result.Outcome.Status != TestStatus.Failed) return; | ||
|
|
||
| if (!Directory.Exists("tests")) | ||
| Directory.CreateDirectory("tests"); | ||
| var filename = "tests/layouter_" + TestContext.CurrentContext.Test.ID + ".png"; | ||
| var renderer = new Renderer(new SKSize(1000, 1000)); | ||
| renderer.CreateRectangles(layouter.GetRectangles()); | ||
| renderer.CreateImage(filename); | ||
|
|
||
|
|
||
| if (TestContext.CurrentContext.Result.Outcome.Status == TestStatus.Failed) | ||
| { | ||
| var filename = "tests/layouter_" + TestContext.CurrentContext.Test.ID + ".png"; | ||
| SaveImage(filename); | ||
| } |
There was a problem hiding this comment.
if (!a) return
if (a) Foo()
Похоже, логика дублируется
А ещё имя файла можно унести в тот же метод сохранения, назвав как-нибудь SaveTestResultImage()
|
|
||
| [Test] | ||
| public void Constructor_ShouldCreateLayouter() | ||
| private SKSize[] GetRandomSizes(int count) | ||
| { | ||
| layouter.Should().NotBeNull(); | ||
| var random = new Random(); | ||
| var sizes = new SKSize[count]; | ||
| for (var i = 0; i < count; i++) | ||
| { | ||
| var size = new SKSize(random.Next(10, 100), random.Next(10, 100)); | ||
| sizes[i] = size; | ||
| } | ||
| return sizes; |
There was a problem hiding this comment.
Этот метод можно сделать ленивым, добавив yield конструкцию. Учитываю, что везде дальше он именно в таком ключе и используется. Урок
| } | ||
|
|
||
| var maxDistanceFromCenter = rectangles.Max(DistanceToCenter); | ||
| const int expectedMaxDistance = 500; |
There was a problem hiding this comment.
Логика его определения всё равно осталась довольно сложной (через константу плотности и арифметику), но теперь точно стало лучше
| var image = renderer.GetEncodedImage(); | ||
| using var stream = File.OpenWrite(DefaultFileName); | ||
| image.SaveTo(stream); | ||
|
|
||
| Assert.That(File.Exists(DefaultFileName)); |
There was a problem hiding this comment.
Даже если нам вернётся пустое изображение, файл создастся. Давай лучше проверять, что возвращаемый объект не пуст
No description provided.