Skip to content

Шагов Владислав#272

Open
ShagovVladislav wants to merge 7 commits into
kontur-courses:masterfrom
ShagovVladislav:hometask
Open

Шагов Владислав#272
ShagovVladislav wants to merge 7 commits into
kontur-courses:masterfrom
ShagovVladislav:hometask

Conversation

@ShagovVladislav

Copy link
Copy Markdown

Comment thread cs/tdd.sln
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Samples", "Samples\Samples.csproj", "{B5108E20-2ACF-4ED9-84FE-2A718050FC94}"
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "TagsCloudVisualization", "TagsCloudVisualization\TagsCloudVisualization.csproj", "{2EDAFD2B-C793-47E2-941E-1B9111073AB7}"

This comment was marked as resolved.

[SetUp]
public void SetUp()
{
generatedRectSizes = [];

This comment was marked as resolved.

var layouter = new CircularCloudLayouter(center);

var rect = layouter.PutNextRectangle(size);
generatedRectSizes.Add(size);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Лишняя строчка, тест без неё работает прекрасно.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Всё ещё работает :)


[SupportedOSPlatform("windows")]

public static class CloudVisualizer

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ответственность этого класса - создать изборажение по заданным параметрам и сохранить в файл. При этом за пользователя очень много чего выбирается по-умолчанию:

  1. расположение центра
  2. размер изображения
  3. цвет фона
  4. цвета прямоугольников
  5. и т.д.

Хорошенько подумай, что можно захотеть настраивать в изображении!

Далее - недавно вы, кажется, проходили fluent interface'ы, помнишь такие? Давай применим новые знания и навыки: во-первых, давай создадим класс-билдер для CloudVisualizer - пусть билдер хранит разнообразные настройки и для каждой имеет fluent-метод для их изменения (например, метод SetCenter мог бы принимать параметры настраиваемого центра); во-вторых, пусть билдер имеет метод Build или Create (или что-то в этом духе), который будет создавать экземпляр CloudVisualizer с указанными настройками. Подумай, что должен делать билдер, если пользователь определил не все настройки, но вызывает метод Build? Spoiler: здесь могут быть разные решения разной сложности.

namespace TagsCloudVisualization;

[SupportedOSPlatform("windows")]

This comment was marked as duplicate.

foreach (var size in sizes)
layouter.PutNextRectangle(size);

layouter.PlacedRectangles.Count.Should().Be(sizes.Count);

This comment was marked as resolved.

var file = Path.Combine(outputDir, "many.png");

var sizes = Enumerable
.Range(1, 200)

This comment was marked as resolved.

}

[Test, Explicit]
public void VizualizeExamples()

This comment was marked as resolved.

}

[Test]
public void PutNextRectangle_ShouldNotCrossCenter()

This comment was marked as resolved.

