Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Мажирин Александр #252

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions cs/TagsCloudVisualization/Layouter/CircularCloudLayouter.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
using SkiaSharp;
using TagsCloudVisualization.PositionGenerator;

namespace TagsCloudVisualization.Layouter;

public class CircularCloudLayouter : ICircularCloudLayouter
{
private readonly IPositionGenerator positionGenerator;
private readonly List<SKRect> rectangles;

public CircularCloudLayouter(SKPoint center)
{
rectangles = [];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Да, но так лучше не делать.

  1. IDE младше 2024 это не распознают - плохо
  2. Создаём вроде список, а инициализируем около массив (стилистическая придирка)

Я бы рекомендовал всё-таки явно использовать new List<SKRect>()

positionGenerator = new SpiralLayoutPositionGenerator(center);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А вот здесь уже важно. Это получается жёсткая связь с использование генератора. Такое необходимо пробрасывать через конструктор в виде интерфейса. Блок по DI у вас будет позже, но я бы рекомендовал уже сейчас это посмотреть. Пример статьи про di в целом https://habr.com/ru/companies/hh/articles/783002/

Сейчас достаточно просто вынести интерфейс в конструктор, а при создании класса явно передавать в него new SpiralLayoutPositionGenerator

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Поправлю. Сделал так, потому что по условию в HomeExercise.md сигнатура конструктора - CircularCloudLayouter(Point center).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Написал в ЛС

}

public SKRect PutNextRectangle(SKSize rectangleSize)
{
if (rectangleSize.Width <= 0 || rectangleSize.Height <= 0)
throw new ArgumentException("Rectangle size must be positive", nameof(rectangleSize));

SKRect rectangle;

do
{
var centerOfRectangle = positionGenerator.GetNextPosition();
var rectanglePosition = new SKPoint(centerOfRectangle.X - rectangleSize.Width / 2,
centerOfRectangle.Y - rectangleSize.Height / 2);
rectangle = new SKRect(
rectanglePosition.X,
rectanglePosition.Y,
rectanglePosition.X + rectangleSize.Width,
rectanglePosition.Y + rectangleSize.Height);
} while (rectangles.Any(r => r.IntersectsWith(rectangle)));

rectangles.Add(rectangle);
return rectangle;
}

public SKRect[] GetRectangles()
{
return rectangles.ToArray();
}
}
9 changes: 9 additions & 0 deletions cs/TagsCloudVisualization/Layouter/ICircularCloudLayouter.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
using SkiaSharp;

namespace TagsCloudVisualization.Layouter;

