Conversation
MrTimeChip
left a comment
MrTimeChip
left a comment
There was a problem hiding this comment.
В целом, предпроверка на максимальный балл, но над многим стоит подумать и переделать)
Нравится, что код чистый, но вот архитектурные моменты стоит подправить, разграничить ответственность.
| { | ||
| public IEnumerable<string> GetInterestingWords(string path, IDullWordChecker dullWordChecker) | ||
| { | ||
| var readText = File.ReadAllText(path); |
There was a problem hiding this comment.
Сейчас тут просто вылетает эксепшен, что файл не найден, плюс нигде не написано, какой путь надо прописать до файла (абсолютный, относительный и т.д.)
Давай здесь выбрасывать другой эксепшен с комментарием и понятным разъяснением?
TagsCloudVisualization/Program.cs
Outdated
| [Argument(0)] [Required] public string InputFilePath { get; set; } | ||
|
|
||
| [Argument(1)] [Required] public string OutputFileName { get; set; } |
There was a problem hiding this comment.
Когда вот этих обязательных аргументов нет, вылетает просто, что их нет, без пояснений, что, как, почему.
Плюс, непонятно, как мне узнать, какие аргументы вообще нужны, надо хотя бы инструкцию, чтобы не лазить в код ради этого)
| [Option("-fs")] public int FontSize { get; set; } = 50; | ||
|
|
||
|
|
||
| private void OnExecute() |
There was a problem hiding this comment.
Сейчас почти все сервисы добавлены синглтоном. Вопрос - почему?)
По умолчанию, нужно начинать с самого маленького скоупа, понимать, подходит ли он, если нет, подумать над большим скоупом, пока не станет ясно, какой подходит. А здесь ситуация обратная - решили сразу поставить максимальный скоуп, хотя каких-то предпосылок я здесь не вижу. Это как по умолчанию все переменные делать глобальными.
TagsCloudVisualization/Program.cs
Outdated
| services.AddSingleton<IDullWordChecker, NoWordsDullChecker>(); | ||
| services.AddSingleton<ICloudLayouter>(x => | ||
| new CloudLayouter(x.GetRequiredService<IPointGenerator>(), | ||
| x.GetRequiredService<Font>(), |
There was a problem hiding this comment.
Здесь какой-то странное использование DI)
Суть DI в том, что он сам может резолвить зависимости, а ты сюда передаешь их напрямую. Зачем?)
| private class TestDullChecker : IDullWordChecker | ||
| { | ||
| public bool Check(string word) | ||
| { | ||
| var testDullWords = new HashSet<string>() { "this", "is" }; | ||
| return testDullWords.Contains(word); | ||
| } | ||
| } |
There was a problem hiding this comment.
Почему для теста отдельный класс создаем?)
Его же и тестируем по итогу.
| public class CloudLayouter : ICloudLayouter | ||
| { | ||
| private readonly IPointGenerator pointGenerator; | ||
| private Dictionary<string, Font> wordsFonts = new(); |
There was a problem hiding this comment.
Кажется, что использование Font и других графических сущностей - вообще не обязанность Layouter'а.
Он, собственно, должен раскладывать прямоугольники, а не вот это вот всё.
| using var smallBitmap = new Bitmap(1, 1); | ||
| using var graphics = Graphics.FromImage(smallBitmap); | ||
| var rectangleSize = graphics.MeasureString(wordFont.Key, wordFont.Value); |
There was a problem hiding this comment.
Здесь тот же момент. Не должно быть здесь работы с графикой. Графика должна рисовать готовую раскладку, а не делать такие странные махинации при раскладке.
| private class JsonWordAnalysis | ||
| { | ||
| public string Text { get; set; } | ||
| public List<Dictionary<string, string>> Analysis { get; set; } | ||
| } |
There was a problem hiding this comment.
Вот есть вопрос насколько вообще правильно создавать такие классы. Просто делать его общим тоже не хочется из-за того как изначальный json выглядит и парсится, а именно: полезная инфа в словаре, а словарь в листе. И каждый раз его доставать звучит ужасно. Поэтому я десериализую в этот приватный класс, а затем уже преобразую в публичный который нормально выглядит
There was a problem hiding this comment.
В целом, такое приемлемо, но, конечно, по возможности стоит избегать. Но в данном контексте - более чем нормально.
TagsCloudVisualization/Program.cs
Outdated
| if (SqareAlgorithm) | ||
| services.AddTransient<IPointGenerator, LissajousCurvePointGenerator>(); | ||
| else | ||
| services.AddTransient<IPointGenerator, SpiralPointGenerator>(); |
There was a problem hiding this comment.
Вот это тоже плохо выглядит, но не смог нагуглить как привязывать сервис в зависимости от условия в microsoft dependency injection
There was a problem hiding this comment.
В целом, нет такой вещи, как зарегистрировать или не зарегистрировать в зависимости от условия.
Тебе надо зарегистрировать PointGenerator'ы все разом, а потом в отдельном классе ты их принимаешь массивом и производишь выбор в зависимости от значения параметра.
TagsCloudVisualization/Program.cs
Outdated
| { "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; } |
There was a problem hiding this comment.
Как-то не очень это расширяемо, а если третий алгоритм добавить?
TagsCloudVisualization/Program.cs
Outdated
| private void OnExecute() | ||
| { | ||
| var services = new ServiceCollection(); | ||
| services.AddTransient<Font>(x => new Font(FontFamily, FontSize)); |
There was a problem hiding this comment.
Ладно, здесь немного со скоупом вышел перебор, пожалуй.
Вообще, по дефолту используют Scoped.
TagsCloudVisualization/Program.cs
Outdated
| new MystemDullWordChecker(RemovedPartsOfSpeech, ExcludedWordsFile)); | ||
| services.AddScoped<IInterestingWordsParser, MystemWordsParser>(); | ||
| services.AddScoped<IRectangleLayouter>( | ||
| x => new RectangleLayouter(x.GetKeyedService<IPointGenerator>(PointGenerator))); |
There was a problem hiding this comment.
Интересная штука, KeyedService новая вещь. Но стоит посмотреть, как её правильно использовать.
https://weblogs.asp.net/ricardoperes/net-8-dependency-injection-changes-keyed-services
| { | ||
| layoutDrawer | ||
| .CreateLayoutImageFromFile(InputFilePath, new Size(ImageWidth, ImageHeight), MinimalFontSize) | ||
| .SaveImage(OutputFilePath, SaveImageFormat); | ||
| } |
There was a problem hiding this comment.
Не уверен насколько сейчас рационально выносить эти поля в TagLayoutSettings, так как сейчас мы не прячем ничего внутрь метода
2 participants
@MrTimeChip