public void PutNextRectangle_ShouldThrow_OnNegativeSize()
{
var layouter = new CircularCloudLayouter(new Point(0, 0));
var size = new Size(-10, 20);

This comment was marked as resolved.

{
while (true)
{
var point = spiral.GetNextPoint();

This comment was marked as resolved.

{
for (var dy = -radiusCells; dy <= radiusCells && yielded < MaxTouchNeighbors; dy++)
{
if (!grid.TryGetValue((cx + dx, cy + dy), out var list)) continue;

This comment was marked as resolved.


public sealed class CircularCloudLayouter
{
private readonly Point center;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Не критично, но я бы тебе предложил структурировать поляшки примерно вот так:
image

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Константны всё равно потом примешал в конец

public CircularCloudLayouter(Point center)
{
this.center = center;
centerX = center.X;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

А зачем отдельные поля для X и Y?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

в том плане что можно Point сделать?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

У тебя и center и X,Y имеют модификатор readonly, т.е. ты их не перепресваиваешь. Почему вместо X не использовать center.X?

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

This comment was marked as resolved.

var cx = centerX / CellSize;
var cy = centerY / CellSize;

for (var dx = -radiusCells; dx <= radiusCells && yielded < MaxTouchNeighbors; dx++)

This comment was marked as resolved.

}
}

if (yielded >= MaxTouchNeighbors) yield break;

This comment was marked as resolved.

if (yielded < MaxTouchNeighbors && placedRectangles.Count > 0)
{
var last = placedRectangles[^1];
foreach (var r in GetRectanglesAround(last, maxCells: 4))

This comment was marked as resolved.


private IEnumerable<Rectangle> GetRectanglesAround(Rectangle r, int maxCells)
{
var x1 = Math.Max((r.Left / CellSize) - maxCells, -1000000);

This comment was marked as resolved.

This comment was marked as resolved.


for (var x = x1; x <= x2; x++)
for (var y = y1; y <= y2; y++)
if (grid.TryGetValue((x, y), out var list))

This comment was marked as resolved.

for (var x = x1; x <= x2; x++)
for (var y = y1; y <= y2; y++)
if (grid.TryGetValue((x, y), out var list))
foreach (var rr in list)

This comment was marked as resolved.

return Math.Abs(bestDist - double.MaxValue) < 0.00000001 ? FindBySpiralFallback(size) : best;
}

private void AddInHashAndCandidates(Rectangle c)

This comment was marked as resolved.

return div;
}

private static int HashRect(Rectangle r)

This comment was marked as resolved.

}
}

private double DistanceToCenterInt(Rectangle r)

This comment was marked as resolved.

{
for (var i = 0; i < 20000; i++)
{
var p = spiral.GetNextPoint();

This comment was marked as resolved.


private Rectangle FindBySpiralFallback(Size size)
{
for (var i = 0; i < 20000; i++)

This comment was marked as resolved.

return r;
}

return new Rectangle(centerX + 50000, centerY + 50000, size.Width, size.Height);

This comment was marked as resolved.


private double DistanceToCenterInt(Rectangle r)
{
var dx = r.X + (r.Width >> 1) - centerX;

This comment was marked as resolved.


private static IEnumerable<(int, int)> GetCells(Rectangle r)
{
var x1 = FloorDiv(r.Left, CellSize);

This comment was marked as resolved.

cellBuffer.AddRange(list);
}

return cellBuffer.Any(t => rect.IntersectsWith(t));

This comment was marked as resolved.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

скорость сильно падает
на 500 прямоугольниках 150мс против 250мс

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Сомнительно - IL код один и тот же должен получаться. Проверишь в качестве факультатива?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

image

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ок, признаю, был не прав :) Похоже действительно делегат под лямбду компилятор один раз создаёт, а вот под метод групп конвершн - на каждой итерации.

}
}

private bool IntersectsGrid(Rectangle rect)

This comment was marked as resolved.

var dx = rect.X + (rect.Width >> 1) < centerX ? -1 : 1;
var dy = rect.Y + (rect.Height >> 1) < centerY ? -1 : 1;

var leftRight = rect with { X = rect.X - dx };

This comment was marked as resolved.

while (moved && iter++ < PullMaxIterations)
{
moved = false;
var dx = rect.X + (rect.Width >> 1) < centerX ? -1 : 1;

This comment was marked as resolved.

{
if (dx == 0 && dy == 0) return rect;

for (var i = 0; i < 1000; i++)

This comment was marked as resolved.

rect = shifted;
}

for (var i = 0; i < 3000; i++)

This comment was marked as resolved.

// left
yield return new Rectangle(
o.Left - size.Width,
o.Top + ((o.Height - size.Height) >> 1),

This comment was marked as resolved.


if (candidatesBuffer.Count > MaxCandidatesToEvaluate)
{
candidatesBuffer.Sort((a, b) => DistanceToCenterInt(a).CompareTo(DistanceToCenterInt(b)));

This comment was marked as resolved.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

candidatesBuffer.Count - MaxCandidatesToEvaluate - как это лаконично обозвать?


if (candidatesBuffer.Count == 0)
{
foreach (var c in GetSpiralCandidates(size, 40))

This comment was marked as resolved.

Color? rectangleBorderColor = null,
string? outputDirectory = null)
{
try

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Сейчас внутри try точно можно поимать ранее пойманные (и залоггированные!) исключения - оберни в try только то, что нужно.

throw new ArgumentException("File name cannot be null or empty", nameof(fileName));

var extension = Path.GetExtension(fileName);
if (string.IsNullOrEmpty(extension))

This comment was marked as resolved.

nameof(fileName));
}

if (string.IsNullOrWhiteSpace(outputDirectory))

This comment was marked as resolved.

var fullPath = Path.GetFullPath(outputDirectory);
Console.WriteLine($"Output directory resolved to: {fullPath}");
}
catch (Exception ex)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

А зачем перевыбрасывать-то его? И вообще - у тебя снаружи есть try-catch. Если ты здесь записал, то прерви выполнение, или наверху оставь большой try-catch и перехватывай всё там. Строго говоря, директория-то может быть синтаксически правильной, а ошибка связана с, например, правами доступа.

Pen? pen = null,
int padding = 50)
{
try

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Во, опять огромный внутренний try-catch - давай в одном месте наверху ошибки записывать и прекращать выполнение, а тут не пытаться всё обернуть?

}

[Test]
public void PutNextRectangle_ShouldBeFast_For500Rectangles()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Периодически падает на этих тестах :) Но там рядом с границей, да

{
var sizes = new List<Size> { new(10, 10) };

var act = () =>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Не переноси ради одного символа)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants