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

Конина Анастасия #229

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cs/TagCloudVisualization.Tests/GlobalUsings.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
global using NUnit.Framework;
25 changes: 25 additions & 0 deletions cs/TagCloudVisualization.Tests/TagCloudVisualization.Tests.csproj
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<Project Sdk="Microsoft.NET.Sdk">

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

<IsPackable>false</IsPackable>
<IsTestProject>true</IsTestProject>
</PropertyGroup>

<ItemGroup>
<PackageReference Include="FluentAssertions" Version="7.0.0-alpha.3" />
Copy link

Choose a reason for hiding this comment

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

Старайся использовать надежные версии, без всяких альфа/бета и прочих наворотов.

<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.6.0"/>
<PackageReference Include="NUnit" Version="3.12.0"/>
<PackageReference Include="NUnit3TestAdapter" Version="4.2.1"/>
<PackageReference Include="NUnit.Analyzers" Version="3.6.1"/>
<PackageReference Include="coverlet.collector" Version="6.0.0"/>
</ItemGroup>

<ItemGroup>
<ProjectReference Include="..\TagsCloudVisualization\TagsCloudVisualization.csproj" />
</ItemGroup>

</Project>
67 changes: 67 additions & 0 deletions cs/TagCloudVisualization.Tests/UnitTest1.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
namespace TagCloudVisualization.Tests;
Copy link

Choose a reason for hiding this comment

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

Имя файла обычно совпадает с именем класса. Сейчас имя файла UnitTest1

using System;
using System.Collections.Generic;
using System.Drawing;
using FluentAssertions;
using System.Linq;
using NUnit.Framework;
using NUnit.Framework.Interfaces;
using TagsCloudVisualization;

[TestFixture]
Copy link

Choose a reason for hiding this comment

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

В TestFixture можно добавить TestOf

public class CircularCloudLayouter_Should
Copy link

Choose a reason for hiding this comment

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

Не встречался с таким неймингом, для меня он странный, но если в дереве тестов это выглядит нормально, то бога ради.

