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

Савельев Григорий #214

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

luvairo-m
Copy link

@luvairo-m luvairo-m commented Dec 4, 2023

Copy link

@MrTimeChip MrTimeChip left a comment

Choose a reason for hiding this comment

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

В целом очень даже неплохо!
За предпроверку ставлю максимум.

public void PutNextRectangle_ShouldNot_SkipRectangles()
{
var sizes = GetRandomLengthSizesArray().ToArray();
_ = sizes.Select(size => layout.PutNextRectangle(size)).ToList();

Choose a reason for hiding this comment

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

Немного странно в такой ситуации усложнять все ради использования LINQ)
Выделяем память под лист, который не используем. В общем, можно проще.

public void PlacedRectangles_ShouldNot_HaveIntersections()
{
var sizes = GetRandomLengthSizesArray();
_ = sizes.Select(size => layout.PutNextRectangle(size)).ToList();

Choose a reason for hiding this comment

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

То же самое

public void Spiral_Should_SaveStateBetweenCalls()
{
var another = new Spiral(distanceDelta, angleDelta);
_ = spiral.GetNextPoint();

Choose a reason for hiding this comment

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

Не совсем понятно, зачем заводит такие переменные. Можно же просто никуда не присваивать.


public static class PointFExtensions
{
public static void ConvertToCartesian(this ref PointF point)

Choose a reason for hiding this comment

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

Зачем здесь использовать ref?

(point.X, point.Y) = (x, y);
}

public static void Center(this ref PointF point, ref PointF center)

Choose a reason for hiding this comment

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

И зачем тут ref?

point.Y += center.Y;
}

public static void ApplyOffset(this ref PointF point, float offsetX, float offsetY)

Choose a reason for hiding this comment

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

А тут зачем?)

Comment on lines 34 to 36
if (Math.Abs(currentRect.X + currentRect.Width / 2 - center.X) < 1e-3
&& Math.Abs(currentRect.Y + currentRect.Height / 2 - center.Y) < 1e-3)
return currentRect;

Choose a reason for hiding this comment

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

А почему бы тогда просто не проверить список прямоугольников?


namespace TagsCloudVisualization;

public class CircularCloudLayout : ILayout

Choose a reason for hiding this comment

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

Смотри в чем есть небольшая архитектурная проблема. Представь что тебе нужно сделать ещё один раскладчик прямоугольников. Допустим, не по кругу, а по диагонали или как угодно ещё.
Тебе нужно будет написать DiagonalCloudLayout, реализовать ILayout и переписать почти весь код кроме одной строчки где ты берешь следующую точку.
Надо отделить логику раскладки и получения следующей точки.

Comment on lines 18 to 19
// Public to enable Unit testing.
public IList<RectangleF> PlacedFigures { get; }

Choose a reason for hiding this comment

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

Немного странно давать доступ к внутренним свойствам просто ради тестов. Хоть это и сделано только ради тестов, никто не мешает воспользоваться этим не по назначению.
Обрати внимание ещё раза на интерфейс, который предлагают реализовать и подумай, как это можно исправить.

Создан класс LayoutVisualizer для сохранения изображения распределения.
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