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

Рушкова Ольга2 #261

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

Conversation

SomEnaMeforme
Copy link

No description provided.

Copy link

@HELLoWorlD01100 HELLoWorlD01100 left a comment

Choose a reason for hiding this comment

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

  • Почему тесты находятся не в отдельном проекте?

{
public class RectangleStorageTests
{
private RectangleStorage storage;

Choose a reason for hiding this comment

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

Класса не существует. Т.е фактически сломанный файл с тестами. Проект даже не сбилдится.

{
internal class CloudCompressor
{
private readonly BruteForceNearestFinder nearestFinder = new ();

Choose a reason for hiding this comment

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

BruteForceNearestFinder - статический класс, не может быть полем.

Choose a reason for hiding this comment

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

Соответственно проект не соберется даже

Comment on lines 53 to 57
var testObj = TestContext.CurrentContext.Test.Parent?.Fixture as CircularCloudLayouterTests;
var info = typeof(CircularCloudLayouterTests)
.GetField("storage", BindingFlags.NonPublic | BindingFlags.Instance);
var st = info?.GetValue(testObj);

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, [])
{ }

internal CircularCloudLayouter(Point center, List<Rectangle> storage)

Choose a reason for hiding this comment

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

Протечка абстракций

private readonly List<Rectangle> storage;
private readonly CloudCompressor compressor;

public CircleLayer CurrentLayer { get; }

Choose a reason for hiding this comment

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

Почему свойство?

Comment on lines 20 to 23
private void Expand()
{
Radius += DeltaRadius;
}

Choose a reason for hiding this comment

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

Лучше не выносить. Чтобы лишний раз по коду не бегать

Direction.Left => (possibleNearest, rectangleForFind) => rectangleForFind.Left - possibleNearest.Right,
Direction.Right => (possibleNearest, rectangleForFind) => possibleNearest.Left - rectangleForFind.Right,
Direction.Top => (possibleNearest, rectangleForFind) => rectangleForFind.Top - possibleNearest.Bottom,
_ => (possibleNearest, rectangleForFind) => possibleNearest.Top - rectangleForFind.Bottom,

Choose a reason for hiding this comment

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

лучше все-таки явно енам тут обрабатывать, чтобы спецэффектов не получить, если я енам расширю

return nearestByDirection.Count > 0 ? nearestByDirection.MinBy(el => el.Distance).Nearest : null;
}

public static Func<Rectangle, Rectangle, int> GetMinDistanceCalculatorBy(Direction direction)

Choose a reason for hiding this comment

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

Чет пересуложненное возвращаемое значение. Почему мы не можем сразу давать результат по дирекшну?

Comment on lines 12 to 65
public CloudCompressor(Point compressTo, List<Rectangle> cloud)
{
compressionPoint = compressTo;
this.cloud = cloud;
}

public Rectangle CompressCloudAfterInsertion(Rectangle insertionRectangle)
{
var toCompressionPoint = GetDirectionsForMovingForCompress(insertionRectangle);
foreach (var direction in toCompressionPoint)
{
insertionRectangle.Location = CalculateRectangleLocationAfterCompress(insertionRectangle, direction);
}
return insertionRectangle;
}

private Point CalculateRectangleLocationAfterCompress(Rectangle forMoving, Direction toCenter)
{
var nearest = nearestFinder.FindNearestByDirection(forMoving, toCenter, cloud);
if (nearest == null) return forMoving.Location;
var distanceCalculator = nearestFinder.GetMinDistanceCalculatorBy(toCenter);
var distanceForMove = distanceCalculator(nearest.Value, forMoving);
return MoveByDirection(forMoving.Location, distanceForMove, toCenter);
}

private static Point MoveByDirection(Point forMoving, int distance, Direction whereMoving)
{
var factorForDistanceByX = whereMoving switch
{
Direction.Left => -1,
Direction.Right => 1,
_ => 0
};
var factorForDistanceByY = whereMoving switch
{
Direction.Top => -1,
Direction.Bottom => 1,
_ => 0
};
forMoving.X += distance * factorForDistanceByX;
forMoving.Y += distance * factorForDistanceByY;

return forMoving;
}

private List<Direction> GetDirectionsForMovingForCompress(Rectangle forMoving)
{
var directions = new List<Direction>();
if (forMoving.Bottom < compressionPoint.Y) directions.Add(Direction.Bottom);
if (forMoving.Left > compressionPoint.X) directions.Add(Direction.Left);
if (forMoving.Right < compressionPoint.X) directions.Add(Direction.Right);
if (forMoving.Top > compressionPoint.Y) directions.Add(Direction.Top);
return directions;
}

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.

Я бы предложил двигать максимально по вектору, как достаточно простое и похожее решение.

Comment on lines 23 to 43
public void CreateImage(string? filePath = null, bool withSaveSteps = false)
{
var rectangles = rectangleStorage.ToArray();

using var image = new Bitmap(imageSize.Width, imageSize.Height);
using var graphics = Graphics.FromImage(image);
graphics.Clear(backgroundColor);
graphics.DrawGrid();
var pen = new Pen(rectangleColor);

for (var i = 0; i < rectangles.Length; i++)
{
var nextRectangle = rectangles[i];
graphics.DrawRectangle(pen, nextRectangle);
if (withSaveSteps)
{
SaveImage(image, filePath);
}
}
SaveImage(image, filePath);
}

Choose a reason for hiding this comment

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

Логику по рисованию и сохранению лучше разделить по SRP.
Но если не просто принципами кидаться, а раскрыть, то:

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

Comment on lines +33 to +36
for (var i = 0; i < rectangles.Length; i++)
{
var nextRectangle = rectangles[i];
graphics.DrawRectangle(pen, nextRectangle);

Choose a reason for hiding this comment

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

Прямоугольники рисуются 1к1 к размеру изображения. Из-за этого получим, что при достаточно большом количестве прямоугольников визуализация поломается

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