Волков Кирилл #216

caimanchik · 2023-12-04T16:28:23Z

desolver · 2023-12-05T15:19:48Z

cs/TagsCloudVisualization/CircularCloudLayouter.cs

+    public Rectangle SetName(Size rectangleSize)
+    {
+        if (isNameSet)
+            throw new InvalidOperationException("Name can be set once");
+
+        isNameSet = true;
+        return AddRectangle(rectangleSize);
+    }


Не очень понимаю назначение этого метода

Он называется SetName, и видя такое название, я подразумеваю, что я должен дать чему-то имя и передать какую-то строку. Но нет, тут я передаю координаты, которые точно так же мог передать и в обычный PutNextRectangle. Тогда вопрос - а зачем вообще этот метод?

Если он используется как обязательная входная точка алгоритма, тогда возникает другой вопрос. Представь - ты в первый раз видишь эту библиотеку и хочешь воспользоваться этим классом, по каким критериям ты поймешь, что SetName обязательно должен быть первым? Не забывай думать о пользователях твоего кода и о том, что чем проще интерфейс, тем лучше

desolver · 2023-12-05T15:22:26Z

cs/TagsCloudVisualization/CircularCloudLayouter.cs

+        var leftOffset = size.Width / 2 + (size.Width % 2 == 0 ? 0 : 1);
+        var topOffset = size.Height / 2 + (size.Height % 2 == 0 ? 0 : 1);


Два действия, похожие с точностью до параметра size....

desolver · 2023-12-05T15:24:44Z

cs/TagsCloudVisualization/CircularCloudLayouter.cs

+            return rectangle;
+        }
+
+        return default;


Точно ли нужно возвращать default? Ведь вызывающий код может быть совсем не готов к такому повороту событий, он вызывал PutNextRectangle и передал конкретные размеры, а мы ему почему-то вернем прямоугольник нулевой длины

desolver · 2023-12-05T15:28:14Z

cs/TagsCloudVisualization/Layout.cs

+        }
+    }
+
+    public bool CanPutInCoords(Rectangle rectangle)


Как считаешь, нет ли проблем с производительностью в этом классе?
Насколько оптимально мерить пересечения прямоугольников по конкретным точкам, из которых они состоят, если есть возможность сравнивать более широкими "мазками" (при том что в тестах алгоритм нахождения пересечения гораздо оптимальнее)?

desolver · 2023-12-05T15:29:08Z

cs/TagsCloudVisualization/Program.cs

@@ -0,0 +1,3 @@
+// See https://aka.ms/new-console-template for more information
+
+Console.WriteLine("Hello, World!");


Прикольно, если покажешь пример взаимодействия со своим кодом в этом месте

desolver · 2023-12-05T15:52:16Z

cs/TagsCloudVisualization/Tests/CircularCloudLayouter_Should.cs

+               || IsPointInRectangle(new Point(r1.Left, r1.Bottom), r2);
+    }
+
+    private static bool IsPointInRectangle(Point p, Rectangle r)


Как и это, если нужно будет

desolver · 2023-12-05T15:59:29Z

cs/TagsCloudVisualization/CircularCloudLayouter.cs

+
+    public Point Center => layout.Center;
+
+    public Rectangle[] GetRectangles => rectangles.ToArray();


К этому свойству у меня сразу несколько вопросов:

Точно ли оно нужно? Как и rectangles выше. Представь, что мы размещаем пару миллионов прямоугольников, помимо того, что мы должны потратить ресурсы на вычисление их положения, так ещё и должны зачем-то сохранить их внутри класса. А вдруг никто даже не вызовет это свойство снаружи? Тогда мы впустую хранили пару миллионов объектов в памяти

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

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

С глаголами в начале мы называем методы, а это свойство

desolver · 2023-12-05T16:00:10Z

cs/TagsCloudVisualization/CircularCloudLayouter.cs

@@ -0,0 +1,59 @@
+using System.Drawing;
+
+namespace TagsCloudVisualization;


Круто, что file-scoped фичу сразу заюзал 🔥

