Зубков Андрей #164
Зубков Андрей #164
Conversation
| var text = document.GetText(); | ||
|
|
||
| return text.Split(new char[] { '\r', '\n' }, StringSplitOptions.RemoveEmptyEntries).Skip(1); | ||
| } | ||
|
|
||
| public IList<string> GetAvailableExtensions() => new List<string>() { "doc", "docx" }; | ||
| private bool FileExists(string inputPath, out string error) |
There was a problem hiding this comment.
С out параметрами нужно быть осторожным, они не являются панацеей. out параметры нельзя использовать, если метод асинхронный, компилятор не даст даже написать такой метод, а асинхронного кода в продакшне будет предостаточно. Если есть хоть небольшой намек на то, что метод может стать асинхронным, лучше сразу перейти на использование других подходов.
Тут, например, можно было использовать все тот же паттерн Result
TagCloud/FileReader/TxtReader.cs
Outdated
| } | ||
|
|
||
| public IList<string> GetAvailableExtensions() | ||
| private bool FileExists(string inputPath, out string error) |
There was a problem hiding this comment.
Хм, точно такой же метод находится в классе DocReader, один-в-один, давай вынесем в отдельный статический класс-утилиту и заиспользуем в двух местах
| throw new ArgumentException($"{generatorName} layouter is not supported"); | ||
| return registeredGenerators.ContainsKey(generatorName) | ||
| ? Result.Ok(registeredGenerators[generatorName]) | ||
| : Result.Fail<IPointGenerator>($"{generatorName} layouter is not supported"); |
There was a problem hiding this comment.
Тут было бы прикольно ещё дать пользователю сразу подсказку, а какие тогда поддерживаются?
TagCloud/Drawer/CloudDrawer.cs
Outdated
| if (!palette.IsSuccess) | ||
| return Result.Fail<Bitmap>(palette.Error); | ||
|
|
||
| var layouter = new CloudLayouter(pointGenerator.Value); | ||
| var tags = PlaceWords(words, layouter); | ||
|
|
||
| return !ValidateImageBorders(tags) | ||
| ? Result.Fail<Bitmap>( | ||
| $"Tags don't fit to given image size of {appSettings.CloudWidth}x{appSettings.CloudHeight}") |
There was a problem hiding this comment.
Аналогичный момент, а что нужно сделать пользователю, чтобы это исправить? Уменьшить / увеличить размеры? Может быть сразу подсказать, какой размер изображения получился сейчас?
TagCloud/Drawer/PaletteProvider.cs
Outdated
| { | ||
| return palettes.ContainsKey(paletteName) | ||
| ? Result.Ok(palettes[paletteName]) | ||
| : Result.Fail<IPalette>($"Palette with name {paletteName} doesn't exist"); |
There was a problem hiding this comment.
А какие палетки можно использовать? Этот текст ошибки не дает ответ на этот вопрос
| var extension = inputPath.Split(".").Last(); | ||
| return readers.ContainsKey(extension) | ||
| ? Result.Ok(readers[extension]) | ||
| : Result.Fail<IFileReader>($"Reading of file {inputPath} with extension {extension} is not supported"); |
| { | ||
| public IEnumerable<(string word, int rank)> RankWords(IEnumerable<string> words) | ||
| { | ||
| return words.GroupBy(word => word.Trim().ToLowerInvariant()).OrderByDescending(g => g.Count()).ToList() |
There was a problem hiding this comment.
А зачем Ranker триммит и приводит к нижнему регистру слова, если это уже сделал (или должен был сделать) препроцессор? Если оно должно существовать здесь, то не очень понятна ответственность препроцессора, зачем он нужен в принципе
| return settings.Value; | ||
| } | ||
|
|
||
| public static ContainerBuilder ConfigureBuilder(IAppSettings settings) |
There was a problem hiding this comment.
Сейчас все сервисы регистрируются по умолчанию в скоупе InstancePerDependency, это значит что при каждом запросе сервиса будет создаваться его экземпляр, даже если у него нет состояния. С таким подходом и увеличением количества сервисов можем начать проигрывать в памяти и скорости выполнения запросов, так как перед выполнением запроса нужно будет создать все зависимости заново.
Предлагаю чуть пооптимизировать и попробовать некоторые зависимости перевести в другие скоупы, в том числе синглтон
@desolver