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

Трофимов Никита #227

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

Conversation

WoeromProg
Copy link

Comment on lines 1 to 10
using System.Drawing;
using System.Drawing.Imaging;
using FluentAssertions;
using NUnit.Framework;

namespace TagsCloudVisualization
{

[TestFixture]
public class Spiral_Should

Choose a reason for hiding this comment

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

Тесты обычно выносятся в отдельный проект. Это делается хотя бы для того, чтобы при релизе приложения не тащить с собой лишний код

Comment on lines 59 to 62
public List<Rectangle> Rectangles()
{
return _rectangles;
}

Choose a reason for hiding this comment

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

Сомнительно. Этот метод был добавлен только для того, чтобы использовать его в тестах, так обычно не делают. А получилось так потому что вся логика работы выполнена в одном классе, хотя следовало бы разделить на разные.
public List<Rectangle> Rectangles() => _rectangles;

if(rectangle.IsIntersectOthersRectangles(_rectangles))
break;
}
MoveRectangleToCenter(ref rectangle);

Choose a reason for hiding this comment

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

Очень сомнительно использование передачи по ссылке.... Так обычно не пишут из-за непредсказуемых мутаций, которые могут произойти внутри метода, логичнее было бы возвращать новый прямоугольник

private Bitmap _bitmap;
private readonly Size shiftToBitmapCenter;
private readonly List<Rectangle> _rectangles;
public DrawCloud(List<Rectangle> rectangles, int width, int height)

Choose a reason for hiding this comment

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

Не уверен, что хорошее решение, задавать ширину и высоту в конструкторе еще может быть окей, а вот сами прямоугольники я бы вынес в метод CreateImage

Comment on lines 19 to 28
List<Rectangle> GenerateRandomRectangles(CircularCloudLayouter layouter, int count)
{
var rectangles = new List<Rectangle>();
var random = new Random(1);
for (var i = 0; i < count; i++)
{
var size = new Size(random.Next(40,100), random.Next(20, 60));
rectangles.Add(layouter.PutNextRectangle(size));
}
return rectangles;

Choose a reason for hiding this comment

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

Метод внутри метода? Лучше в отдельный класс


namespace TagsCloudVisualization;

public class Spiral

Choose a reason for hiding this comment

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

Непонятное название класса

@69raccoon96
Copy link

Ну и само tdd как таковое отсутствует, все в одном коммите

Comment on lines 5 to 7
public class GenerateRandomRectangles
{
public List<Rectangle> RectangleGenerator(CircularCloudLayouter layouter, int count)

Choose a reason for hiding this comment

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

Имя классы и имя метода местами нужно поменять

_step = step;
}

public Point NextPoint()

Choose a reason for hiding this comment

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

GetNextPoint

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