desolver · 2023-12-05T16:01:00Z

cs/TagsCloudVisualization/Tests/CircularCloudLayouter_Should.cs

+
+namespace TagsCloudVisualization.Tests;
+
+public class CircularCloudLayouter_Should


Понимаю, что это не входило в рамки задания, но вкину немного продакнш практик. Тесты и основной код обычно делятся на два отдельных проекта - основной код, который раскатается потом на прод, и тесты - которые отработают на этапе тестирования и всё.

Так сборка основного кода не получит лишнюю нагрузку в виде компиляции и рестора зависимостей тестов и будет чуть быстрее собираться, что может значительно повлиять на CI/CD пайплайн

К чему я это - давай и тут сделаем так же)

После разделения проверь пакеты - чтобы ничего лишнего не попало в тот или иной проект

desolver · 2023-12-05T16:01:17Z

cs/TagsCloudVisualization/Layout.cs

+
+namespace TagsCloudVisualization;
+
+public class Layout


А где же тесты на этот класс?)

… Layout.cs

desolver · 2023-12-07T17:17:07Z

cs/TagsCloudVisualization/CircularCloudLayouter.cs

+    public Rectangle PutNextRectangle(Size rectangleSize)
+    {
+        return AddRectangle(rectangleSize);
+    }


Получился метод, который просто проксирует вызов до другого метода, в таком случае AddRectangle просто не нужен, и можно всю его внутрянку втащить в основной PutNextRectangle

desolver · 2023-12-07T17:18:04Z

cs/TagsCloudVisualization/CircularCloudLayouter.cs

+            size.Width / 2 + (size.Width % 2 == 0 ? 0 : 1),
+            size.Height / 2 + (size.Height % 2 == 0 ? 0 : 1));


Все ещё строчки, похожие друг на друга, как близнецы. В таких случаях общая логика выносится в метод и вызывается дважды с разными параметрами

desolver · 2023-12-07T17:20:48Z

cs/TagsCloudVisualization/CircularCloudLayouter.cs

+            break;
+        }
+
+        return result;


Логика с возвращение дефолтного значения никуда не исчезла. Писал о ней тут #216 (comment)

Ситуация, что мы не смогли найти место для прямоугольника на плоскости - исключительная, в 99,9% случаев все будет работать правильно. Давай попросту кинем тут ошибку, тогда result можно будет занести внутрь цикла и вернуть сразу же там

desolver · 2023-12-07T17:22:27Z

cs/TagsCloudVisualization/Layout.cs