{
private CircularCloudLayouter cloud;
private Point center;
private List<Rectangle> rectangles;

[SetUp]
public void Setup()
Copy link

Choose a reason for hiding this comment

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

SetUp

{
center = new Point(250, 250);
cloud = new CircularCloudLayouter(center);
rectangles = new List<Rectangle>();
}

[TearDown]
public void TearDown()
{
if (TestContext.CurrentContext.Result.Outcome.Status != TestStatus.Failed)
return;

var path = TestContext.CurrentContext.TestDirectory + "\\" + TestContext.CurrentContext.Test.Name + ".bmp";
Copy link

Choose a reason for hiding this comment

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

CloudDrawer.DrawAndSaveCloud(rectangles.ToArray(), path);
TestContext.WriteLine("Tag cloud visualization saved to file: " + path);
}

[Test]
public void PutNextRectangle_NoIntersects_AfterPutting()
Copy link

Choose a reason for hiding this comment

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

Нигде нет структуры AAA

{
var rectangleSize = new Size(10, 50);

rectangles = new List<Rectangle>();
for (var i = 0; i < 100; i++)
Copy link

Choose a reason for hiding this comment

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

Достаточно сложный тест сам по себе, хотя от этого и никуда не деться вроде как. Тем не менее, его можно чуть улучшить.
Для начала ограничение первого цикла можно вынести в параметр через TestCase, указывая кол-во прямоугольников, которое будет генериться. У нас сразу появится возможность быстро добавить кейсов для иного кол-во прямоугольников.
Во-вторых, rectangles.Where(x => x != rectangle) вызывает подозрение. Если у нас первый прямоугольник не пересекается с вторым, то нужно ли убеждаться, что второй не пересекается с первым? Вопрос уже к тебе непосредственно.

{
rectangles.Add(cloud.PutNextRectangle(rectangleSize));
Copy link

Choose a reason for hiding this comment

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

Все прямоугольники тут одинакового размера. Т.е. твое облако тегов в таком случае так же должно состоять из слов одинакового размера, что является пограничным случаем. К тому же у тебя все прямоугольники в виде столбцов var rectangleSize = new Size(10, 50);, а не строк, а алгоритм раскладки не отдает предпочтения горизонтальному расположению.

Использовать Random можно, но возникает вероятность создать тест, который будет то падать, то проходить. Лучше подумать над тем, как создать источник данных, который бы не менялся при запусках тестов, но при этом мог создавать различные прямоугольники. Но это всё в идеале, конечно же.

}

foreach (var rectangle in rectangles)
{
foreach (var secondRectangle in rectangles.Where(x => x != rectangle))
{
rectangle.IntersectsWith(secondRectangle).Should().BeFalse();
}
}
}

[Test]
public void PutNextRectangle_OnCenter_AfterFirstPut()
Copy link

Choose a reason for hiding this comment

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

Не вижу смысла заводить SetUp и TearDown ради двух тестов.

{
var rectangleSize = new Size(100, 50);
var expectedRectangle = new Rectangle(center, rectangleSize);
var rectangle = cloud.PutNextRectangle(rectangleSize);
rectangles.Add(rectangle);
rectangle.Should().BeEquivalentTo(expectedRectangle);
}

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.

Думаю, стоит добавить тестов на генератор точек. Например, я могу написать var pointGenerator = new SpiralPointsGenerator(center, -10000); Что случится с твоим кодом? В теста ответа нет.

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
17 changes: 17 additions & 0 deletions cs/TagsCloudVisualization/CloudDrawer.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
using System.Drawing;
using System.Collections.Generic;
using System.Drawing.Imaging;

namespace TagsCloudVisualization;

public class CloudDrawer
Copy link

Choose a reason for hiding this comment

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

  1. Кажется, класс тоже можно пометить статическим.
  2. Ты слишком сильно привязалась к cloud. Разве твой класс и метод занимаются только тем, что рисуют облака?
    Во-первых, смотря на картинки, я не вижу никакого облака. Вижу прямоугольники загнанные в окружность.
    Во-вторых, если я подам на вход пачку прямоугольников, которые к твоему заданию не имеют никакого отношения, то я тоже получу облако 🙃?
  3. Не заметил, чтоб этот функционал использовался где-то, кроме тестов.

{
public static void DrawAndSaveCloud(Rectangle[] rectangles, string fileName)
{
var bitmap = new Bitmap(500, 500);
Copy link

Choose a reason for hiding this comment

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

Эти штуки должны реализовывать интерфейс IDisposable. Значит, к ним применим оператор using

Copy link

Choose a reason for hiding this comment

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

Не очень хорошо, что битмап хардкодится по ширине и длине.

var gr = Graphics.FromImage(bitmap);
gr.FillRectangle(Brushes.Black, 0, 0, 500, 500);
Copy link

Choose a reason for hiding this comment

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

Кажется, если сделать не кисть, а карандаш, то твои прямоугольники буду полые, тогда на них будет проще смотреть. Если карандаш не поможет, то всё равно стоит поискать пути сделать только контуры прямоугольников для наглядности.

gr.FillRectangles(Brushes.White, rectangles);
bitmap.Save(fileName);
Copy link

Choose a reason for hiding this comment

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

Что будет, если вывалится IOException?

}
}
8 changes: 8 additions & 0 deletions cs/TagsCloudVisualization/IPointsGenerator.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
using System.Drawing;

namespace TagsCloudVisualization;

public interface IPointsGenerator
{
IEnumerable<Point> GetPoints();
}
8 changes: 8 additions & 0 deletions cs/TagsCloudVisualization/ITagCloudLayouter.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
using System.Drawing;

namespace TagsCloudVisualization;

public interface ITagCloudLayouter
{
Rectangle PutNextRectangle(Size rectangleSize);
}
36 changes: 36 additions & 0 deletions cs/TagsCloudVisualization/Program.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
using System.Drawing;
using System.Collections.Generic;
using TagsCloudVisualization;

public class CircularCloudLayouter : ITagCloudLayouter
Copy link

@pakapik pakapik Dec 12, 2023

Choose a reason for hiding this comment

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

К вопросу о методе Main добавляется вопрос, почему этот файл называется Program?

{
private readonly Point cloudCenter;
private readonly IList<Rectangle> _rectangels = new List<Rectangle>();
Copy link

@pakapik pakapik Dec 12, 2023

Choose a reason for hiding this comment

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

  1. Снова о нейминге полей.
    Нужно использовать единый подход. Либо для всех полей по всем проекте использовать префикс _, либо без него, но при использовании полей использовать this.
    Мешать эти подходы не нужно.

  2. Не замечание, но у нас в проекте принято выравнивать поля лесенкой. Тогда было вот так, что, на мой взгляд, симпатичнее. Но это чистой воды вкусовщина.

private readonly Point _cloudCenter;
private readonly SpiralPointsGenerator _spiralPointsGenerator;
private readonly IList<Rectangle> _rectangels = new List<Rectangle>();

private readonly SpiralPointsGenerator spiralPointsGenerator;


public CircularCloudLayouter(Point cloudCenter)
{
this.cloudCenter = cloudCenter;
spiralPointsGenerator = new SpiralPointsGenerator(cloudCenter);
Copy link

Choose a reason for hiding this comment

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

= new SpiralPointsGenerator(cloudCenter); делает твой класс жестко связанным с SpiralPointsGenerator. Как правило, подобного рода зависимости должны инжектиться, т.е. внедряться через конструктор.

Почитай про принцип инверсии зависимостей, там немного, но полезно и интересно.

Ну а если совсем коротко, то твой генератор реализует интерфейс IPointsGenerator, его и нужно использовать в полях и конструкторе.

}

public Rectangle PutNextRectangle(Size rectangleSize)
{
foreach (var point in spiralPointsGenerator.GetPoints())
{
var rectangle = new Rectangle(point, rectangleSize);
if (_rectangels.Any(x => x.IntersectsWith(rectangle)))
Copy link

Choose a reason for hiding this comment

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

Переделывать не надо, но есть ощущение (смотря на твои изображения), что генератор не отдает предпочтения горизонтальному расположение прямоугольников - хотя большая часть слов располагаться будет все же горизонтально.

continue;

_rectangels.Add(rectangle);
return rectangle;
Copy link

Choose a reason for hiding this comment

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

Перед return принято оставлять пустую строку.

}

return new Rectangle(cloudCenter, rectangleSize);
}

static void Main(string[] args)
Copy link

Choose a reason for hiding this comment

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

  1. Зачем здесь нужен метод Main?
  2. Всегда и везде указывай модификатор доступа. Кроме членов интерфейса - там не надо, сейчас у тебя с этим всё как надо.
  3. Для подобных заглушек необязательно переносить строки, можно было бы написать
    private static void Main(string[] args) {}.

{
}
}
8 changes: 8 additions & 0 deletions cs/TagsCloudVisualization/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
100 квадратов со случайными размерами (максимальный размер - 70)
![TagCloud](100(randomButLesser70)Rectangles.png )

40 квадратов со случайными размерами (максимальный размер - 50)
![TagCloud](40(randomButLesser50)Rectangles.png )

100 квадратов со сторонами 10, 50
![TagCloud](100(10,50)Rectangles.png )
Copy link

Choose a reason for hiding this comment

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

Хотелось бы сразу увидеть большее кол-во - 500 / 1000, например.

35 changes: 35 additions & 0 deletions cs/TagsCloudVisualization/SpiralPointsGenerator.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
using System.Drawing;

namespace TagsCloudVisualization;

public sealed class SpiralPointsGenerator : IPointsGenerator
{
private readonly Point center;
private readonly double radiusStep;

public SpiralPointsGenerator(Point center, double radiusStep = 1)
{
this.center = center;
this.radiusStep = radiusStep;
}

///<exception cref="System.OverflowExeption">
Copy link

Choose a reason for hiding this comment

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

Почти, но не совсем. Описание метода должно быть отдельно - в summary. Исключение отдельно - в <exception>. А вот <exception cref="System.OverflowException">сюда уже можно добавить, почему может вываливаться исключение</exception>

/// Long point generation operation
/// </exception>
public IEnumerable<Point> GetPoints()
{
double radius = 0;
Copy link

Choose a reason for hiding this comment

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

Можно использовать var, но тип при этом останется double. Нужно особым (но простым) способом записать присваиваемое значение.

var term = radiusStep / 360;
Copy link

Choose a reason for hiding this comment

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

Что такое term?

while (true)
{
for (var pointNumber = 0; pointNumber < 360; pointNumber++)
{
var pointAngle = 2 * Math.PI * pointNumber / 360;
Copy link

Choose a reason for hiding this comment

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

var pointAngle = 2 * Math.PI * pointNumber / 360; все ещё вычисляется много раз. Можно чуть ускорить, если вынести постоянную часть за рамки цикла.

var currentPoint = new Point(center.X, center.Y);
Copy link

Choose a reason for hiding this comment

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

  1. Кажется, логичнее сделать наоборот.
    currentPoint проинициализировать со значениями (int)(Math.Cos(pointAngle) * radius), (int)(Math.Sin(pointAngle) * radius), а затем сделать смещение, используя center.

  2. Запись (int)(Math.Cos(pointAngle) * radius), (int)(Math.Sin(pointAngle) * radius) не очень приятно читать. Я уже просил это декомпозировать. Как минимум стоит сделать вот так:

var x = (int)(Math.Cos(pointAngle) * radius);
var y = (int)(Math.Sin(pointAngle) * radius);

Как максимум вынести это в отдельный метод, дав ему название того, что он действительно делает - переводит из полярной системы координат в декартову.

currentPoint.Offset((int)(Math.Cos(pointAngle) * radius), (int)(Math.Sin(pointAngle) * radius));
yield return currentPoint;
Copy link

Choose a reason for hiding this comment

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

Стоит добавить отступов, чтоб ясно видеть yield return

radius += term;
}
}
}
}
19 changes: 19 additions & 0 deletions cs/TagsCloudVisualization/TagsCloudVisualization.csproj
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<Project Sdk="Microsoft.NET.Sdk">

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

<ItemGroup>
<PackageReference Include="FluentAssertions" Version="6.12.0" />
Copy link

Choose a reason for hiding this comment

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

Мы с тобой вынесли тесты в отдельную папку и отдельный проект. Следовательно, из этого проекта нужно убрать ненужные зависимости.

<PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.7.1" />
<PackageReference Include="NUnit" Version="3.12.0" />
<PackageReference Include="NUnit3TestAdapter" Version="3.17.0" />
<PackageReference Include="System.Drawing.Common" Version="8.0.0" />
</ItemGroup>

</Project>
7 changes: 7 additions & 0 deletions cs/global.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"sdk": {
"version": "8.0.0",
"rollForward": "latestMajor",
"allowPrerelease": true
}
}
12 changes: 12 additions & 0 deletions cs/tdd.sln
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ 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
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "TagCloudVisualization.Tests", "TagCloudVisualization.Tests\TagCloudVisualization.Tests.csproj", "{FCBA6AEA-59FC-4A6B-817E-5FCE66279D38}"
EndProject
Global
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Debug|Any CPU = Debug|Any CPU
Expand All @@ -21,6 +25,14 @@ 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
{FCBA6AEA-59FC-4A6B-817E-5FCE66279D38}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{FCBA6AEA-59FC-4A6B-817E-5FCE66279D38}.Debug|Any CPU.Build.0 = Debug|Any CPU
{FCBA6AEA-59FC-4A6B-817E-5FCE66279D38}.Release|Any CPU.ActiveCfg = Release|Any CPU
{FCBA6AEA-59FC-4A6B-817E-5FCE66279D38}.Release|Any CPU.Build.0 = Release|Any CPU
EndGlobalSection
GlobalSection(SolutionProperties) = preSolution
HideSolutionNode = FALSE
Expand Down