Холстинин Егор #190

Yrwlcm · 2024-01-23T13:38:29Z

MrTimeChip

В целом, предпроверка на максимальный балл, но над многим стоит подумать и переделать)
Нравится, что код чистый, но вот архитектурные моменты стоит подправить, разграничить ответственность.

MrTimeChip · 2024-01-24T08:56:26Z

TagsCloudVisualization/WordParsers/WordParser.cs

+{
+    public IEnumerable<string> GetInterestingWords(string path, IDullWordChecker dullWordChecker)
+    {
+        var readText = File.ReadAllText(path);


Сейчас тут просто вылетает эксепшен, что файл не найден, плюс нигде не написано, какой путь надо прописать до файла (абсолютный, относительный и т.д.)
Давай здесь выбрасывать другой эксепшен с комментарием и понятным разъяснением?

MrTimeChip · 2024-01-24T09:00:06Z

TagsCloudVisualization/Program.cs

+    [Argument(0)] [Required] public string InputFilePath { get; set; }
+
+    [Argument(1)] [Required] public string OutputFileName { get; set; }


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

MrTimeChip · 2024-01-24T09:04:02Z

TagsCloudVisualization/Program.cs

+    [Option("-fs")] public int FontSize { get; set; } = 50;
+
+
+    private void OnExecute()


Сейчас почти все сервисы добавлены синглтоном. Вопрос - почему?)
По умолчанию, нужно начинать с самого маленького скоупа, понимать, подходит ли он, если нет, подумать над большим скоупом, пока не станет ясно, какой подходит. А здесь ситуация обратная - решили сразу поставить максимальный скоуп, хотя каких-то предпосылок я здесь не вижу. Это как по умолчанию все переменные делать глобальными.

MrTimeChip · 2024-01-24T09:04:50Z

TagsCloudVisualization/Program.cs

+        services.AddSingleton<IDullWordChecker, NoWordsDullChecker>();
+        services.AddSingleton<ICloudLayouter>(x =>
+            new CloudLayouter(x.GetRequiredService<IPointGenerator>(),
+                x.GetRequiredService<Font>(),


Здесь какой-то странное использование DI)
Суть DI в том, что он сам может резолвить зависимости, а ты сюда передаешь их напрямую. Зачем?)

MrTimeChip · 2024-01-24T09:11:06Z

TagsCloudVisualizationTests/WordParser_Should.cs

+    private class TestDullChecker : IDullWordChecker
+    {
+        public bool Check(string word)
+        {
+            var testDullWords = new HashSet<string>() { "this", "is" };
+            return testDullWords.Contains(word);
+        }
+    }


Почему для теста отдельный класс создаем?)
Его же и тестируем по итогу.

MrTimeChip · 2024-01-24T09:14:06Z

TagsCloudVisualization/CloudLayouter.cs