+
+public class Layout
+{
+    private readonly List<Rectangle> _rectangles = new();


Извиняюсь за внесенную смуту, в первой версии коллекция rectangles располагалась в правильном месте, давай вернем её в лейаутер вместе с двумя методами для взаимодействия с ними

desolver · 2023-12-07T17:23:02Z

cs/TagsCloudVisualization/TagsCloudVisualization.csproj

+    </PropertyGroup>
+
+    <ItemGroup>
+      <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.5.0" />


В проекте, где нет тестов, этот пакет стал уже не нужен

desolver · 2023-12-07T17:28:10Z

cs/TagsCloudVisualizationTests/CircularCloudLayouterTests.cs

+
+    private static CircularCloudLayouter _layouter;
+
+    [TestCase(0, 0, TestName = "CircularCloudLayouter_PutNextRectangle_ThrowsArgumentExceptionOnZeroDimensions")]


TestName'ы в TestCase'ах нужны для того. чтобы пояснять и дополнять основное название теста подробностями о том, какие параметры они передают (без On, When и прочего) - сухие Zero dimensions и Negative dimensions, и увидишь, как сильно улучшилась читаемость тестов в инспекторе

desolver · 2023-12-07T17:30:45Z

cs/TagsCloudVisualizationTests/CircularCloudLayouterTests.cs

+
+    [TestCase(0, 0, TestName = "CircularCloudLayouter_PutNextRectangle_ThrowsArgumentExceptionOnZeroDimensions")]
+    [TestCase(-1, -1, TestName = "CircularCloudLayouter_PutNextRectangle_ThrowsArgumentExceptionOnNegativeDimensions")]
+    public static void CircularCloudLayouter_PutNextRectangle_ThrowsArgumentExceptionOn(int width, int height)


При таком подходе именования, который ты выбрал, на первое место ставится метод, который тестируется, то есть PutNextRectangle (потому что CircularCloudLayouter уже есть в названии тест-класса), далее идет то, что он должен сделать ShouldThrows..., а в конце, когда он это должен сделать On... или WhenArgumentsAreIncorrect. И TestCase'ы как раз будут дополнять, что за "некорректные аргументы" такие

По такой аналогии проверь, пожалуйста, все тест-методы, каждый можно улучшить

desolver · 2023-12-07T17:35:52Z

cs/TagsCloudVisualizationTests/LayoutTests.cs

+
+    private static Layout _layout;
+
+    [TestCaseSource(nameof(CenterInSource))]


Давай будем исходить из принципа, что TestCaseSource нужно использовать в самую последнюю очередь, когда ты точно уверен, что ни Test, ни TestCase тебе не подходят. Но точно ли тебе тут не подходит TestCase? Аргументы различаются лишь количеством располагаемых прямоугольников, которое ты как раз и можешь передать в аргументах к тесту, а уже в самом тесте вставить их нужное количество раз и проверить результат

Читаемость теста возрастет в разы

desolver · 2023-12-07T17:37:29Z

cs/TagsCloudVisualizationTests/LayoutTests.cs

+    [TestCase(
+        new []{0, 0, 1, 1}, 
+        new [] {2, 2, 1, 1}, 
+        true,
+        TestName = "Layout_CanPutRectangle_ReturnsTrueOnPuttingRectangleToEmptyPlace")]
+    [TestCase(
+        new []{0, 0, 1, 1}, 
+        new [] {0, 0, 1, 1}, 
+        false,
+        TestName = "Layout_CanPutRectangle_ReturnsFalseOnPuttingRectangleToOccupiedPlace")]
+    public static void Layout_CanPutRectangle_Returns(int[] r1, int[] r2, bool expected)


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

desolver · 2023-12-07T17:38:24Z

cs/TagsCloudVisualizationTests/CircularCloudLayouterTests.cs

+            .BeEmpty();
+    }
+
+    [Test, Timeout(400)]


О как, огонь, даже на моем стареньком ноуте тест стабильно стал проходить, молодец!

# Conflicts: # cs/TagsCloudVisualizationTests/CircularCloudLayouterTests.cs

desolver · 2023-12-11T17:51:17Z

cs/TagsCloudVisualization/CircularCloudVisualizer.cs

+        var bitmap = new Bitmap(_visualizerParams.Width, _visualizerParams.Height);
+        var graphics = Graphics.FromImage(bitmap);
+
+        graphics.FillRectangle(new SolidBrush(_visualizerParams.BgColor), new Rectangle(0, 0, _visualizerParams.Width, _visualizerParams.Height));
+
+        var rectanglesPen = new Pen(_visualizerParams.RectangleColor);


Все ли ресурсы тут освобождены?

desolver · 2023-12-11T17:53:51Z

cs/TagsCloudVisualization/VisualizerParams.cs

+    private int _width;
+    private int _height;
+    private string _pathToFile;
+    private string _fileName;
+
+    public VisualizerParams(int width = 500, int height = 500, string fileName = "TagsCloud.png")
+    {
+        Width = width;
+        Height = height;
+        PathToFile = "../../../TagCloudImages";
+        BgColor = Color.Black;
+        RectangleColor = Color.Chocolate;
+        FileName = fileName;
+    }


А если фактическое расположение прямоугольников не влезет в переданный размер изображения?
Получится улучшить это место так, чтобы автоматически подстраиваться под размер?

desolver · 2023-12-11T17:54:17Z

cs/TagsCloudVisualizationExample/Program.cs

+
+
+
+
+
+
+
+


Кучка лишних строк

