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

Ренат Зубакин #222

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

Conversation

SolarHunger
Copy link

@gisinka
Copy link

gisinka commented Dec 5, 2023

Все комменты обсуждаемы - можем обсуждать или в тредах, или в личке в телеге
За количество комментов не парься - это нормально, процесс обучения, повышения скилла, ревью именно для этого и нужно

На пофикшенные комментарии можно ставить реакции или писать пометочки, что сделал и почему

cs/TagsCloudVisualization/CircularCloudLayouter.cs Outdated Show resolved Hide resolved
cs/TagsCloudVisualization/CircularCloudLayouter.cs Outdated Show resolved Hide resolved
cs/TagsCloudVisualization/CircularCloudLayouter.cs Outdated Show resolved Hide resolved
cs/TagsCloudVisualization/CircularCloudLayouter.cs Outdated Show resolved Hide resolved
}

[Test]
public void PutNextRectangleFirstRectanglePositionEqualCenter()
Copy link

Choose a reason for hiding this comment

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

PutNextRectangle_ShouldPlaceFirstOnCenter или что-нибудь такое, чтобы читаемость была лучше

cs/TagsCloudVisualization/CircularCloudLayouter.cs Outdated Show resolved Hide resolved
cs/TagsCloudVisualization/CircularCloudLayouter.cs Outdated Show resolved Hide resolved
cs/TagsCloudVisualization/CircularCloudLayouter.cs Outdated Show resolved Hide resolved
{
public class SpiralDistribution : IDistribution
{
public Point Center { get; private set; }

This comment was marked as resolved.


public SpiralDistribution(Point center)
{
Center = center;

This comment was marked as resolved.

Copy link

Choose a reason for hiding this comment

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

throw new ArgumentException(); должен жить на отдельной строчке

Copy link

Choose a reason for hiding this comment

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

Считаю, что конструктор должен быть максимально простым, никаких проверок внутри не должно быть, его задача - инициализировать объект, если хочешь сделать проверку на входные параметры, то лучше сделать статичный метод который это будет проверять, и потом сам вызовет тебе приватный конструктор,
Ну и декомпозиция, все дела
Да и тесты так как то приятнее писать когда вызываешь какой нибудь метод Create вместо new

Как вариант валидацию выносить в отдельный приватный метод

[Test]
public void CircularCloudLayouter_Initialize_Params()
{
Assert.AreEqual(0, tagsCloud.WordPositions.Count);
Copy link

Choose a reason for hiding this comment

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

FluentAssertions давай использовать

Copy link

@gisinka gisinka Dec 12, 2023

Choose a reason for hiding this comment

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

Еще несколько всяких Assert.Throws

}


private static IEnumerable<TestCaseData> PutNextRectangleIncorrectArguments()
Copy link

Choose a reason for hiding this comment

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

Тестовые данные лучше выносить в отдельные файлы ЧетоТамTestData.cs

public void SetUp()
{
drawer = new CloudLayouterDrawer(10);
fileName = "testcreation.png";
Copy link

Choose a reason for hiding this comment

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

Если строчка не меняется, почему бы ее в константу не унести?

Copy link

Choose a reason for hiding this comment

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

Красивое, а чего поля кривые?

Copy link
Author

@SolarHunger SolarHunger Dec 11, 2023

Choose a reason for hiding this comment

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

я нагенерил через цикл с параметрами, что высота может быть больше чем ширина, поэтому их так и покривило

Copy link

Choose a reason for hiding this comment

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

Странное поведение

@gisinka
Copy link

gisinka commented Dec 10, 2023

Давай придерживаться общепринятого: тред в ревью резолвит тот, кто его создал

@@ -0,0 +1,81 @@
using System;

This comment was marked as resolved.

}


public bool CheckIntersection(Rectangle currRectangle)

This comment was marked as resolved.

{
private Random random;
public int margin;
public int imageWidth;

This comment was marked as resolved.


public CloudLayouterDrawer(int margin)
{
this.margin= margin;

This comment was marked as resolved.

}


public Rectangle ComperessRectangle(Rectangle rectangle)

This comment was marked as resolved.


var newRectanglesPositions = GetNewRectanglesPositions(rectangles);

using (var bitmap = new Bitmap(imageWidth, imageHeight))
Copy link

@gisinka gisinka Dec 10, 2023

Choose a reason for hiding this comment

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

можно упростить до using var bitmap = new Bitmap(imageWidth, imageHeight)

Copy link
Author

Choose a reason for hiding this comment

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

ругает версию шарпа

Copy link

Choose a reason for hiding this comment

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

Поднять - никто не запрещал)

public int imageWidth;
public int imageHeight;

public CloudLayouterDrawer(int margin)
Copy link

Choose a reason for hiding this comment

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

Цвет фона можно сделать параметром рисовальщика

var brush = new SolidBrush(color);
graphics.FillRectangle(brush, rectangle);
}

Copy link

Choose a reason for hiding this comment

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

Сохранение в файл отлично декомпозируется в отдельный метод

}

[Test]
public void CloudLayouterDrawer_IsCreate_Imgae()
Copy link

Choose a reason for hiding this comment

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

Очепятка

private SpiralDistribution spiralDistribution;

[SetUp]
public void SetUp()

This comment was marked as resolved.

using NUnit.Framework;
using NUnit.Framework.Interfaces;

namespace TagsCloudVisualization

This comment was marked as resolved.


namespace TagsCloudVisualization
{
[TestFixture]

This comment was marked as resolved.

namespace TagsCloudVisualization.TagsCloudVisualizationTests
{
[TestFixture]
public class SpiralDistributionTests
Copy link

@gisinka gisinka Dec 10, 2023

Choose a reason for hiding this comment

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

Не хватает тестов на уникальность точек, увеличение радиуса

}


private CircularCloudLayouter getRandomTagsCloud()
Copy link

Choose a reason for hiding this comment

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

Функции должны называться с больших буков

{
var distanceToCenter =
Math.Sqrt(Math.Pow(tag.X - tagsCloud.Center.X, 2) + Math.Pow(tag.Y - tagsCloud.Center.Y, 2));
return distanceToCenter <= GetCircilarCloudLayouterRadius(randomTagsCloud);
Copy link

Choose a reason for hiding this comment

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

Раскладка в форме капли тоже отработает в это условие, не?

{
public class CloudLayouterDrawer
{
private readonly Random random;
Copy link

@gisinka gisinka Dec 12, 2023

Choose a reason for hiding this comment

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

В одном файле поля и свойства до конструктора, в другом после, как-то уж определись)

using System.IO;
using System.Linq;

namespace TagsCloudVisualization
Copy link

Choose a reason for hiding this comment

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

Нет абстракции - какого-то интерфейса


namespace TagsCloudVisualization
{
public class CircularCloudLayouter
Copy link

Choose a reason for hiding this comment

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

И тут нет интерфейса

drawer = new CloudLayouterDrawer(10);
var projectDirectory = Directory.GetParent(Environment.CurrentDirectory).Parent.FullName;
testFilePath = Path.Combine(projectDirectory, "images", fileName);
if (File.Exists(testFilePath)) File.Delete(testFilePath);
Copy link

Choose a reason for hiding this comment

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

Перенос протерян

}

[Test]
public void SprialDistribution_Initialize__Custom_Params()
Copy link

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