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

SolarHunger · 2024-01-26T15:29:13Z

No description provided.

desolver · 2024-01-26T15:36:16Z

TagsCloud/ColorGenerators/RandomColorGenerator.cs

+
+namespace TagsCloud.ColorGenerators;
+
+public class RandomColorGenerator: IColorGenerator


Пройдись форматтером по файликам, нужно поправить отступы, после класса должен быть пробел, потом двоеточие, потом снова пробел и класс или интерфейс, от которого наследуешься. В других местах лишние пробелы после ключевых слов или лишние пустые строки в конце файлов. В общем, пофикси, пожалуйста

desolver · 2024-01-26T16:47:42Z

TagsCloud/Distributors/SpiralDistributor.cs

+        private double Angle { get;  set; }
+        private double AngleStep { get; set; }
+        private double RadiusStep { get; set; }
+        private Point Center { get; set; }
+        private double Radius { get; set; }


Приватные свойства пишутся с маленькой буквы

desolver · 2024-01-26T16:49:19Z

TagsCloud/Entities/Tag.cs

+        using (Graphics g = Graphics.FromImage(new Bitmap(1,1)))
+        {
+            size = g.MeasureString(content, font);
+        }


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

desolver · 2024-01-26T16:50:04Z

TagsCloud/Entities/Tag.cs

+    public RectangleF TagRectangle;
+    public  Color Color;
+    public  string Content;
+    public Font Font;
+    public SizeF size;


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

desolver · 2024-01-26T16:51:34Z

TagsCloud/Layouters/CircularCloudLayouter.cs