desolver · 2023-12-11T17:55:47Z

cs/TagsCloudVisualizationTests/CircularCloudLayouterTests.cs

+        AddRectangles(_layouter).Count();
+    }
+
+    private static IEnumerable<Rectangle> AddRectangles(CircularCloudLayouter layouter, int count = 100)


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

desolver · 2023-12-11T18:08:43Z

cs/TagsCloudVisualizationTests/CircularCloudLayouterTests.cs

+    }
+}
+
+public static class TypeExtensions


Также по конвентам - на один класс один файлик

caimanchik added 3 commits December 4, 2023 21:06

CircularCloudLayouter realization and tests

3d16bce

Вынес в отдельную сущность layout взаимодействие с полем для тегов

2caaad1

fixed efficient test

afd9427

desolver suggested changes Dec 5, 2023

View reviewed changes

caimanchik added 5 commits December 6, 2023 22:31

refactored code according to commits by @desolver and added tests for…

b63330d

… Layout.cs

Moved tests to separate test project for best practices

e7c9adc

made inside methods in tests private

23b6b46

removed unused files and packages

c41f348

refactored tests

bd58447

desolver suggested changes Dec 7, 2023

View reviewed changes

caimanchik added 5 commits December 9, 2023 15:44

refactoring according to comments

6bb1bc6

task 2 solve

670e1b8

task 3 solve

d6bc57d

task 3 solve

af81ed9

Merge remote-tracking branch 'origin/master'

8bf1912

# Conflicts: # cs/TagsCloudVisualizationTests/CircularCloudLayouterTests.cs

desolver suggested changes Dec 11, 2023

View reviewed changes

caimanchik added 4 commits December 11, 2023 23:59

refactored according to comments

87ed9da

added example for VisualizerParams constructor overload

b7b1ec5

added using for IDisposable classes

02815c6

added using for IDisposable classes

840302f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Волков Кирилл #216

Волков Кирилл #216

caimanchik commented Dec 4, 2023

desolver Dec 5, 2023

desolver Dec 5, 2023

desolver Dec 5, 2023

desolver Dec 5, 2023

desolver Dec 5, 2023

desolver Dec 5, 2023

desolver Dec 5, 2023

desolver Dec 5, 2023

desolver Dec 5, 2023

desolver Dec 5, 2023

desolver Dec 7, 2023

desolver Dec 7, 2023

desolver Dec 7, 2023

desolver Dec 7, 2023

desolver Dec 7, 2023

desolver Dec 7, 2023

desolver Dec 7, 2023

desolver Dec 7, 2023

desolver Dec 7, 2023

desolver Dec 7, 2023

desolver Dec 11, 2023

desolver Dec 11, 2023

desolver Dec 11, 2023

desolver Dec 11, 2023

desolver Dec 11, 2023

		var leftOffset = size.Width / 2 + (size.Width % 2 == 0 ? 0 : 1);
		var topOffset = size.Height / 2 + (size.Height % 2 == 0 ? 0 : 1);

		@@ -0,0 +1,3 @@
		// See https://aka.ms/new-console-template for more information

		Console.WriteLine("Hello, World!");


		public Point Center => layout.Center;

		public Rectangle[] GetRectangles => rectangles.ToArray();

		@@ -0,0 +1,59 @@
		using System.Drawing;

		namespace TagsCloudVisualization;


		namespace TagsCloudVisualization.Tests;

		public class CircularCloudLayouter_Should

		size.Width / 2 + (size.Width % 2 == 0 ? 0 : 1),
		size.Height / 2 + (size.Height % 2 == 0 ? 0 : 1));


		private static CircularCloudLayouter _layouter;

		[TestCase(0, 0, TestName = "CircularCloudLayouter_PutNextRectangle_ThrowsArgumentExceptionOnZeroDimensions")]


		private static Layout _layout;

		[TestCaseSource(nameof(CenterInSource))]

Волков Кирилл #216

Are you sure you want to change the base?

Волков Кирилл #216

Conversation

caimanchik commented Dec 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment