-
Notifications
You must be signed in to change notification settings - Fork 307
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
base: master
Are you sure you want to change the base?
Мажирин Александр #252
Changes from 7 commits
684cfb8
b6d5899
0427e65
df3fff6
0ba6961
89d210a
51eb51b
6460a80
4eb7c92
79caa7d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 = []; | ||
positionGenerator = new SpiralLayoutPositionGenerator(center); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. А вот здесь уже важно. Это получается жёсткая связь с использование генератора. Такое необходимо пробрасывать через конструктор в виде интерфейса. Блок по DI у вас будет позже, но я бы рекомендовал уже сейчас это посмотреть. Пример статьи про di в целом https://habr.com/ru/companies/hh/articles/783002/ Сейчас достаточно просто вынести интерфейс в конструктор, а при создании класса явно передавать в него new SpiralLayoutPositionGenerator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Поправлю. Сделал так, потому что по условию в HomeExercise.md сигнатура конструктора - CircularCloudLayouter(Point center). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
} | ||
} |
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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Интересная конструкция ) |
||
{ | ||
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); | ||
} | ||
} |
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"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
} |
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); | ||
} |
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); | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. В данном случае фигурные скобки ни на что не влияют. Чтобы задать область для using, требуется писать переменную в скобках |
||
canvas.Clear(SKColors.LightGray); | ||
} | ||
} | ||
|
||
public void CreateRectangles(SKRect[] rectangles) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
{ | ||
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"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Метод сохраняет изображение, а называется |
||
{ | ||
using var image = SKImage.FromBitmap(bitmap); | ||
using var data = image.Encode(SKEncodedImageFormat.Png, 100); | ||
using var stream = File.OpenWrite(path); | ||
data.SaveTo(stream); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Получилось так, что мы наделили класс рисователя ещё и правом на сохранение. Согласись, что нарисовать и сохранить - разные зоны ответственности. А если нам понадобится отображать на экране эту картинку? Предлагаю здесь оставить только получение картинки, а сохранять уже при необходимости There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Если развивать этот метод, то можно ещё в параметры задать формат, где значение по умолчанию задать png. Тогда можно будет выбирать, в каком формате сохранять |
||
} | ||
} |
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"/> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> |
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")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Как уже писал выше, проверку можно не производить, так как |
||
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}"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Предлагаю это вынести в отдельный метод сохранения результата раскладки, чтобы не загружать TearDown. Тогда можно будет сделать более очевидную проверку |
||
} | ||
|
||
private CircularCloudLayouter layouter; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Все поля и свойства принято писать в начале класса, чтобы не приходилось бегать по классу в поисках нужного |
||
|
||
[Test] | ||
public void Constructor_ShouldCreateLayouter() | ||
{ | ||
layouter.Should().NotBeNull(); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Но А ещё не нашёл теста, который бы проверял размеры полученного прямоугольника |
||
|
||
[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++) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Здесь это не сломается, но лучше делать |
||
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
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); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Подобный код встречается во всех тестах в той или иной вариации. Получится его вынести в отдельный метод получения N прямоугольников? |
||
|
||
var maxDistanceFromCenter = rectangles.Max(DistanceToCenter); | ||
const int expectedMaxDistance = 500; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. По какому принципу выбиралась эта константа? Такие неочевидные моменты лучше комментировать прямо в коде There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
} |
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. В целом можно, но не рекомендуется. При появлении большего числа файлов имена зависимостей станут повторяться и станет неудобно указывать полные имена классов |
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")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
Assert.That(File.Exists("image.png")); | ||
} | ||
|
||
[TestCase(0, 200)] | ||
[TestCase(-1, 50)] | ||
public void CreateRectangles_ShouldThrowException_WhenRectanglesAreOutOfBounds(int topLeft, int bottomRight) | ||
{ | ||
Assert.Throws<ArgumentException>(() => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)])); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Да, но так лучше не делать.
Я бы рекомендовал всё-таки явно использовать new
List<SKRect>()