Мажирин Александр #21
Conversation
| .WithParsed(opts => | ||
| { | ||
| cliHandler.RunOptionsAndReturnExitCode(opts); | ||
| if (opts.Demo) |
There was a problem hiding this comment.
Если вдруг дальше (при большом количестве всяких флагов) с таким подходом станет тяжко - посмотри вот на эту библиотеку, мне кажется, что с ней может быть удобнее
|
Текущий сценарий использования (из Demo) не сказать, что удобный. Надо знать много всего про библиотеку, чтобы разобраться Будет круто, если придумаешь библиотеке фасад. Представь, что тебе надо написать доку раздел "Get Started". Хотелось бы видеть там что-то типа // конфигурация
Action<TagCloudOptions> optionsBuilder = options =>
{
options
.UseGraphics<MyCustomGraphics>()
// ... some another config
.UseWordProcessor<SomeWordProcessor>();
};
// использование
string[] words = // какие-то слова
var tagCloud = new TagCloud(optionsBuilder);
byte[] image = tagCloud.From(words);
// из docx
byte[] content;
var image = tagCloud.FromDocx(content);ну или на что у тебя хватит фантазии и представления об удобстве 😊 |
| }; | ||
| }).SingleInstance(); | ||
| }) | ||
| .Build(); |
There was a problem hiding this comment.
Как-то кажется, что перемудрил.
вот так попроще
var cliOptions = Parser.Default.ParseArguments<CliOptions>(args).Value;
var tagCloudConfig = new TagCloudConfig
{
FontFamily = cliOptions.FontFamily,
// ...
};
builder.RegisterInstance(tagCloudConfig);| var line = _outputReader.ReadLine(); | ||
| if (line == null) | ||
| { | ||
| throw new InvalidEnumArgumentException("MyStem returned null"); |
There was a problem hiding this comment.
Тут бы ошибки прочитать из StandardError. Не понятно что пошло не так
|
|
||
| var options = builder.Build(); | ||
|
|
||
| var childScope = _scope.BeginLifetimeScope(cb => { cb.RegisterModule(new TagCloudModule(options)); }); |
There was a problem hiding this comment.
Все-таки хорошо бы организовать код так, чтобы регистрировать можно было в основной контейнер без создания дочерних скоупов (можно отказаться от модуля Autofac и сделать, как обычно бывает, экстеншн методы для регистрации). Думаю, что работать с библиотекой в этом случае станет сильно проще (понятнее станет время жизни сервисов).
Фабрика у тебя Scoped, из-за дорегистрации зависимостей в дочернем скоупе, реализации библиотечных интерфейсов (регистрируешь как Singleton) тоже становятся по сути Scoped. Кажется, что с таким подходом мы просто создаем много лишнего мусора.
|
|
||
| public SortOrder SortOrder { get; set; } = SortOrder.Descending; | ||
|
|
||
| public int MaxWordsCount { get; set; } = 50; |
There was a problem hiding this comment.
Это вроде обычный норм код, так пишут. Но здесь плохая инкапсуляция, сеттеры по умолчанию должны быть приватными. А в данном случае настройки могли бы быть вообще private readonly
|
|
||
| public class MyStemTextProcessor : ITextProcessor | ||
| { | ||
| private ILogger<MyStemTextProcessor> _logger; |
| var minWeight = words.Values.Min(); | ||
| var maxWeight = words.Values.Max(); | ||
|
|
||
| return words.Select(word => |
There was a problem hiding this comment.
прекрасно заменяется на foreach => + к читаемости
| { | ||
| if (maxWeight == minWeight) | ||
| return _minFontSize; | ||
| var adjustedFontSize = (float)(_minFontSize + (_maxFontSize - _minFontSize) * (wordValue - minWeight) / (maxWeight - minWeight)); |
There was a problem hiding this comment.
норм, можно было еще попробовать логарифм
|
|
||
| public SKImage DrawTags(IEnumerable<Tag> tags) | ||
| { | ||
| var tagList = tags.ToList(); |
There was a problem hiding this comment.
Просто перекопировал тэги в новый список, а зачем? Тут надо подумать, ты подразумеваешь, что может быть такой алгоритм, который может быть оптимизирован засчет ленивой работы с тэгами?
Мне кажется тут надо принимать IReadOnlyCollection и не будет проблем)
| var startY = int.MaxValue; | ||
| foreach (var tag in tags) | ||
| { | ||
| endX = Math.Max(endX, (int)tag.BBox.Right); |
There was a problem hiding this comment.
var endX = tags.MaxBy(x => x.BBox.Right).BBox.Right;
мб так идея прозрачнее
| return new Rectangle(startX, startY, endX, endY); | ||
| } | ||
|
|
||
| private void ValidateRectangle(Rectangle rectangle) |
There was a problem hiding this comment.
Почитай про бедные и богатые модели. Этот метод должен быть не здесь
No description provided.