+public class CloudLayouter : ICloudLayouter
+{
+    private readonly IPointGenerator pointGenerator;
+    private Dictionary<string, Font> wordsFonts = new();


Кажется, что использование Font и других графических сущностей - вообще не обязанность Layouter'а.
Он, собственно, должен раскладывать прямоугольники, а не вот это вот всё.

MrTimeChip · 2024-01-24T09:14:50Z

TagsCloudVisualization/CloudLayouter.cs

+            using var smallBitmap = new Bitmap(1, 1);
+            using var graphics = Graphics.FromImage(smallBitmap);
+            var rectangleSize = graphics.MeasureString(wordFont.Key, wordFont.Value);


Здесь тот же момент. Не должно быть здесь работы с графикой. Графика должна рисовать готовую раскладку, а не делать такие странные махинации при раскладке.

Yrwlcm · 2024-01-28T13:36:14Z

TagsCloudVisualization/WordParsers/MystemWordsParser.cs

+    private class JsonWordAnalysis
+    {
+        public string Text { get; set; }
+        public List<Dictionary<string, string>> Analysis { get; set; }
+    }


Вот есть вопрос насколько вообще правильно создавать такие классы. Просто делать его общим тоже не хочется из-за того как изначальный json выглядит и парсится, а именно: полезная инфа в словаре, а словарь в листе. И каждый раз его доставать звучит ужасно. Поэтому я десериализую в этот приватный класс, а затем уже преобразую в публичный который нормально выглядит

В целом, такое приемлемо, но, конечно, по возможности стоит избегать. Но в данном контексте - более чем нормально.

Yrwlcm · 2024-01-28T13:41:01Z

TagsCloudVisualization/Program.cs

+        if (SqareAlgorithm)
+            services.AddTransient<IPointGenerator, LissajousCurvePointGenerator>();
+        else
+            services.AddTransient<IPointGenerator, SpiralPointGenerator>();


Вот это тоже плохо выглядит, но не смог нагуглить как привязывать сервис в зависимости от условия в microsoft dependency injection

В целом, нет такой вещи, как зарегистрировать или не зарегистрировать в зависимости от условия.
Тебе надо зарегистрировать PointGenerator'ы все разом, а потом в отдельном классе ты их принимаешь массивом и производишь выбор в зависимости от значения параметра.

MrTimeChip · 2024-01-30T10:25:25Z

TagsCloudVisualization/Program.cs

+        { "ADVPRO", "APRO", "INTJ", "CONJ", "PART", "PR", "SPRO" };
+
+    [Option("-square", Description = "Will use another algorithm to generate square tag cloud instead of circular.")]
+    private bool SquareAlgorithm { get; set; }


Как-то не очень это расширяемо, а если третий алгоритм добавить?

MrTimeChip · 2024-01-30T10:26:38Z

TagsCloudVisualization/Program.cs

+    private void OnExecute()
+    {
+        var services = new ServiceCollection();
+        services.AddTransient<Font>(x => new Font(FontFamily, FontSize));


Ладно, здесь немного со скоупом вышел перебор, пожалуй.
Вообще, по дефолту используют Scoped.

MrTimeChip · 2024-01-30T13:14:09Z

TagsCloudVisualization/Program.cs

+            new MystemDullWordChecker(RemovedPartsOfSpeech, ExcludedWordsFile));
+        services.AddScoped<IInterestingWordsParser, MystemWordsParser>();
+        services.AddScoped<IRectangleLayouter>(
+            x => new RectangleLayouter(x.GetKeyedService<IPointGenerator>(PointGenerator)));


Интересная штука, KeyedService новая вещь. Но стоит посмотреть, как её правильно использовать.
https://weblogs.asp.net/ricardoperes/net-8-dependency-injection-changes-keyed-services

Yrwlcm · 2024-01-30T17:34:10Z

TagsCloudVisualization/Program.cs

+        {
+            layoutDrawer
+                .CreateLayoutImageFromFile(InputFilePath, new Size(ImageWidth, ImageHeight), MinimalFontSize)
+                .SaveImage(OutputFilePath, SaveImageFormat);
+        }


Не уверен насколько сейчас рационально выносить эти поля в TagLayoutSettings, так как сейчас мы не прячем ничего внутрь метода

Yrwlcm added 4 commits January 22, 2024 11:26

Added initial TagCloudLayoter

9edd430

Word parser

b44c2b2

Now draws text instead of rectangles

2d7478c

Console app with DI container

98269c6

MrTimeChip reviewed Jan 24, 2024

View reviewed changes

Yrwlcm added 6 commits January 27, 2024 15:00

Refactored responsibilities and DI

b4aac5f

Word parser with word formation processing

f0e45c9

Pallete now supports color ordering

7522b53

Refactoring and description for options

d3d0d51

Another algorithm

491567f

Readme and examples

d7d051b

Yrwlcm commented Jan 28, 2024

View reviewed changes

Cleared .cjproj

3485b50

Yrwlcm commented Jan 28, 2024

View reviewed changes

Fixed typo

8d308b8

MrTimeChip reviewed Jan 30, 2024

View reviewed changes

Refactored DI

7505905

MrTimeChip reviewed Jan 30, 2024

View reviewed changes

Yrwlcm added 3 commits January 30, 2024 21:35

Refactored with TagLayoutSettings

4c45b2c

More refactoring using TagLayoutSettings

5c8dff7

Updated tests

b3fbaa8

Yrwlcm commented Jan 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Холстинин Егор #190

Холстинин Егор #190

Yrwlcm commented Jan 23, 2024

MrTimeChip left a comment

MrTimeChip Jan 24, 2024

MrTimeChip Jan 24, 2024

MrTimeChip Jan 24, 2024

MrTimeChip Jan 24, 2024

MrTimeChip Jan 24, 2024

MrTimeChip Jan 24, 2024

MrTimeChip Jan 24, 2024

Yrwlcm Jan 28, 2024 •

edited

Loading

MrTimeChip Jan 30, 2024

Yrwlcm Jan 28, 2024 •

edited

Loading

MrTimeChip Jan 30, 2024

MrTimeChip Jan 30, 2024

MrTimeChip Jan 30, 2024

MrTimeChip Jan 30, 2024

Yrwlcm Jan 30, 2024

		[Argument(0)] [Required] public string InputFilePath { get; set; }

		[Argument(1)] [Required] public string OutputFileName { get; set; }

		[Option("-fs")] public int FontSize { get; set; } = 50;


		private void OnExecute()

Холстинин Егор #190

Are you sure you want to change the base?

Холстинин Егор #190

Conversation

Yrwlcm commented Jan 23, 2024

MrTimeChip left a 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

Yrwlcm Jan 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yrwlcm Jan 28, 2024 • edited Loading

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

Yrwlcm Jan 28, 2024 •

edited

Loading

Yrwlcm Jan 28, 2024 •

edited

Loading