Skip to content

Commit

Permalink
Написала классы TagsCloudVisualization и PointsOnSpiral
Browse files Browse the repository at this point in the history
  • Loading branch information
@anastasiakimura
anastasiakimura committed Dec 5, 2023
1 parent cd1a221 commit d4e16ed
Show file tree
Hide file tree
Showing 5 changed files with 80 additions and 0 deletions.
28 changes: 28 additions & 0 deletions cs/TagsCloudVisualization/PointsOnSpiral.cs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
using System.Drawing;

namespace TagsCloudVisualization;

public class PointsOnSpiral

This comment has been minimized.

Sign in to view

Copy link
@pakapik

pakapik Dec 6, 2023

Делай класс запечатанным, если не предполагаешь, что от него будут наследоваться. Вообще, можешь взять за правило делать классы sealed всегда, но убирать только в тех случаях, когда это действительно необходимо.
Это можно даже настроить в настройках среды, чтоб при создании нового класса он был sealed, а при отсутствии ключевого слова показывалась бы подсказка на это дело.

Используя это ключевое слово, ты, во-первых, явно обозначишь, что твой класс имеет законченное поведение, во-вторых, такой код будет чуть быстрее, т.е. вместо виртуальных вызовов callvirt компилятор будет стараться генерировать call.
В первом случае используется полиморфный способ вызова, и CLR в рантайме каждый раз определяет тип, который вызывает метод (потому что вдруг кто-то переопределил поведение?).
Во втором случае, т.к. класс sealed, т.е. точно не имеет никаких иных переопределений, вызов метода будет происходить через команду call - сразу из памяти дернется метод.
{
private Point center;

This comment has been minimized.

Sign in to view

Copy link
@pakapik

pakapik Dec 6, 2023

  1. Если поле должно использоваться только чтения, то это стоит явно обозначить.
  2. Обычно к полям класса добавляют префикс _ . Есть адепты, которые против этого, но тогда при использовании полей в членах класса явно пиши this, чтоб можно было отделить поля от локальных переменных.

public PointsOnSpiral(Point center)
{
this.center = center;

This comment has been minimized.

Sign in to view

Copy link
@pakapik

pakapik Dec 6, 2023

Можно воспользоваться expression body.
}

public IEnumerable<Point> GetPointsOnSpiral()

This comment has been minimized.

Sign in to view

Copy link
@pakapik

pakapik Dec 6, 2023

Класс называется PointsOnSpiral, метод называется GetPointsOnSpiral, а возвращаешь ты перечисление Point'ов. Кажется, где-то закралась логическая ошибка в неймингах. По сути, ведь класс не представляет собой точки на спирали? Он только генерирует их.
{
var radius = 0;

This comment has been minimized.

Sign in to view

Copy link
@pakapik

pakapik Dec 6, 2023 •
edited
Loading

Что будет, если пользователю твоего типа потребуется спираль с шагом радиуса 0.25? А если 0.75? А если он захочет не облако-круг, а торбублик-облако? При этом в 90% случаев всегда будет использоваться облако-круг? Можно посмотреть вот сюда
while (true)
{
for (var i = 0; i < 360; i++)
{
yield return new Point((int)(Math.Cos(2 * Math.PI * i / 360) * radius) + center.X,

This comment has been minimized.

Sign in to view

Copy link
@pakapik

pakapik Dec 6, 2023 •
edited
Loading

  1. 2 * Math.PI * i / 360 вычисляется дважды.
  2. Запись (int)(Math.Cos(2 * Math.PI * i / 360) * radius не имеет смысла. Косинус и синус, какие бы внутри ни были аргументы, имеет область значений [-1;1]. Запусти код и сразу увидишь, в чем проблема.
  3. Math.Cos(2 * Math.PI * i / 360) * radius + смещение - по сути приведение в иную систему координат. Стоит это явным образом подчеркнуть.
  4. Тип Point имеет метод Offset
(int)(Math.Sin(2 * Math.PI * i / 360) * radius) + center.Y);
}

radius++;

This comment has been minimized.

Sign in to view

Copy link
@pakapik

pakapik Dec 6, 2023 •
edited
Loading

Коммент, который надо принять к сведению, но можно не исправлять, т.к. вкупе с комментами выше он потеряет актуальность.

Ты используешь вечный цикл.
Радиус (целочисленный) в вечном цикле инкрементируется.
Кому-то понадобилось огромное не облако , а облачище тегов.
Этот кто-то получает OverflowException.

Учитывая, что у тебя вечный цикл, то гипотетическая возможность переполнения остается - даже если ты будешь использовать uint / long / ulong. BigInteger сюда пихать смысла нет, но есть смысл пометить через summary, что твой код может выбросить исключение.
}
}
}
29 changes: 29 additions & 0 deletions cs/TagsCloudVisualization/Program.cs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
using System.Drawing;
using TagsCloudVisualization;

class CircularCloudLayouter

This comment has been minimized.

Sign in to view

Copy link
@pakapik

pakapik Dec 6, 2023 •
edited
Loading

  1. Модификатор доступа стоит указывать явно.
  2. Можно выделить интерфейс, это может помочь в будущем при тестировании.
  3. Класс должен быть запечатан или нет?
{
private Point cloudCenter;
private List<Rectangle> rectangels = new List<Rectangle>();

This comment has been minimized.

Sign in to view

Copy link
@pakapik

pakapik Dec 6, 2023 •
edited
Loading

  1. Кажется, эти поля не изменяются и используются только для чтения. Стоит это явно указать.
  2. Можно обобщить rectangels до IList или ICollection. Это позволит безболезненно изменить реализацию коллекции, если вдруг появится такая потребность.
  3. rectangels => rectangles
private PointsOnSpiral points;

public CircularCloudLayouter(Point cloudCenter)
{
this.cloudCenter = cloudCenter;
points = new PointsOnSpiral(cloudCenter);
}

public Rectangle PutNextRectangle(Size rectangleSize)
{
foreach (var point in points.GetPointsOnSpiral())

This comment has been minimized.

Sign in to view

Copy link
@pakapik

pakapik Dec 6, 2023

То, о чем я и говорил выше - points.GetPointsOnSpiral() - мы у точек запрашиваем точки.
{
var rectangle = new Rectangle(point, rectangleSize);
if (rectangels.Any(x => x.IntersectsWith(rectangle)))

This comment has been minimized.

Sign in to view

Copy link
@pakapik

pakapik Dec 6, 2023

rectangle почти не отличается rectangels в плане нейминга. Это к вопросу о том, что следует подчеркивать, что ты используешь поле, а не локальную переменную.
continue;
rectangels.Add(rectangle);

This comment has been minimized.

Sign in to view

Copy link
@pakapik

pakapik Dec 6, 2023

Не бойся разделять код пустыми строками.
Например, после

            if (rectangels.Any(x => x.IntersectsWith(rectangle)))
                continue;

так и просится пустая строка.
return rectangle;
}

return new Rectangle(cloudCenter, rectangleSize);

This comment has been minimized.

Sign in to view

Copy link
@pakapik

pakapik Dec 6, 2023

Если тип явно выводится, то можно опускать повторение имени типа.
return new Rectangle(cloudCenter, rectangleSize); => return new(cloudCenter, rectangleSize);

Но это дело вкуса, можешь писать, как тебе нравится.
}
}
10 changes: 10 additions & 0 deletions cs/TagsCloudVisualization/TagsCloudVisualization.csproj
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>net7.0</TargetFramework>
<ImplicitUsings>enable</ImplicitUsings>
<Nullable>enable</Nullable>
</PropertyGroup>

</Project>
7 changes: 7 additions & 0 deletions cs/global.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"sdk": {
"version": "8.0.0",
"rollForward": "latestMajor",
"allowPrerelease": true
}
}
6 changes: 6 additions & 0 deletions cs/tdd.sln
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "BowlingGame", "BowlingGame\
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Samples", "Samples\Samples.csproj", "{B5108E20-2ACF-4ED9-84FE-2A718050FC94}"
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "TagsCloudVisualization", "TagsCloudVisualization\TagsCloudVisualization.csproj", "{21297E12-D769-413A-BDBE-7A1D2022B650}"
EndProject
Global
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Debug|Any CPU = Debug|Any CPU
Expand All @@ -21,6 +23,10 @@ Global
{B5108E20-2ACF-4ED9-84FE-2A718050FC94}.Debug|Any CPU.Build.0 = Debug|Any CPU
{B5108E20-2ACF-4ED9-84FE-2A718050FC94}.Release|Any CPU.ActiveCfg = Release|Any CPU
{B5108E20-2ACF-4ED9-84FE-2A718050FC94}.Release|Any CPU.Build.0 = Release|Any CPU
{21297E12-D769-413A-BDBE-7A1D2022B650}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{21297E12-D769-413A-BDBE-7A1D2022B650}.Debug|Any CPU.Build.0 = Debug|Any CPU
{21297E12-D769-413A-BDBE-7A1D2022B650}.Release|Any CPU.ActiveCfg = Release|Any CPU
{21297E12-D769-413A-BDBE-7A1D2022B650}.Release|Any CPU.Build.0 = Release|Any CPU
EndGlobalSection
GlobalSection(SolutionProperties) = preSolution
HideSolutionNode = FALSE
Expand Down

0 comments on commit d4e16ed

Please sign in to comment.