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

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

Open
wants to merge 25 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.

  • Где csproj? Мне пришлось создавать все самому + подключать все нужные пакеты.
    Изначально не было рабочего проекта.
  • Смотри что говорит тебе intelisense. Очень много мест которые он подсвечивал попали в мои комментарии.
  • Код получился очень запутанным, читать и разбираться очень сложно, а поддерживать его будет еще сложнее,много протечек абстракций, много непонятного нейминга
  • Во всем проекте лейб-мотивом проносится то, что у нас может быть первый прямоугольник

{
private List<Rectangle> rectangles = new();

public void Insert(Rectangle r)

Choose a reason for hiding this comment

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

Что с неймингом

this.storage = storage;
}

public CircleLayer CurrentLayer { get; private set; }

Choose a reason for hiding this comment

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

Отложенная инициализация. Из-за этого

  • словлю NRE
  • получается спагети код

Copy link
Author

Choose a reason for hiding this comment

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

+

Comment on lines 47 to 51
private int SaveRectangle(Point firstLocation, Size rectangleSize)
{
var id = storage.Add(new Rectangle(firstLocation, rectangleSize));
return id;
}

Choose a reason for hiding this comment

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

А как нам этот метод жизнь упрощает?

Copy link
Author

Choose a reason for hiding this comment

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

+

Comment on lines 30 to 41
Point firstRectanglePosition;
var isFirstRectangle = IsFirstRectangle();
if (isFirstRectangle)
{
CreateFirstLayer(rectangleSize);
firstRectanglePosition = PutRectangleToCenter(rectangleSize);
}
else
{
firstRectanglePosition = CurrentLayer.CalculateTopLeftRectangleCornerPosition(rectangleSize);
}

Choose a reason for hiding this comment

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

А зачем мы каждый раз считаем firstRectanglePosition?

Choose a reason for hiding this comment

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

Код очень запутанный и постоянно заставляет переключать контекст. Нам точно нужно кастомно обрабатывать первый прямоугольник?

Много методов - тоже плохо. Мне на каждое действие фактически придется переключаться и смотреть что же в конкретном методе происходит

Copy link
Author

Choose a reason for hiding this comment

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

Первый прямоугольник всё равно по другим правилам помещается на окружность. так что да, но я постаралась это спрятать от других классов


namespace TagsCloudVisualization
{
public class CircularCloudVisualization

Choose a reason for hiding this comment

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

Лучше Visualizer. Т.к у тебя сервис чем-то занимается, а Visualization - это больше про название модельки

Copy link
Author

Choose a reason for hiding this comment

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

+

Pen pen = new Pen(RECTANGLE_COLOR);
for (var i = 0; i < sizes.Length; i++)
{
var r = layouter.PutNextRectangle(sizes[i]);

Choose a reason for hiding this comment

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

нейминг

Copy link
Author

Choose a reason for hiding this comment

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

+

Comment on lines 67 to 80
using (var image = new Bitmap(ImageSize.Width, ImageSize.Height))
{
using (Graphics graphics = Graphics.FromImage(image))
{
graphics.Clear(BACKGROUND_COLOR);
DrawGrid(graphics);
Pen pen = new Pen(RECTANGLE_COLOR);
for (var i = 0; i < sizes.Length; i++)
{
var r = layouter.PutNextRectangle(sizes[i]);
var currentFileName = $"{startName}{i}{extension}";
graphics.DrawRectangle(new Pen(GetColorBySector(layouter.CurrentLayer.CurrentSector)), r);
var filePath = Path.Combine(Path.GetTempPath(), currentFileName);
image.Save(filePath, ImageFormat.Png);

Choose a reason for hiding this comment

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

Дублирование

Copy link
Author

Choose a reason for hiding this comment

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

+


public Rectangle PutRectangleOnCircleWithoutIntersection(int id)
{
var r = storage.GetById(id);

Choose a reason for hiding this comment

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

нейминг

Copy link
Author

Choose a reason for hiding this comment

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

+

Comment on lines 42 to 43
var id = SaveRectangle(firstRectanglePosition, rectangleSize);
var rectangleWithOptimalPosition = OptimiseRectanglePosition(id, isFirstRectangle);

Choose a reason for hiding this comment

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

Зачем мы сохраняем только лишь для того, чтобы в следующем приватном методе же и получить?
Нам точно нужна вообще абстракция хранилки для этой задачи? Может листом обойдемся и не будет вводить доп. абстракции лишь для того, чтобы ввести доп. абстракции

Copy link
Author

Choose a reason for hiding this comment

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

+

Comment on lines 102 to 105
var factorForDistanceByX = moveInfo.Direction == Direction.Left ? -1 :
moveInfo.Direction == Direction.Right ? 1 : 0;
var factorForDistanceByY = moveInfo.Direction == Direction.Top ? -1 :
moveInfo.Direction == Direction.Bottom ? 1 : 0;

Choose a reason for hiding this comment

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

switch

Copy link
Author

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