Савельев Григорий #200

luvairo-m · 2024-01-24T17:31:37Z

…sualization теперь ялвяется библиотекой классов. Исправление некоторых тестов.

pakapik · 2024-01-25T11:19:52Z

TagsCloud/Colorizers/AllRandomColorizer.cs

+
+    public AllRandomColorizer(Color[] colors) : base(colors)
+    {
+        random = new Random();


По возможности стоит поля инициализировать на месте объявления.

pakapik · 2024-01-25T11:26:58Z

TagsCloud/Colorizers/ColorizerBase.cs

+        this.colors = colors;
+    }
+
+    public abstract void Colorize(Dictionary<CloudTag, int> frequencyStatistics);


Думаю, Dictionary можно было бы заменить на IDictionary. Как правило, входящие параметры принимают максимально возможный общий тип, а на выход отдается максимально возможный конкретный тип. Говорю "максимально возможный", т.к. всё зависит от конкретной ситуации.

pakapik · 2024-01-25T11:28:56Z

TagsCloud/Colorizers/OneVsRestColorizer.cs

+[ColorizerName(ColoringStrategy.OneVsRest)]
+public class OneVsRestColorizer : ColorizerBase
+{
+    public OneVsRestColorizer(Color[] colors) : base(colors)


Например, тут вполне мог бы быть IList, а не массив.

pakapik · 2024-01-25T11:29:34Z

TagsCloud/Contracts/IFactoryOptions.cs

+
+public interface IFactoryOptions
+{
+    public FontFamily FontFamily { get; set; }


В интерфейсе нет смысла задавать модификатор доступа его членам. Проверь везде, пожалуйста

pakapik · 2024-01-25T11:31:09Z

TagsCloud/Colorizers/ColorizerBase.cs

+        this.colors = colors;
+    }
+
+    public abstract void Colorize(Dictionary<CloudTag, int> frequencyStatistics);


Расскажи, пожалуйста, почему решил, что это должен быть именно абстрактный класс и абстрактный метод, а не интерфейс, объявляющий этот метод? В таком случае, ничего не мешает и абстрактный класс в т.ч. реализовать.

pakapik · 2024-01-25T13:33:24Z

TagsCloud.Tests/SpiralTests.cs

+    private Spiral spiral;
+    private Random random;
+
+    private float distanceDelta, angleDelta;


На самом деле, такую запись видел исключительно в справочниках C#)
Она неплохая, но редко используемая

pakapik · 2024-01-25T13:35:17Z

TagsCloud.Tests/SpiralTests.cs

+        var another = new Spiral(distanceDelta, angleDelta);
+        spiral.GetNextPoint();
+
+        spiral.GetNextPoint().Should().NotBe(another.GetNextPoint());


Ты проверил, что 2-а точка spiral не равняется первой точке another. Не очень понимаю, причем тут SaveStateBetweenCalls

pakapik · 2024-01-25T13:42:44Z

TagsCloud.Tests/SpiralTests.cs

+    }
+
+    [Test]
+    public void GetNextPoint_Should_ReturnCartesianCoordinates()


Не уверен, что этот тест имеет смысл
Объясни, пожалуйста, что и зачем проверяет этот тест?

pakapik · 2024-01-25T13:43:25Z

TagsCloud.Tests/SpiralTests.cs

+    }
+
+    [Test]
+    public void GetNextPoint_Should_ReturnCorrectValues()


Тем более, что этот тест делает ровно то же самое, что и тест выше, но в цикле

pakapik · 2024-01-25T13:46:18Z

TagsCloud/Helpers/ColorizerHelper.cs