+
+public class CircularCloudLayouter:ILayouter
+{
+    public List<Tag> Tags { get; set; }


По той же причине не очень хорошо выставлять наружу List, [], HashSet и др, даже если там будет стоять private set - да, инициализировать заново ты коллекцию не сможешь, а вот сделать извне layouter.Tags.Add(new Tag) - вполне - и это сломает выполнение программы

desolver · 2024-01-26T16:52:46Z

TagsCloud/TagsCloudPainters/SimplePainter.cs

+        var maxX = tags.Select(rec => rec.TagRectangle.X + rec.TagRectangle.Width / 2).Max();
+        var minX = tags.Select(rec => rec.TagRectangle.X - rec.TagRectangle.Width / 2).Min();
+        var maxY = tags.Select(rec => rec.TagRectangle.Y + rec.TagRectangle.Height / 2).Max();
+        var minY = tags.Select(rec => rec.TagRectangle.Y - rec.TagRectangle.Height / 2).Min();


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

desolver · 2024-01-26T16:53:50Z

TagsCloud/TextReaders/FileTextReader.cs

+
+    public Tuple<string,int>[] GetWords(string filePath)
+    {
+        var words = File.ReadAllText(filePath).Split('\n');


Давай обработаем все возможные переносы строк, вдруг файл был сохранен на другой системе - \n и \r\n

desolver · 2024-01-26T16:54:03Z

TagsCloud/TextReaders/FileTextReader.cs

+    public Tuple<string,int>[] GetWords(string filePath)
+    {
+        var words = File.ReadAllText(filePath).Split('\n');
+        return words.Where(w=>validator.IsWordValid(w))


.Where тоже на новую строчку нужно, как и ToArray ниже

desolver · 2024-01-26T16:55:37Z

TagsCloud/TextReaders/FileTextReader.cs

+        var words = File.ReadAllText(filePath).Split('\n');
+        return words.Where(w=>validator.IsWordValid(w))
+            .GroupBy(word => word)
+            .Select(group => Tuple.Create(group.Key, group.Count())).ToArray(); 


А почему ты используешь тут массив таплов, а не словарь, где ключом будет слово, а значением - его количество?

desolver · 2024-02-01T18:12:52Z

TagsCloud/Program.cs

+            var o = scope.Resolve<Options>().PartsOfSpeech.Count();
+            Console.WriteLine(o);


Эти строчки кажутся тут лишними

desolver · 2024-02-01T18:13:38Z

TagsCloud/Program.cs

+        var options = CommandLine.Parser.Default.ParseArguments<Options>(args).Value;
+        var container = ContainerConfig.Configure(options);
+
+        using (var scope = container.BeginLifetimeScope())


Можно подсыпать сахарку немного и написать так:

using var scope = container.BeginLifetimeScope();

Тогда можно будет убрать лишнюю вложенность в фигурных скобках

desolver · 2024-02-01T18:15:20Z

TagsCloud/WordsProvider/IWordsProvider.cs

@@ -0,0 +1,8 @@
+using TagsCloud.WordValidators;
+
+namespace TagsCloud.TextReaders;


Неймспейс интерфейса и класса в этой папке не соответствуют ожидаемому (скорее всего просто папку переименовал, а неймспейсы нет)

desolver · 2024-02-01T18:19:49Z

TagsCloud/Distributors/SpiralDistributor.cs

+    public double Angle { get; private set; }
+    public double Radius { get; private set; }
+    public readonly double AngleStep;
+    public readonly double RadiusStep;
+    public readonly Point Center;


А почему часть значений сделана свойствами, а часть - полями?

Давай придерживаться практики, что полями должны быть только зависимости, прилетающие в конструкторе (и возможно некоторые приватные компоненты)

desolver · 2024-02-01T18:36:05Z

TagsCloud/Layouters/CircularCloudLayouter.cs

+        var changes = 1;
+        while (changes > 0)


Логика с changes тут выглядить немного лишней, так как вызов CompressByAxis по идее должен выглядеть как-то так:

rectangle = CompressByAxis(rectangle, true); rectangle = CompressByAxis(rectangle, false);

Условно - пожали по X, пожали по Y, вроде бы changes тут даже и не нужно

это сделано для того чтобы если мы не можем допустим сдвинуть по X, но можем по Y, проверить можем ли мы после сдвига все таки еще подвинуть по X. Например такая вообразимая ситуация: у нас есть свободное пространство в виде лесенки, где мы можем сместить объект по Х затем по Y, потом опять по X и только если мы будем делать это по очереди, с этой штукой я максимально как бы убеждаюсь, что я максимально возможно сдвинул все к центру. Возможно это немного лишнее, но вот просто в голове возник такой кейс и я решил немного логику усложнить

desolver · 2024-02-01T18:38:09Z

TagsCloud/Layouters/CircularCloudLayouter.cs

+        bottomBorder = bottom < bottomBorder ? bottom : bottomBorder;
+    }
+
+    private bool CheckIntersection(Rectangle currRectangle)


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

desolver · 2024-02-01T18:49:07Z

TagsCloud/TagsCloudPainters/SimplePainter.cs

+        this.backgroundColor = Color.FromName(options.Background);
+    }
+
+    public void DrawCloud(ILayouter layouter)


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

desolver · 2024-02-01T18:49:32Z

TagsCloud/TagsCloudPainters/SimplePainter.cs

+            throw new ArgumentException();
+        var bitmapsize = imageSize.IsEmpty ? layouter.GetImageSize() : imageSize;
+
+        using (var bitmap = new Bitmap(bitmapsize.Width, bitmapsize.Height))


Тут можно провернуть аналогичный трюк с using, получится покрасивше

desolver · 2024-02-01T18:51:43Z

TagsCloud/WordFontCalculators/SimpleWordFontCalculator.cs

+using System.Drawing;
+using TagsCloud.ConsoleCommands;
+
+namespace TagsCloud.WordSizeCalculators;


Неймспейсы класса и интерфейса в папке тоже не совпадают с ожидаемыми

desolver · 2024-02-01T18:58:10Z

TagsCloudTests/Distributors/SpiralDistributorTests.cs

+    [TestCase(0, 1, TestName = "When_AngleStep_Is_Zero")]
+    [TestCase(0.6, -1, TestName = "When_Radius_Is_Negative")]
+    [TestCase(0.6, 0, TestName = "When_Radius_Is_Zero")]
+    public void SpiralDistribution_Initialize_Throw_ArgumentException(double angleStep, double radiusStep)


Нужно поработать над названиями всех тестов в проекте. Ты придерживаешься подхода Action_Should..._When/On..., то есть определенное действие_должно сделать то-то_при таких то условиях. Давай рассмотрим это на примере этого теста: тут ты работаешь с конструктором, проверяешь именно его, поэтому Action-Constructor; что должен сделать? должен упасть с эксепшном, поэтому ShouldThrowArgumentException, и условия падения прописаны в тесткейсах, тут норм. В итоге будет Constuctor_ShouldThrowArgumentException.

Давай подчеркивания использовать только для разделения трех частей в названии. Каждое слово отделять не нужно

Add interfaces and simple classes

6a52d04

desolver suggested changes Jan 26, 2024

View reviewed changes

SolarHunger changed the title ~~Add interfaces and simple classes~~ Ренат Зубакин Jan 28, 2024

SolarHunger added 5 commits February 1, 2024 03:48

Add tests, di and console

8b5d4f9

refactoring

edcb92f

refactoring

f74ba1c

code cleaning

ecd3b3d

поправил тест для рисовальщика

7c92409

desolver suggested changes Feb 1, 2024

View reviewed changes

SolarHunger added 2 commits February 3, 2024 18:11

Corrected mentor's comments

7c9ab4b

Correct tests run Error

4b5972c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

SolarHunger commented Jan 26, 2024

desolver Jan 26, 2024

desolver Jan 26, 2024

desolver Jan 26, 2024

desolver Jan 26, 2024

desolver Jan 26, 2024

desolver Jan 26, 2024

desolver Jan 26, 2024

desolver Jan 26, 2024

desolver Jan 26, 2024

desolver Feb 1, 2024

desolver Feb 1, 2024

desolver Feb 1, 2024

desolver Feb 1, 2024

desolver Feb 1, 2024

SolarHunger Feb 3, 2024

desolver Feb 1, 2024

desolver Feb 1, 2024

desolver Feb 1, 2024

desolver Feb 1, 2024

desolver Feb 1, 2024


		namespace TagsCloud.ColorGenerators;

		public class RandomColorGenerator: IColorGenerator

		var o = scope.Resolve<Options>().PartsOfSpeech.Count();
		Console.WriteLine(o);

		@@ -0,0 +1,8 @@
		using TagsCloud.WordValidators;

		namespace TagsCloud.TextReaders;

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

Are you sure you want to change the base?

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

Conversation

SolarHunger commented Jan 26, 2024

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