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

Юдин Павел #217

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

Юдин Павел #217

wants to merge 15 commits into from

Conversation

PavelUd
Copy link

@PavelUd PavelUd commented Dec 4, 2023

@PavelUd PavelUd marked this pull request as draft December 4, 2023 18:02
@PavelUd PavelUd marked this pull request as ready for review December 4, 2023 18:02
Copy link

@razor2651 razor2651 left a comment

Choose a reason for hiding this comment

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

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

вот это плохая декомпозиция на коммиты
image
а если мне надо добавить 100 файлов то это будет 100 коммитов?
коммиты должны отображать какое то логическое изменение в коде
например реализовал <фичанейм>

cs/tagsCloud/tagsCloud.csproj Outdated Show resolved Hide resolved
cs/tagsCloud/Spiral.cs Outdated Show resolved Hide resolved
cs/tagsCloud/Spiral.cs Outdated Show resolved Hide resolved
x += xOffset;
y += yOffset;
var point = new Point((int)Math.Round(x), (int)Math.Round(y));
center = point;

Choose a reason for hiding this comment

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

не очень понимаю, зачем тебе центр смещать каждый раз? тебе же достаточно каждый раз вычислять только сдвиг

Copy link
Author

@PavelUd PavelUd Dec 5, 2023

Choose a reason for hiding this comment

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

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

private const int MaxCoordinate = 5000;


public static readonly Func<Color> GetRandomColor =

Choose a reason for hiding this comment

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

объясни, зачем тебе тут везде Func?

Copy link
Author

@PavelUd PavelUd Dec 5, 2023

Choose a reason for hiding this comment

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

По-моему, лямбда-выражения лаконичнее и читабельнее, чем просто методы. И когда в теле один только return, очень хочется записать всё под "стрелочную" функцию.


<ItemGroup>
<PackageReference Include="FluentAssertions" Version="7.0.0-alpha.3" />
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.6.0"/>

Choose a reason for hiding this comment

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

лишние ссылки стоит убирать, они сильно захламляют артефакты, помимо того что в ide тебе могут предложить использовать не те сущности через intellisense

Copy link
Author

Choose a reason for hiding this comment

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

если я это уберу, то тесты не запускаются

cs/TagsCloudTests/TagsCloudTests.csproj Outdated Show resolved Hide resolved
cs/TagsCloudTests/TagsCloudTests.csproj Outdated Show resolved Hide resolved
cs/TagsCloudTests/TagsCloudTests.csproj Outdated Show resolved Hide resolved
cs/tagsCloud/tagsCloud.csproj Outdated Show resolved Hide resolved
cs/tagsCloud/Utils.cs Outdated Show resolved Hide resolved
cs/tagsCloud/RectanglesVisualizer.cs Outdated Show resolved Hide resolved
cs/tagsCloud/CloudLayouterExtension.cs Show resolved Hide resolved
cs/tagsCloud/CircularCloudLayouter.cs Outdated Show resolved Hide resolved
cs/tagsCloud/RectanglesVisualizer.cs Outdated Show resolved Hide resolved
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