Брозовский Максим #250
Брозовский Максим #250
Conversation
| private readonly double angleOffset; | ||
| private readonly double radius; | ||
|
|
||
| public DefaultPointsDistributor(double radius, double angleOffset) |
There was a problem hiding this comment.
Разве Default? По-моему расположение происходит по спирали.
|
|
||
| namespace TagsCloudVisualization; | ||
|
|
||
| public class DefaultPointsDistributor : IPointsDistributor |
There was a problem hiding this comment.
А куда он их распределяет?) Больше похоже на генератор
| while (true) | ||
| { | ||
| yield return GetPointByPolarCords(start, angle); | ||
| angle += angleOffset; | ||
| } |
There was a problem hiding this comment.
А как мне контролировать количество точек? Мне не очень нравится, что если я побегу форичем, у меня цикл никогда не кончится, при этом это будет скрыто в реализации.
А че если я предварительно хочу Linq вызвать? Типа Count(), Single(), Last()
| return rectangle; | ||
| } | ||
|
|
||
| private static Rectangle CreateRectangleWithCenterAndSize(Point center, Size rectangleSize) => |
There was a problem hiding this comment.
А сильно непонятнее станет, если будет просто CreateRectangle?
| } | ||
|
|
||
| [Test] | ||
| [Repeat(15)] |
| [SetUp] | ||
| public void Setup() | ||
| { | ||
| randomizer = new Random(); |
There was a problem hiding this comment.
Так, а если у нас будут тесты, в которых не нужен рандом, все равно будем создавать?) Например, с одинаковыми прямоугольниками, с квадратами и тд
| randomizer = new Random(); | ||
| } | ||
|
|
||
| [Test] |
There was a problem hiding this comment.
Нет тестов на разные параметры спирали, на разные кейсы прямоугольников и тд
|
|
||
| namespace TagsCloudVisualization; | ||
|
|
||
| public class SpiralCloudLayouter : ICloudLayouter |
There was a problem hiding this comment.
А реализовать надо было CircularCloudLayouter :)
| while (true) | ||
| { | ||
| pointEnumerator.MoveNext(); | ||
| var rectanglePosition = pointEnumerator.Current; | ||
| rectangle = CreateRectangleWithCenterAndSize(rectanglePosition, rectangleSize); | ||
|
|
||
| if (!rectangles.Any(rectangle.IntersectsWith)) break; | ||
| } |
There was a problem hiding this comment.
А какой профит тогда было возвращать IEnumerable, если по итогу мы каждую точку через Enumerator проходим?
| private readonly IEnumerator<Point> pointEnumerator; | ||
| private readonly List<Rectangle> rectangles = new List<Rectangle>(); | ||
|
|
||
| public SpiralCloudLayouter(Point center, double radius, double angleOffset) |
There was a problem hiding this comment.
При достаточно большом радиусе получается спираль из прямоугольников. В рамках SpiralCloudLayouter норм, но в задаче все-таки CircularCloudLayouter :)
There was a problem hiding this comment.
Я до сих пор могу задавать значения и у меня будут разбросаны на картинке прямоугольники
| <PackageReference Include="SkiaSharp" Version="3.118.0-preview.1.2" /> | ||
| <PackageReference Include="SkiaSharp.Views.WindowsForms" Version="3.118.0-preview.1.2" /> |
There was a problem hiding this comment.
А для чего превью-версии? Почему стабильную не можем использовать?
Превью версии это фактически тестовый нюгет пакет, там могут быть уязвимости, итоговый пакет может отличаться абстракциями и тд
There was a problem hiding this comment.
не смог задаунгрейдить, понял, в будущем будут инсталить стабильные стараться.
Вот логи:
@ Installing SkiaSharp in TagsCloudVisualization finished (1,803 sec)
[Notification][Install] Install failed (project: TagsCloudVisualization, package: SkiaSharp v2.88.9)
Package restore failed. Rolling back package changes for 'TagsCloudVisualization'.
Package 'OpenTK 3.1.0' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v ... grade: SkiaSharp from 3.118.0-preview.1.2 to 2.88.9. Reference the package directly from the project to select a different version.
TagsCloudVisualization -> SkiaSharp.Views.WindowsForms 3.118.0-preview.1.2 -> SkiaSharp (>= 3.118.0-preview.1.2)
TagsCloudVisualization -> SkiaSharp (>= 2.88.9)
There was a problem hiding this comment.
ааааа, короче там трабла там на ласт стабильной dotnet 6.0, такие дела
|
|
||
| namespace TagsCloudVisualization; | ||
|
|
||
| public class CircularCloudLayouter(Point center, IPointsGenerator pointsGenerator) : ICloudLayouter |
There was a problem hiding this comment.
Ага, то есть я могу сюда засунуть, например, рандомный генератор точек. Тогда круг из прямоугольников у нас не получится, как заявлено в названии
| public Point Center => center; | ||
| public IEnumerable<Rectangle> Rectangles => rectangles; |
There was a problem hiding this comment.
А зачем нам это делать свойствами? Пусть полями внутри лежат. А то я могу взять и поменять координату, там же публичный сеттер у точки
There was a problem hiding this comment.
Point - Struct, а значит это не проблема (он immutable), подробнее тут - https://stackoverflow.com/questions/3456554/points-properties-in-net
(как и SKPoint). Мне они нужны конкретно в тестах, потому что удобно понимать где центр и доставать прямоугольники для отрисовки случаев когда тесты падают. Эти свойства никак инкапсуляцию не нарушают, они readonly getters.
| } | ||
| } | ||
|
|
||
| public static Rectangle CreateRectangle(Point center, Size rectangleSize) => |
There was a problem hiding this comment.
Я бы тут явно обозначил в названии, что мы не просто создаем прямоугольник, а создаем его с центром в точке center
| public IEnumerable<Rectangle> Rectangles => rectangles; | ||
|
|
||
| public CircularCloudLayouter(Point center) : | ||
| this(center, new SpiralPointsGenerator(center, 1d, 0.5d)) |
| private readonly SKBitmap bitmap; | ||
| private readonly SKCanvas canvas; | ||
|
|
||
| public Visualizer(int width, int height) | ||
| { | ||
| bitmap = new SKBitmap(width, height); | ||
| canvas = new SKCanvas(bitmap); | ||
| } |
There was a problem hiding this comment.
Ну смотри. Из-за того, что у тебя этот контекст расшарен, а задавать можно разные наборы прямоугольников, я смог добиться вот такого поведения. Код твоих абстракций я не изменял, если что. Нам правда нужно это в поля выносить?
| canvas = new SKCanvas(bitmap); | ||
| } | ||
|
|
||
| public SKBitmap VisualizeTagCloud(IEnumerable<Rectangle> rectangles) |
There was a problem hiding this comment.
Давай тогда везде использовать абстракции из SkiaSharp. А то странно же получается, что где-то System.Drawing, а где то SkiaSharp) Интерфейс изначальный давай тоже мб поменяем?
|
|
||
| namespace TagsCloudVisualization; | ||
|
|
||
| public class Visualizer |
cs/TagsCloudVisualization/Saver.cs
Outdated
|
|
||
| namespace TagsCloudVisualization; | ||
|
|
||
| public class Saver(string imageDirectory) |
cs/TagsCloudVisualization/Program.cs
Outdated
| var bitmap = visualizer.VisualizeTagCloud(rectangles); | ||
|
|
||
| var saver = new Saver(ImageDirectory); | ||
| saver.SaveAsPng(bitmap, $"{NumberOfRectangles}_TagCloud.png"); |
There was a problem hiding this comment.
Говорим, что сохраним в пнг, но неявно требуем, чтобы в пути тоже было пнг. Можем упростим как-то?
| var rect = circularCloudLayouter.PutNextRectangle(rectSize); | ||
|
|
||
| rect.Should().BeOfType<Rectangle>(); |
There was a problem hiding this comment.
убрал, смысл был проверить тип, что уже и так делает интерфейс
| var currentContext = TestContext.CurrentContext; | ||
| if (currentContext.Result.Outcome.Status != TestStatus.Failed) return; | ||
|
|
||
| var layoutSize = GetLayoutSize(circularCloudLayouter.Rectangles.ToList()); | ||
| var visualizer = new Visualizer(layoutSize.Width, layoutSize.Height); | ||
| var bitmap = visualizer.VisualizeTagCloud(circularCloudLayouter.Rectangles); | ||
|
|
||
| var saver = new Saver(ImagesDirectory); | ||
| var filename = $"{currentContext.Test.Name}.png"; | ||
| saver.SaveAsPng(bitmap, filename); | ||
|
|
||
| TestContext.Out.WriteLine($"Tag cloud visualization saved to file {Path.Combine(ImagesDirectory, filename)}"); |
There was a problem hiding this comment.
TearDown : System.Exception : Unable to allocate pixels for the bitmap.
There was a problem hiding this comment.
Странно у меня все тесты рисует...
| public void PutNextRectangle_ShouldReturnRectangleAtCenter_WhenFirstInvoked() | ||
| { | ||
| var rectSize = randomizer.NextSize(1, int.MaxValue); | ||
|
|
||
| var actualRect = circularCloudLayouter.PutNextRectangle(rectSize); | ||
| var expectedRect = CircularCloudLayouter.CreateRectangle(circularCloudLayouter.Center, rectSize); | ||
|
|
||
| actualRect.Should().BeEquivalentTo(expectedRect); | ||
| } |
There was a problem hiding this comment.
Почему мы тестируем один метод через то, что он вызывает внутри себя? Если в этом методе будет баг, то мы не отловим в тестах этого
| [Test] | ||
| public void PutNextRectangle_ShouldReturnRectangle_WithCorrectSize() | ||
| { | ||
| var recSize = randomizer.NextSize(1, int.MaxValue); |
| public class CircularCloudLayouterTest | ||
| { | ||
| private const string ImagesDirectory = "../../../failedTests"; | ||
| private readonly Random randomizer = new(); |
There was a problem hiding this comment.
Опять же, рандом не потокобезопасный. Если появятся асинхронные тесты, то придется отлавливать отдельно
| circularCloudLayouter = TestContext.CurrentContext.Test.Name.Contains("Optimal") ? | ||
| CreateCircularCloudLayouterWithOptimalParams() : CreateCircularCloudLayouterWithRandomParams(); |
There was a problem hiding this comment.
Вносим неявную конвенцию через ненадежный источник - имя теста. Так делать не стоит
There was a problem hiding this comment.
Я хотел Category поюзать, но стало лень)
| rectangles.Any(fr => rectangles.Any(sr => fr != sr && fr.IntersectsWith(sr))) | ||
| .Should().BeFalse(); |
There was a problem hiding this comment.
Оч запутано, прям вчитываться приходится. Можем упростить?
| canvas = new SKCanvas(bitmap); | ||
| } | ||
|
|
||
| public SKBitmap VisualizeTagCloud(IEnumerable<Rectangle> rectangles) |
There was a problem hiding this comment.
При достаточно больших размерах прямоугольников все начинает ломаться
There was a problem hiding this comment.
Не уверен в чем тут проблема..... вроде размеры не очень большие типа
There was a problem hiding this comment.
не понял что тут надо сделать короче
No description provided.