public interface ICircularCloudLayouter
{
SKRect PutNextRectangle(SKSize rectangleSize);
SKRect[] GetRectangles();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
using SkiaSharp;

namespace TagsCloudVisualization.PositionGenerator;

public interface IPositionGenerator
{
public SKPoint GetNextPosition();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

У методов интерфейсов не принято писать модификатор доступа, по умолчанию он всегда public

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
using SkiaSharp;

namespace TagsCloudVisualization.PositionGenerator;

public class SpiralLayoutPositionGenerator(SKPoint center, double step = 0.01) : IPositionGenerator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Интересная конструкция )
Наблюдение: не 2024 райдер такое не поддерживает
Важный момент 1: в данном случае мы не можем добавлять модификатор доступа к этим полям. Очевидно, что center и step должны быть readonly, так как их нельзя изменять по ходу работы. Но здесь они просто компилируются как private. Потенциальное место для ошибки в большом проекте.
Важный момент 2: рекомендуется использовать primary конструктор только в классах, которые не содержать ничего больше в себе. Например в DataClasses. В противном случае высок риск ошибки

{
private double angle;

public SKPoint GetNextPosition()
{
var radius = step * angle;
var x = (float)(center.X + radius * Math.Cos(angle));
var y = (float)(center.Y + radius * Math.Sin(angle));

angle += step;

return new SKPoint(x, y);
}
}
31 changes: 31 additions & 0 deletions cs/TagsCloudVisualization/Program.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
using SkiaSharp;
using TagsCloudVisualization.Layouter;
using TagsCloudVisualization.Renderer;

internal class Program
{
private static void Main()
{
if (!Directory.Exists("results"))
Directory.CreateDirectory("results");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creates all directories and subdirectories in the specified path unless they already exist
Дока

Так что можно упростить код и убрать лишнюю проверку


RenderCloud(GenerateRandomCloud(10), "results/cloud_10.png");
RenderCloud(GenerateRandomCloud(50), "results/cloud_50.png");
RenderCloud(GenerateRandomCloud(100), "results/cloud_100.png");
}

private static SKRect[] GenerateRandomCloud(int count)
{
var layouter = new CircularCloudLayouter(new SKPoint(500, 500));
var rectangleSizes = Enumerable.Range(0, count)
.Select(_ => new SKSize(new Random().Next(10, 100), new Random().Next(10, 100)));
return rectangleSizes.Select(layouter.PutNextRectangle).ToArray();
}

private static void RenderCloud(SKRect[] rectangles, string path)
{
var renderer = new Renderer(new SKSize(1000, 1000));
renderer.CreateRectangles(rectangles);
renderer.CreateImage(path);
}
}
9 changes: 9 additions & 0 deletions cs/TagsCloudVisualization/Renderer/IRenderer.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
using SkiaSharp;

namespace TagsCloudVisualization.Renderer;

public interface IRenderer
{
void CreateRectangles(SKRect[] rectangles);
void CreateImage(string path);
}
48 changes: 48 additions & 0 deletions cs/TagsCloudVisualization/Renderer/Renderer.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
using SkiaSharp;

namespace TagsCloudVisualization.Renderer;

public class Renderer : IRenderer
{
private readonly SKBitmap bitmap;
private readonly SKPaint paint;

public Renderer(SKSize size)
{
bitmap = new SKBitmap((int)size.Width, (int)size.Height);
paint = new SKPaint
{
Color = SKColors.Black,
IsStroke = true,
TextSize = 24
};
using var canvas = new SKCanvas(bitmap);
{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В данном случае фигурные скобки ни на что не влияют. Чтобы задать область для using, требуется писать переменную в скобках using (var canvas = new SKCanvas(bitmap)). Здесь же это просто ещё одна область видимости. Но сейчас using успешно высвобождает ресурсы по выходу из метода, поэтому нет смысла писать конструкции вида using (var a = ...){a.Do()}. Всегда достаточно using var a = ...

canvas.Clear(SKColors.LightGray);
}
}

public void CreateRectangles(SKRect[] rectangles)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Этот метод скорее рисует или добавляет прямоугольники, нежели их создаёт. Create методы зачастую возвращают какой-то объект наружу
  2. Точно необходимо массив прямоугольников, или можно обойтись перечислением?

{
using var canvas = new SKCanvas(bitmap);
foreach (var rectangle in rectangles)
{
if (rectangle.Left < 0 || rectangle.Top < 0 || rectangle.Right > bitmap.Width ||
rectangle.Bottom > bitmap.Height)
throw new ArgumentException("Rectangle is out of bounds");
if (rectangle.Left >= rectangle.Right || rectangle.Top >= rectangle.Bottom)
throw new ArgumentException("Rectangle is invalid");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Давай вынесем в отдельный метод валидации

canvas.DrawRect(rectangle, paint);
paint.Color = new SKColor((byte)(paint.Color.Red + 21), (byte)(paint.Color.Green + 43),
(byte)(paint.Color.Blue + 67));
}
}

public void CreateImage(string path)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Метод сохраняет изображение, а называется create. Сложно без просмотра реализации понять, что здесь происходит

{
using var image = SKImage.FromBitmap(bitmap);
using var data = image.Encode(SKEncodedImageFormat.Png, 100);
using var stream = File.OpenWrite(path);
data.SaveTo(stream);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Получилось так, что мы наделили класс рисователя ещё и правом на сохранение. Согласись, что нарисовать и сохранить - разные зоны ответственности. А если нам понадобится отображать на экране эту картинку? Предлагаю здесь оставить только получение картинки, а сохранять уже при необходимости

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Если развивать этот метод, то можно ещё в параметры задать формат, где значение по умолчанию задать png. Тогда можно будет выбирать, в каком формате сохранять

}
}
Binary file added cs/TagsCloudVisualization/Samples/cloud_10.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added cs/TagsCloudVisualization/Samples/cloud_100.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added cs/TagsCloudVisualization/Samples/cloud_50.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
17 changes: 17 additions & 0 deletions cs/TagsCloudVisualization/TagsCloudVisualization.csproj
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>net8.0</TargetFramework>
<ImplicitUsings>enable</ImplicitUsings>
<Nullable>enable</Nullable>
<GenerateProgramFile>false</GenerateProgramFile>
</PropertyGroup>

<ItemGroup>
<PackageReference Include="FluentAssertions" Version="6.12.2"/>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Точно нужна эта зависимость здесь?

<PackageReference Include="SkiaSharp" Version="2.88.9"/>
<PackageReference Include="SkiaSharp.NativeAssets.Linux.NoDependencies" Version="2.88.9"/>
</ItemGroup>

</Project>
114 changes: 114 additions & 0 deletions cs/TagsCloudVisualizationTests/CircularCloudLayouterTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
using NUnit.Framework.Interfaces;
using TagsCloudVisualization.Layouter;
using TagsCloudVisualization.Renderer;

namespace TagsCloudVisualizationTests;

[TestFixture]
public class CircularCloudLayouterTests
{
[SetUp]
public void SetUp()
{
layouter = new CircularCloudLayouter(new SKPoint(500, 500));
}

[TearDown]
public void TearDown()
{
if (TestContext.CurrentContext.Result.Outcome.Status != TestStatus.Failed) return;

if (!Directory.Exists("tests"))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Как уже писал выше, проверку можно не производить, так как CreateDirectory сделает всё за тебя

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);

var path = Path.Combine(Directory.GetCurrentDirectory(), filename);
Console.WriteLine($"Tag cloud visualization saved to file {path}");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Предлагаю это вынести в отдельный метод сохранения результата раскладки, чтобы не загружать TearDown. Тогда можно будет сделать более очевидную проверку if (failed) SaveImage(). В текущей реализации правильно выделил в if наименьший блок кода

}

private CircularCloudLayouter layouter;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Все поля и свойства принято писать в начале класса, чтобы не приходилось бегать по классу в поисках нужного


[Test]
public void Constructor_ShouldCreateLayouter()
{
layouter.Should().NotBeNull();
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Расскажи, что проверяет этот тест?


[Test]
public void PutNextRectangle_ShouldThrowArgumentException_WhenSizeNotPositive()
{
Action action = () => layouter.PutNextRectangle(new SKSize(0, 100));
action.Should().Throw<ArgumentException>();
}

[Test]
public void PutNextRectangle_ShouldReturnRectangles()
{
var rectangles = new List<SKRect>();
for (var i = 0; i < 10; i++) rectangles.Add(layouter.PutNextRectangle(new SKSize(10, 10)));

rectangles.Should().HaveCount(10);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Но PutNextRectangle возвращает только один прямоугольник. Давай сделаем всё-таки проверку, что он именно возвращает ненулевой прямоугольник, а не в целом какие-то 10

А ещё не нашёл теста, который бы проверял размеры полученного прямоугольника


[Test]
public void PutNextRectangle_ShouldNotIntersectRectangles()
{
var rectangles = new List<SKRect>();
for (var i = 0; i < 10; i++) rectangles.Add(layouter.PutNextRectangle(new SKSize(10, 10)));

for (var i = 0; i < rectangles.Count; i++)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Здесь это не сломается, но лучше делать Count-1, так как идейно мы не можем проверить пересечение последнего с самим собой

for (var j = i + 1; j < rectangles.Count; j++)
rectangles[i].IntersectsWith(rectangles[j]).Should().BeFalse();
}

[Test]
[Repeat(3)]
public void PutNextRectangle_ShouldGenerateDenseLayout()
{
var rectangles = new List<SKRect>();
float totalRectArea = 0;
var random = new Random();
for (var i = 0; i < 100; i++)
{
var size = new SKSize(random.Next(10, 100), random.Next(10, 100));
var rect = layouter.PutNextRectangle(size);
rectangles.Add(rect);
totalRectArea += size.Width * size.Height;
}

var boundingCircleRadius = rectangles.Max(DistanceToCenter);
var boundingCircleArea = Math.PI * Math.Pow(boundingCircleRadius, 2);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Возведение в квадрат лучше делать через умножение. Pow работает быстро для больших степеней, но сравнительно медленно для квадратов
image


var density = totalRectArea / boundingCircleArea;
density.Should().BeGreaterOrEqualTo(0.7f);
}

[Test]
[Repeat(3)]
public void PutNextRectangle_ShouldPlaceRectanglesInCircle()
{
var rectangles = new List<SKRect>();
var random = new Random();
for (var i = 0; i < 100; i++)
{
var size = new SKSize(random.Next(10, 100), random.Next(10, 100));
var rect = layouter.PutNextRectangle(size);
rectangles.Add(rect);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Подобный код встречается во всех тестах в той или иной вариации. Получится его вынести в отдельный метод получения N прямоугольников?


var maxDistanceFromCenter = rectangles.Max(DistanceToCenter);
const int expectedMaxDistance = 500;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

По какому принципу выбиралась эта константа? Такие неочевидные моменты лучше комментировать прямо в коде

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Логика его определения всё равно осталась довольно сложной (через константу плотности и арифметику), но теперь точно стало лучше


maxDistanceFromCenter.Should().BeLessOrEqualTo(expectedMaxDistance);
}

private static float DistanceToCenter(SKRect rect)
{
var center = new SKPoint(500, 500);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А потом в SetUp изменится центр и все тесты упадут )
Предлагаю вынести в константу

var rectCenter = new SKPoint(rect.MidX, rect.MidY);
return SKPoint.Distance(center, rectCenter);
}
}
3 changes: 3 additions & 0 deletions cs/TagsCloudVisualizationTests/GlobalUsings.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
global using NUnit.Framework;
global using FluentAssertions;
global using SkiaSharp;
Comment on lines +1 to +3

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В целом можно, но не рекомендуется. При появлении большего числа файлов имена зависимостей станут повторяться и станет неудобно указывать полные имена классов

58 changes: 58 additions & 0 deletions cs/TagsCloudVisualizationTests/RendererTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
using NUnit.Framework.Interfaces;
using TagsCloudVisualization.Renderer;

namespace TagsCloudVisualizationTests;

[TestFixture]
public class RendererTests
{
[SetUp]
public void SetUp()
{
render = new Renderer(new SKSize(100, 100));
}

[TearDown]
public void TearDown()
{
if (TestContext.CurrentContext.Result.Outcome.Status != TestStatus.Failed) return;

if (!Directory.Exists("tests"))
Directory.CreateDirectory("tests");
var filename = "tests/renderer_" + TestContext.CurrentContext.Test.ID + ".png";
render.CreateImage(filename);
var path = Path.Combine(Directory.GetCurrentDirectory(), filename);
Console.WriteLine($"Attempted to save result to file {path}");


if (File.Exists("image.png"))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Мы в этот блок попадём только в случае провала тестов. А чистить файлы хочется всегда

File.Delete("image.png");
}

private Renderer render;

[Test]
public void CreateImage_ShouldCreateImage()
{
render.CreateImage("image.png");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image.png выглядит как константа. Повторяющиеся строки не стоит копировать

Assert.That(File.Exists("image.png"));
}

[TestCase(0, 200)]
[TestCase(-1, 50)]
public void CreateRectangles_ShouldThrowException_WhenRectanglesAreOutOfBounds(int topLeft, int bottomRight)
{
Assert.Throws<ArgumentException>(() =>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В тестах не стоит смешивать fluent стиль и Assert, даже если это разные файлы. В будущем такое станет сложно поддерживать

render.CreateRectangles([new SKRect(topLeft, topLeft, bottomRight, bottomRight)]));
}

[TestCase(0, 0, 0, 0)]
[TestCase(50, 50, 0, 0)]
[TestCase(50, 0, 50, 0)]
public void CreateRectangles_ShouldThrowException_WhenRectanglesAreInvalid(int topLeft, int topRight,
int bottomLeft, int bottomRight)
{
Assert.Throws<ArgumentException>(() =>
render.CreateRectangles([new SKRect(topLeft, topRight, bottomLeft, bottomRight)]));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Точно правильно расставлены параметры? Предлагаю отказаться от двойных названий и сделать явные, как в конструкторе
image

}
}
Loading