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

Чечулин Антон #211

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

Conversation

fearppen
Copy link

@fearppen fearppen commented Dec 3, 2023

Copy link

@Pasha0666 Pasha0666 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 CircularCloudLayouterTests
{
private Point center = new(250, 250);
private CircularCloudLayouter layouter;

Choose a reason for hiding this comment

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

Обычно объект который тестируют называют sut

Comment on lines 3 to 18
namespace TagsCloudVisualization
{
public class Spiral
{
private readonly Point center;
private readonly double deltaAngle;
private readonly double deltaRadius;
private double radius;
private double angle;

public Spiral(Point center, double deltaAngle, double deltaRadius)
{
this.center = center;
this.deltaAngle = deltaAngle;
this.deltaRadius = deltaRadius;
}

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.

В целом для этого класса можно добавить интерфейс, т.к. в целом у тебя мб несколько вариантов генерации точек

Choose a reason for hiding this comment

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

Я бы попробовал еще точки генерировать лениво через yield return, и посмотрел что из этого получиться

Comment on lines 20 to 26
public static void SaveBitmap(Bitmap bitmap, string fileName, string pathToDirectory)
{
if (!Directory.Exists(pathToDirectory))
{
Directory.CreateDirectory(pathToDirectory);
}

Choose a reason for hiding this comment

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

Чисто технически путь может быть не правильным, допустим иметь не корректные символы в названии

Comment on lines 37 to 45
var rectangleLocation = GetLeftUpperCornerFromRectangleCenter(CloudCenter, rectangleSize);
var rectangle = new Rectangle(rectangleLocation, rectangleSize);

while (rectanglesInLayout.Any(rect => rect.IntersectsWith(rectangle)))
{
var pointOnSpiral = spiral.GetNextPointOnSpiral();
rectangleLocation = GetLeftUpperCornerFromRectangleCenter(pointOnSpiral, rectangleSize);
rectangle = new Rectangle(rectangleLocation, rectangleSize);
}

Choose a reason for hiding this comment

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

Возможно с do while код будет более читаемый

Choose a reason for hiding this comment

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

Кажется лишний файл проскачил, т.к. тест проходит, а файл остался в репе

this.deltaRadius = deltaRadius;
}

public IEnumerable<Point> GetPointsOnSpiral()
Copy link
Author

Choose a reason for hiding this comment

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

Хотелось бы спросить, какие преимущества имеет такой подход с использованием IEnumerable перед подходом с вычислением каждой следующей точки отдельно

Choose a reason for hiding this comment

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

В основном тем что тебе чтобы сгенерировать несколько пиралей не нужно будет пересоздовать класс, а исполькользать просто метод


namespace TagsCloudVisualization
{
public static class Visualizer
Copy link
Author

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.

Я не вижу особо идей честно его протестировать, но можно застабать и посмотреть что метод вызывется с правильными аргументами

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