Conversation
SquirrelLeonid
left a comment
There was a problem hiding this comment.
Много вопросов касательно устройства консольного клиента. В частности парсинга аргументов. Как минимум нужно поправить замечания, как максимум - рекомендую все таки обратить внимание на одну из предлагаемых библиотек для парсинга.
Плюс исправить другие моменты.
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Text; | ||
| using System.Threading.Tasks; | ||
| using NUnit.Framework; | ||
| using System.Linq; | ||
| using TagCloudGenerator.Infrastructure.Filters; |
There was a problem hiding this comment.
Вот тут есть неиспользуемые зависимости. Нужно убрать
| [SetUp] | ||
| public void Setup() | ||
| { | ||
| filter = new BoringWordsFilter(); | ||
| } |
There was a problem hiding this comment.
Нет особой нужды создавать новый объект перед каждым тестом. Внутри класса у тебя ведь нет кэширования, чувствительной информации или логики, которая может повлиять на последующие прогоны.
В прочем если на перспективу будешь реализовывать управление списком скучных слов, то тогда Setup будет уместнее. Но там скорее каждый тест будет внутри себя конфигурировать этот объект
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Text; | ||
| using System.Threading.Tasks; | ||
| using Moq; | ||
| using NUnit.Framework; | ||
| using System.Collections.Generic; | ||
| using System.Drawing; | ||
| using System.Linq; | ||
| using TagCloudGenerator.Core.Interfaces; | ||
| using TagCloudGenerator.Core.Models; | ||
| using TagCloudGenerator.Core.Services; |
There was a problem hiding this comment.
Здесь тоже есть неиспользуемые зависимости.
Вообще, во многих классах они есть. Нужно пройтись и подчистить
| File.Delete(tempFilePath); | ||
| } | ||
|
|
||
| private void WriteToTempFile(string content) |
There was a problem hiding this comment.
Приватные методы следует располагать после публичных
| var expectedLines = new[] { "line1", "line2", "line3" }; | ||
| WriteToTempFile(string.Join(Environment.NewLine, expectedLines)); | ||
|
|
||
| var result = reader.TryRead(tempFilePath).ToList(); |
There was a problem hiding this comment.
Здесь можем свалиться на null reference exception при вызове ToList. В других местах тоже
| { | ||
| public class RenderSettings : IRenderSettings | ||
| { | ||
| public string OutputPath { get; set; } |
There was a problem hiding this comment.
Не уверен, что это должно быть у RenderSettings. Лучше убрать его отсюда и в CloudGenerator просто передавать путь в вызов PngRenderer
|
|
||
| namespace TagCloudGenerator.Core.Models | ||
| { | ||
| public class RenderSettings : IRenderSettings |
There was a problem hiding this comment.
Тоже вопрос - а насколько это вероятная точка расширения? С точки зрения новых реализаций.
То, что RenderSettings может быть дополнен - это бесспорно, но вот наличие разных IRenderSettings скорее запутает, чем поможет разделить код
| public string FontFamily { get; set; } = "Arial"; | ||
| public float MinFontSize { get; set; } = 12f; | ||
| public float MaxFontSize { get; set; } = 72f; | ||
| public Color BackgroundColor { get; set; } | ||
| public Color TextColor { get; set; } | ||
| public Size CanvasSize { get; set; } | ||
| public bool CenterCloud { get; set; } | ||
| public bool ShowRectangles { get; set; } | ||
| public int Padding { get; set; } = 50; |
There was a problem hiding this comment.
Я бы предложил сгруппировать настройки. Как минимум можно выделить в отдельный класс настройки для текста (семейство шрифта, размер, цвет)
|
|
||
| namespace TagCloudGenerator.Core.Models | ||
| { | ||
| public class CloudItem : ICloudItem |
There was a problem hiding this comment.
В другом комментарии уже поднял вопрос о целесообразности интерфейса
| public static CloudItem Create(string word, Rectangle rectangle, float fontSize) | ||
| { | ||
| return new CloudItem(word, rectangle, fontSize); | ||
| } | ||
|
|
||
| public CloudItem WithRectangle(Rectangle newRectangle) | ||
| { | ||
| return new CloudItem(Word, newRectangle, FontSize, Color, FontFamily, FontStyle, Frequency, Weight); | ||
| } | ||
|
|
||
| public CloudItem WithFontSize(float newFontSize) | ||
| { | ||
| return new CloudItem(Word, Rectangle, newFontSize, Color, FontFamily, FontStyle, Frequency, Weight); | ||
| } | ||
|
|
||
| public CloudItem WithColor(Color newColor) | ||
| { | ||
| return new CloudItem(Word, Rectangle, FontSize, newColor, FontFamily, FontStyle, Frequency, Weight); | ||
| } | ||
| } |
There was a problem hiding this comment.
Ранее я писал замечание про то, что мы переходим на зависимость от конкретной реализации.
Этого набора методов не видел. В целом он тоже решает проблему. У нас есть метод Create, который создает объект и есть набор методов для редактирования полей.
Только если будешь оставлять интерфейс ICloudItem, то убедись, что нужные методы в нем объявлены. Плюс стоит сделать конструктор приватным и пользоваться методами интерфейса
SquirrelLeonid
left a comment
There was a problem hiding this comment.
Предпроверку засчитываю.
- Посмотри новые комментарии
- Просьба: когда вносишь правки по комментарию - отпишись под ним, что было сделано (кратко). Это значительно упрощает ревью и дает понять, что комментарий был увиден
- Правки стоит разбивать на более мелкие коммиты. Когда все одной пачкой - проверять сложнее
| ts.MinFontSize == 12f && | ||
| ts.MaxFontSize == 72f), |
There was a problem hiding this comment.
Тут IDE подсказывает, что сравнение может давать погрешность (скажем, в 10-ом знаке после запятой). В таких случаях сравнение лучше проводить через сравнение модуля разности:
| actualValue - expectedValue | <= epsilon, где epsilon - какая-то маленькая константа
| } | ||
|
|
||
| private bool IsFlag(string arg) | ||
| private static Color? TryParseColor(string? colorStr) |
There was a problem hiding this comment.
'string?' не нужен, т.к. ты отключил nullable в csproj
| [Option("padding", HelpText = "Canvas padding (default if 50).")] | ||
| public int? Padding { get; set; } |
There was a problem hiding this comment.
Есть некоторые непонятки с тем, за что именно отвечает это свойство. Отступ от центра? Отступ от краев холста? Стоит уточнить имя
|
|
||
| namespace TagCloudGenerator.Algorithms | ||
| { | ||
| public class BasicTagCloudAlgorithm : ITagCloudAlgorithm |
There was a problem hiding this comment.
Только сейчас обратил внимание на имя класса. Я бы предложил в названии как-то сослаться на спиральную раскладку или что-то вроде того.
| private readonly Random random = new Random(); | ||
|
|
||
| private readonly List<Rectangle> rectangles = new List<Rectangle>(); | ||
|
|
||
| private double currentAngle = 0; | ||
| private double currentRadius = 0; | ||
|
|
||
| private int countToGenerate = 10; | ||
|
|
||
| private (int min, int max) width = new(20, 21); | ||
| private (int min, int max) height = new(20, 21); |
|
|
||
| namespace TagCloudGenerator.Infrastructure.Filters | ||
| { | ||
| public class ToLowerCaseFilter : IFilter |
There was a problem hiding this comment.
Помню, что сам предлагал вынести это в фильтры, но только сейчас понял, что под такие действия правильнее завести что-то вроде INormalizer.
Мы ведь не фильтруем здесь слова, а проводим какие-то действия. Так что будет хорошо ввести такую абстракцию
| public Size Measure(string word, float fontSize, string fontFamily) | ||
| { | ||
| using var font = new Font(fontFamily, fontSize); | ||
| using var bitmap = new Bitmap(1, 1); | ||
| using var graphics = Graphics.FromImage(bitmap); | ||
|
|
||
| var size = graphics.MeasureString(word, font); | ||
| return new Size((int)Math.Ceiling(size.Width), (int)Math.Ceiling(size.Height)); | ||
| } |
There was a problem hiding this comment.
На заметку. Здесь просматриваются два момента для оптимизации.
Во-первых, можно оптимизировать работу с памятью. Обрати внимание, что bitmap и graphics у тебя для любого случая одинаковые. Класс мог бы единожды определить их при создании и хранить на протяжении своего жизненного цикла. Но для этого он обязан реализовать DIspose и этот метод должен откуда-то вызываться, иначе наоборот получим утечку. Можешь попробовать реализовать это
Во-вторых, если бы сценарий предполагал наличие повторов вида слово-шрифт, то можно было бы ввести некоторый кэш, который перед расчетами проверял - было ли такое.
| @@ -0,0 +1,20 @@ | |||
| using TagCloudGenerator.Core.Interfaces; | |||
|
|
|||
| namespace TagCloudGenerator.Infrastructure.Reader | |||
| public void Render(IEnumerable<CloudItem> items, CanvasSettings canvasSettings, TextSettings textSettings, string outputFile) | ||
| { | ||
| var itemsList = items.ToList(); | ||
| if (items.Count() == 0) |
| private (int offsetX, int offsetY) CalculateOffset( | ||
| List<CloudItem> items, | ||
| CanvasSettings settings) | ||
| { | ||
| if (!settings.CenterCloud) | ||
| return (settings.Padding, settings.Padding); | ||
|
|
||
| var minX = items.Min(i => i.Rectangle.X); | ||
| var minY = items.Min(i => i.Rectangle.Y); | ||
| var maxX = items.Max(i => i.Rectangle.Right); | ||
| var maxY = items.Max(i => i.Rectangle.Bottom); | ||
|
|
||
| var cloudWidth = maxX - minX; | ||
| var cloudHeight = maxY - minY; | ||
|
|
||
| var offsetX = (settings.CanvasSize.Width - cloudWidth) / 2 - minX; | ||
| var offsetY = (settings.CanvasSize.Height - cloudHeight) / 2 - minY; | ||
|
|
||
| return (offsetX, offsetY); | ||
| } |
There was a problem hiding this comment.
Могу ошибаться, но кажется эта логика лишняя
- Координаты центра облака мы можем принимать как входной параметр (в собственных координатах)
- Расположение прямоугольников относительно центра мы уже посчитали заранее
- Фактически все элементы у нас уже должны быть размещены там, где надою. Нужно только привести их координаты к координатам холста. Это, по сути, можно делать налету для каждого тэга прямо перед вставкой
| It.IsAny<IEnumerable<CloudItem>>(), | ||
| It.IsAny<CanvasSettings>(), | ||
| It.IsAny<TextSettings>())) | ||
| .Returns(new Bitmap(1, 1)); |
There was a problem hiding this comment.
Вот тут у тебя утечка. Bitmap - это объект, требующий уничтожения после использования.
| private Mock<IFormatReader> reader1Mock; | ||
| private Mock<IFormatReader> reader2Mock; |
There was a problem hiding this comment.
Лучше словами. firstReaderMock, secondReaderMock
| using DocumentFormat.OpenXml.Packaging; | ||
| using DocumentFormat.OpenXml.Wordprocessing; | ||
| using NUnit.Framework; | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Text; | ||
| using System.Threading.Tasks; | ||
| using TagCloudGenerator.Infrastructure.Readers; |
There was a problem hiding this comment.
Не используемый using. В других местах тоже проверь
| <Project Sdk="Microsoft.NET.Sdk"> | ||
|
|
||
| <PropertyGroup> | ||
| <OutputType>WinExe</OutputType> | ||
| <TargetFramework>net8.0-windows</TargetFramework> | ||
| <Nullable>enable</Nullable> | ||
| <UseWindowsForms>true</UseWindowsForms> | ||
| <ImplicitUsings>enable</ImplicitUsings> | ||
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <ProjectReference Include="..\TagCloudGenerator\TagCloudGenerator.csproj" /> | ||
| </ItemGroup> | ||
|
|
||
| </Project> No newline at end of file |
There was a problem hiding this comment.
У тебя этот проект не включен в решение. Подключи его.
Могут быть конфликты из-за двух методов Main. Можешь удалить старый и в TagCloudGenerator.csproj в тэге OutputType сменить на Library, Либо сделать это через IDE (свойства проекта)
| <Project Sdk="Microsoft.NET.Sdk"> | ||
|
|
||
| <PropertyGroup> | ||
| <OutputType>Exe</OutputType> | ||
| <TargetFramework>net8.0</TargetFramework> | ||
| <ImplicitUsings>enable</ImplicitUsings> | ||
| <Nullable>enable</Nullable> | ||
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <ProjectReference Include="..\TagCloudGenerator\TagCloudGenerator.csproj" /> | ||
| </ItemGroup> | ||
|
|
||
| </Project> |
There was a problem hiding this comment.
Проект тоже не подключен к решению
| var result = wordFreqDictionary | ||
| .OrderByDescending(pair => pair.Value) | ||
| .ThenBy(pair => pair.Key) | ||
| .Select(pair => (pair.Key, pair.Value)) | ||
| .ToList(); |
There was a problem hiding this comment.
Вообще, есть некоторые сомнения касательно сортировки слов как либо.
Здесь просматривается введение в цепочку какого-то аналога INormalizer, но только для коллекции целиком, а не отдельных слов. Грубо говоря:
- Вычитай слова
- Нормализуй каждое из них
- Отфильтруй каждое из них
- Посчитай частоту оставшихся слов
- -- Отсортируй слова так или иначе---
- Помести в облако тегов
Тебе предложение на подумать.
| public BoringWordsFilter() { } | ||
|
|
||
| public BoringWordsFilter(string[] words) | ||
| { | ||
| boringWords = words; | ||
| } | ||
|
|
||
| public BoringWordsFilter(List<string> wordsList) | ||
| { | ||
| boringWords = wordsList.ToArray(); | ||
| } |
There was a problem hiding this comment.
Можно схлопнуть до одного конструктора, принимающего IEnumerable<sting> или ICollection<string>
|
|
||
| namespace TagCloudGenerator.Infrastructure.Readers | ||
| { | ||
| public class CompositeReader : IReader |
There was a problem hiding this comment.
Про него уже писал в интерфейсе
| { | ||
| using var doc = WordprocessingDocument.Open(filePath, false); | ||
|
|
||
| return doc.MainDocumentPart.Document.Body |
There was a problem hiding this comment.
Можешь свалиться с NullReference.
Офисные пакеты так устроены, что существование документа не гарантирует существование каких-либо элементов в нем. К примеру, можно программно создать абсолютно пустой docx контейнер, который будет валиден с точки зрения внутренней структуры.
| <ItemGroup> | ||
| <None Include="Infrastructure\Readers\TxtReader.cs" /> | ||
| </ItemGroup> | ||
|
|
There was a problem hiding this comment.
Вот это по-моему лишнее вообще. Класс ведь определен в этом же проекте
SquirrelLeonid
left a comment
There was a problem hiding this comment.
Оставил еще несколько комментариев.
Тем не менее задачу засчитываю на полный балл. Выполнены обязательные требования и несколько пунктов на перспективу
| words = ApplyFilters(words, filters); | ||
| if(words.Count == 0) return null; | ||
|
|
||
| var wordsWithFreq = _analyzer.Analyze(words); | ||
| var wordsWithFreq = _sorterer.Sort(_analyzer.Analyze(words)); |
There was a problem hiding this comment.
Предложение: применение фильтра и сортировку (да и в целом, наверное, любую предобработку слов) можно в целом утащить из CloudGenerator.
Тут можно по разному смотреть. Зависит от того, какой смысл вкладывать в метод Generate.
Решение за тобой
There was a problem hiding this comment.
Я решил что пусть лучше останется как есть, изначально я и задумал генератор как некоторое "ядро" в котором у меня будет собрана вся обработка и генерация
| if (!wordFreqDictionary.ContainsKey(word)) wordFreqDictionary.Add(word, 1); | ||
| else wordFreqDictionary[word]++; |
There was a problem hiding this comment.
Это можно заменить вызовом TryAdd (..., 1). Если не получилось, то делать инкремент. Так ты избежишь двойного прохода по словарю
| try | ||
| { | ||
| using var doc = WordprocessingDocument.Open(filePath, false); | ||
| if (doc == null) return new List<string>(); |
There was a problem hiding this comment.
Если прям хорошо провалиться в исходники WordprocessingDocument.Open(), то увидим, что null никогда не возвращается. Т.е. эта проверка всегда ложна.
Я имел ввиду, что NullReferenceException может быть при обращении к doc.MainDocumentPart - вот она точно может быть null
There was a problem hiding this comment.
А проверка то все равно нужна, просто на другое поле :)
Оставлю комментарий уже в рамках следующей задачи
| public IFormatReader TryGetReader(string filePath) | ||
| { | ||
| var reader = _readers.FirstOrDefault(r => r.CanRead(filePath)); | ||
| if (reader == null) | ||
| throw new NotSupportedException("Формат не поддерживается"); | ||
|
|
||
| return reader.TryRead(filePath); | ||
| return reader; |
There was a problem hiding this comment.
А является ли для ReadersRepository отсутствие подходящего читателя исключительным состоянием?
Вообще, для методов вида Try<что-то там> очень удобна такая сигнатура:
bool Try...(<Тип> param1, <Тип> param2, ..., out <Тип> outputResult)
Об успешности операции сообщается через булевое значение. При этом если ты ищешь что-то, например, в словаре, то возвращаемый результат передается через out параметр.
Таким образом на вызывающей стороне код будет выглядеть вот так:
if (Try...(param1, param2, out <Тип>outputResult))
// Делаем, что хотели
else
// Вот для внешнего кода это может быть исключительным состоянием. Можем кинуть Exception| /// <summary> | ||
| /// Тут пожалуй отвечу насчет использования списка кортежей, насколько я выяснил в интернете, в целом словари можно сортировать, | ||
| /// но вообще говоря Dictionary не гарантирует верный порядок элементов из-за особенностей своего устройства | ||
| /// В большинстве случаев - да, будет сортировать, но гарантий нам никто не дает, особенно на больших словарях | ||
| /// есть SortedDictionary, но он сортирует по ключу, поэтому я решил пойти таким путем - отделить сортировку от анализа частот | ||
| /// </summary> |
There was a problem hiding this comment.
Такие комментарии в коде оставлять не нужно. Это больше подойдет для ревью.
Комментарии стоит использовать либо для документации, либо пояснения каких-то нетривиальных вещей.
| /// </summary> | ||
| public class FrequencyDescendingSorterer : ISorterer | ||
| { | ||
| public List<(string Word, int Frequency)> Sort(Dictionary<string, int> wordsWithFreqs) |
There was a problem hiding this comment.
Принято, я сам запамятовал, что словарь не гарантирует порядок. Молодец, что не стал просто заменять на словарь.
There was a problem hiding this comment.
Выяснил случайно пока искал как это сделать, спасибо что предложил попробовать, теперь точно запомню что с сортировкой словарей есть нюансы
|
|
||
| Assert.IsNotNull(result); | ||
|
|
||
| result.Dispose(); |
There was a problem hiding this comment.
У IDE (во всяком случае у Rider) подсветка с ума сходит от того, что Bitmap (и в целом System.Drawing) доступен только на платформе windows. Вещь, конечно, правильная, т.к. на Unix такой код не запустится (там по другому устроены графические примитивы и работа с ними).
Но мы здесь не решаем проблему кроссплатформенности. Снять предупреждение можно, если в csproj файлах явно указать платформу под windows. В этом случае это будет вот так:
<TargetFramework>net8.0-windows</TargetFramework>
| Assert.That(result[0].Key, Is.EqualTo("hello")); | ||
| Assert.That(result[0].Value, Is.EqualTo(3)); | ||
| Assert.That(result[1].Key, Is.EqualTo("world")); | ||
| Assert.That(result[1].Value, Is.EqualTo(2)); | ||
| Assert.That(result[2].Key, Is.EqualTo("test")); | ||
| Assert.That(result[2].Value, Is.EqualTo(1)); |
There was a problem hiding this comment.
Вот здесь кстати хорошо видно, чем словарь не удобен. Раньше были конкретные поля Word и Frequency.
Предложение: Из WordsFrequencyAnalyzer возвращать не словарь строка-число, а список объектов, каждый из которых содержит слово и его частоту.
Решение за тобой
There was a problem hiding this comment.
Возможно неверно понял суть, но вроде как мы сознательно ушли от этого к использованию словаря и разделения на анализатор частоты и сортировщик слов по частоте.
Поскольку тут у меня есть выбор, я наверное оставлю как есть, мне кажется что названия и содержания тестов должно быть достаточно чтобы понять что анализатор хранит пары "слово - его частота"

No description provided.