+    public static ColorizerBase? GetAppropriateColorizer(Color[] colors, ColoringStrategy strategy)
+    {
+        var colorizerType = Assembly
+            .GetExecutingAssembly()


Сборка будет сканироваться при каждом вызове метода. DI-контейнер должен единожды понять, что тебе нужно, а затем возвращать реализации типов по запросу.

pakapik · 2024-01-26T13:11:25Z

TagsCloud/Entities/WordAnalysis.cs

+{
+    [JsonPropertyName("analysis")] public List<WordAnalysis> Analyses    { get; set; }
+    [JsonPropertyName("text")]     public string             InitialWord { get; set; }
+    public                                bool               IsRussian   => Analyses.Count > 0;


Analyses.Count > 0; может вызвать NRE

pakapik · 2024-01-26T13:18:56Z

TagsCloud/Entities/WordInfo.cs

+
+    [JsonPropertyName("lex")] public string Infinitive   { get; set; }
+    [JsonPropertyName("gr")]  public string Grammar      { get; set; }
+    public                           string LanguagePart => Grammar.Split(grammarSeparators)[0];


Тоже потенциальный nre + выход за границу массива

pakapik · 2024-01-26T13:20:19Z

TagsCloud/Extensions/ServiceCollectionExtensions.cs

+    public static ServiceCollection AddFiltersWithOptions(this ServiceCollection collection, IFilterOptions options)
+    {
+        var filters = Assembly.GetExecutingAssembly().GetTypes()
+            .Where(type => type.IsClass && type.IsSubclassOf(typeof(FilterBase)));


Кажется, что если type.IsSubclassOf(typeof(FilterBase)) истина, то и type.IsClass автоматически тоже.

pakapik · 2024-01-26T13:24:10Z

TagsCloud/Filters/SpeechPartFilter.cs

+    public override void Apply(List<WordToStatus> words)
+    {
+        var rawWords = words.Select(word => word.Word).ToList();
+        var analyses = TextAnalyzer.GetTextAnalysis(rawWords).ToList();


List подразумевает расширение коллекции. Ты же только обращаешься по индексу к коллекции.

pakapik · 2024-01-26T13:26:01Z

TagsCloud/Filters/SpeechPartFilter.cs

+                finalWord = options.ToInfinitive ? wordAnalysis.Infinitive : wordInfo.InitialWord;
+            }
+
+            words[i].Word = options.WordsCase == CaseType.Lower ? finalWord.ToLower() : finalWord.ToUpper();


Должно ли изменение слова происходить в фильтре? Фильтр подразумевает отсеивание элементов. Например, .Where() - это фильтр

pakapik · 2024-01-26T13:32:05Z

TagsCloud/Helpers/FileHelper.cs

+        var reader = FindAppropriateReader(fileExtension);
+
+        if (reader == null)
+            throw new NotSupportedException("Unknown file extension!");


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

pakapik · 2024-01-26T13:38:10Z

TagsCloud/Program.cs

+
+        var colors = Colors!.Select(color => Color.ParseHex(color)).ToArray();
+
+        var options = new OptionsBuilder().SetColorizer(colors, Strategy).SetWordsCase(WordsCase)


В прошлый раз, когда каждый Set был с новой строки, было симпатичнее)

pakapik · 2024-01-26T13:39:40Z

TagsCloudVisualization/Layout.cs

+
+public class Layout : ILayout
+{
+    public const float Accuracy = 1e-3f;


Думаю, можно вынести в ILayout

pakapik · 2024-01-26T13:43:08Z

TagsCloud/Contracts/IFactoryOptions.cs

+
+public interface IFactoryOptions
+{
+    FontFamily    FontFamily { get; set; }


Просто как факт: в разного рода Options часто используется init вместо set, т.к. опции редко меняются

pakapik · 2024-01-26T13:50:43Z

TagsCloud/TagCloudFacade.cs

+    {
+        var provider = new ServiceCollection().AddFiltersWithOptions(options).BuildServiceProvider();
+
+        var filterConveyor = provider.GetService<FilterConveyor>();


Будет хорошо, если ты всё же пощупаешь di чуть более конкретно.
Минимум:

Сделай класс (а над ним интерфейс), в котором будет конструктор с 1 или более зависимостями (обязательно интерфейсы).
Всё это надо зарегистрировать в di-контейнере

Как правило, из DI достают не конкретный тип, как здесь, а реализацию интерфейса - какая конкретно вернется реализация, уже другой вопрос.

Почитай статью про различия DI-контейнера и ServiceLocator

Через DI зарезолви тип из пункта 1

Максимум:
В идеале в одном месте зарегистрировать в DI все типы (руками или через рефлексию), на старте приложения запустить DI, а дальше оно уже само как-нибудь

…ключенных слов и частей речи. Добавление сортировки, различных алгоритмов измерения текста.

pakapik · 2024-01-29T05:27:54Z

TagsCloud/FileReaders/CsvFileReader.cs

+
+public class CsvFileReader : IFileReader
+{
+    public string SupportedExtension => "csv";


Объясни, пожалуйста, разницу между => и = в данном случае?

pakapik · 2024-01-29T05:34:25Z

TagsCloud/Processors/CloudProcessor.cs

+        var minFontSize = cloudOptions.MinFontSize;
+        var maxFontSize = cloudOptions.MaxFontSize;
+
+        var maxFrequency = wordGroups.Max(group => group.Count);


Почему бы не реализовать метод MinMax?

pakapik · 2024-01-29T05:35:57Z

TagsCloud/Processors/CloudProcessor.cs

+
+        foreach (var group in wordGroups)
+        {
+            var fontSize = measurer.GetFontSize(


Из сигнатуры GetFontSize не очень понятно было, что надо передавать частоты и "чужие" размеры шрифта

pakapik · 2024-01-29T05:39:30Z

TagsCloud/Processors/CloudProcessor.cs

+        var painter = painters
+            .SingleOrDefault(colorizer => colorizer.Strategy == cloudOptions.ColoringStrategy);
+
+        painter!.Colorize(wordGroups, cloudOptions.Colors);


Зачем используется связка OrDefault с последующим !?

pakapik · 2024-01-29T06:11:45Z

TagsCloud/Processors/CloudProcessor.cs

+    {
+        return cloudOptions.Sort switch
+        {
+            SortType.Ascending => wordGroups.OrderBy(group => group.Count).ToHashSet(),


HashSet неупорядоченная структура. Тот факт, что ты создаешь хэшсет из упорядоченного набора, ничего не значит. .net при перечислении будет возвращать элементы в том порядке, в котором они были добавлены, но это странный контракт, который вполне может измениться. Более того, это обычно ломается, если из коллекции удаляются элементы. А далее кто помешает после GetSortedGroups в полученный хэшсет добавить элемент и сломать всю сортировку?
Так что надо либо найти замену HashSet, либо подумать над тем, зачем тебе вообще его сортировать, если HashSet - принципиально не про сортированный набор.

pakapik · 2024-01-29T06:16:16Z

TagsCloud/Processors/InputProcessor.cs

+    public HashSet<WordTagGroup> CollectWordGroupsFromFile(string filename)
+    {
+        var extension = filename.Split('.', StringSplitOptions.RemoveEmptyEntries)[^1];
+        var reader = FindFileReader(extension);


nit: кажется, что в FindFileReader можно сразу передать filename

pakapik · 2024-01-29T06:17:22Z

TagsCloud/Processors/InputProcessor.cs

+        var extension = filename.Split('.', StringSplitOptions.RemoveEmptyEntries)[^1];
+        var reader = FindFileReader(extension);
+
+        if (reader == null)


nit: да и проверку туда же можно было бы запихнуть

pakapik · 2024-01-29T06:35:33Z

TagsCloudVisualization/WordTagGroup.cs

+
+    public override bool Equals(object obj)
+    {
+        return obj is WordTagGroup group && group.WordInfo.Text.Equals(WordInfo.Text);


в Equals следует передать StringComparsion. Не забудь, что GetHashCode и Equals работают в паре

pakapik · 2024-01-29T06:44:03Z

TagsCloud/Builders/CloudOptionsBuilder.cs

+        else
+        {
+            const string fontName = nameof(TagsCloud) + ".Fonts.Vollkorn-SemiBold.ttf";
+            var fontStream = Assembly.GetExecutingAssembly().GetManifestResourceStream(fontName);


Почему бы не убрать это в поля? В Lazy-поле по желанию

pakapik · 2024-01-29T06:47:16Z

TagsCloud/Extensions/ServiceCollectionExtensions.cs

+        var readers = GetTypesByInterface(readerType);
+
+        foreach (var reader in readers)
+            collection.AddSingleton(readerType, reader);


Неплохо)
Но тут можешь вспомнить историю с атрибутами.

Сканируешь сборку, достаешь все типы из неё с нужным атрибутом. Атрибут выполняет роль "положи меня в DI" и там же удобно будет указать жизненный цикл сущности. И кладешь в DI-сразу и регистрируемый тип (как правило, интерфейс), и реализацию типа да ещё и ~~цикленный жизл~~ жизненный цикл в придачу указать можно).

И доставай сервисы через тип-интерфейс, если возможно - чтоб было по фен-шую

…та InjectionAttribute. Небольшие правки в некоторых классах.

pakapik · 2024-01-30T14:26:10Z

TagsCloud/Processors/CloudProcessor.cs

    }

-    private IFontMeasurer FindFontMeasurer()
+    private static (int minValue, int maxValue) GetMinMaxValues(IEnumerable<WordTagGroup> wordGroups)


nit: из этого можно было бы сделать обобщённый утилитный метод MinMax

pakapik · 2024-01-30T14:27:54Z

TagsCloud/Processors/InputProcessor.cs

    {
-        return fileReaders.SingleOrDefault(reader => reader.SupportedExtension.Equals(extension));
+        var extension = GetFileExtension(filename);
+        var reader = fileReaders.SingleOrDefault(reader => reader.SupportedExtension.Equals(extension));


Почему Equals, а не ==? В таком случае стоило бы передать StringComparsion

pakapik · 2024-01-30T14:32:35Z

TagsCloud.Tests/DefaultPostFormatterTests.cs

+    [TestCase("A", "A")]
+    [TestCase("   ___   Juice", "")]
+    [TestCase("                   ", "")]
+    public void Formatter_Should_CutLineToFirstNonLetterCharacter(string line, string expected)


Попробуй ещё сделать вот так, но перезаливать нет нужды, просто попробуй.

В TestFixture можно добавить TestOf. К методу можно добавить такой же атрибут TestOf - тогда нет нужды в имя теста вписывать имя метода / сущности. Тестируемую систему часто называют sut

pakapik · 2024-01-30T14:35:40Z

TagsCloud.Tests/FileReadersTests.cs

+            var match = ReaderNamePattern().Match(reader.GetType().Name);
+
+            match.Success.Should().Be(true);
+            match.Groups[1].Value.ToLower().ShouldBeEquivalentTo(reader.SupportedExtension);


Сомнительный тест, т.к. по контракт по имени класса не самый сильный, но в целом пойдет, т.к. административные конвенции всё же встречаются.

pakapik · 2024-01-30T14:38:35Z

TagsCloud.Tests/TextAnalyzerTests.cs

+            group.WordInfo.IsRussian.Should().Be(isEnglish);
+    }
+
+    private static IEnumerable<(WordTagGroup Group, bool isRussian)> GetTestGroups()


Кажется, проще было реализовать массив, чем прописывать yield return, но ок

pakapik · 2024-01-30T14:39:22Z

TagsCloud/Processors/InputProcessor.cs

                         .GroupBy(line => line)
                         .Select(group => new WordTagGroup(group.Key, group.Count()))
                         .ToHashSet();

+        if (wordGroups.Count == 0)
+            throw new ArgumentException("No words found! Check file structure.");


Можно информативнее 🙃, у тебя на руках есть имя файла

luvairo-m added 9 commits January 22, 2024 09:47

Initial commit

da15221

Переработка класса, отвечающего за отрисовку изображения. TagsCloudVi…

0780e96

…sualization теперь ялвяется библиотекой классов. Исправление некоторых тестов.

Добавление точки входа в программу. Замена Word => Tag

73a2267

Добавление фильтров и источников информации.

b22dab8

Правки в фильтрах.

350b9c1

Первая рабочая версия.

b3a8bfe

Добавление переворотов слов.

6fe9c1d

Добавлен CLI

553449d

Исправление ошибки ввода частей речи.

a216c4e

pakapik reviewed Jan 25, 2024

View reviewed changes

luvairo-m added 3 commits January 26, 2024 17:56

Первые правки.

554883b

Reformat кода.

c9e8946

Reformat TagCloudVisualization.

5579be7

pakapik reviewed Jan 26, 2024

View reviewed changes

luvairo-m added 5 commits January 27, 2024 23:12

Новая структура.

9965d1e

Переработка фильтров. Расширение ServiceCollectionExtensions.

5ecfab1

Опции для процессоров теперь неизменяемые.

4892a1e

Реализация csvfilereader + docxfilereader.

609ddca

pakapik reviewed Jan 29, 2024

View reviewed changes

luvairo-m added 2 commits January 30, 2024 10:34

Сведение регистрации зависимостей к одному методу и добавление атрибу…

f3c6de3

…та InjectionAttribute. Небольшие правки в некоторых классах.

Добавление тестов.

2f385c6

pakapik reviewed Jan 30, 2024

View reviewed changes


		var colors = Colors!.Select(color => Color.ParseHex(color)).ToArray();

		var options = new OptionsBuilder().SetColorizer(colors, Strategy).SetWordsCase(WordsCase)

Савельев Григорий #200

Are you sure you want to change the base?

Савельев Григорий #200

Conversation

luvairo-m commented Jan 24, 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

pakapik Jan 26, 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

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

pakapik Jan 29, 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

Choose a reason for hiding this comment

pakapik Jan 26, 2024 •

edited

Loading

pakapik Jan 29, 2024 •

edited

Loading