-
Notifications
You must be signed in to change notification settings - Fork 201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Юдин Павел #172
base: master
Are you sure you want to change the base?
Юдин Павел #172
Conversation
TagsCloud/FileReader/FileReader.cs
Outdated
return File.Exists(filePath) | ||
? parsers.TryGetValue(Path.GetExtension(filePath).Trim('.'), out var parser) | ||
? Result.Ok<IEnumerable<string>>(parser.GetWordList(filePath)) | ||
: Result.Fail<IEnumerable<string>>($"not found parser for {Path.GetExtension(filePath).Trim('.')}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Представь, что ты, как пользователь, видишь сообщение, что "этот тип не поддерживается". Не возникнет ли у тебя логичного вопроса, а какие типы тогда вообще поддерживаются? В тексте ошибки круто писать не только ошибку, но и способ её возможного устранения или место, где его можно найти.
Условно, в этом месте можно было бы сразу написать, какие типы поддерживаются, чтобы пользователь сразу понял, что первый файл не подошел, и он сразу без раскопок второй попыткой отправит файл с нужным расширением
var brush = new SolidBrush(tag.Color); | ||
if (IsRectangleOutOfBounds(tag.Rectangle, sizeImage)) | ||
{ | ||
return Result.Fail<None>("Облако тегов вышло за границы изображения."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Опять же давай посмотрим со стороны пользователя, а какой размер он должен поставить, видя такой текст ошибки?
Рассуждения: раз вышло, значит должен сделать больше, хм, в два раза? Сделал в два раза больше. Либо снова не влезло, либо наоборот слишком большой фон с небольшим рисунком получился. Начинаю экспериментировать с размерами, чтобы получить наиболее удачный.
А что если мы подскажем пользователю сразу оптимальный размер? Чтобы он не гадая знал, какой минимальный размер для облака ему нужен
SettingsForm.For(tag).ShowDialog(); | ||
|
||
if (!fonts.Contains(tag.FontFamily.ToLower())){ | ||
ErrorHandler.HandleError($"Шрифт {tag.FontFamily} не найден"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А не проверял случайно, если написать ARiaL
, то шрифт норм отработает? Так как ты приводишь к нижнему регистру, то твои валидации пройдут успешно, но найдется ли потом такой шрифт для отрисовки?
public class FlowerCloudAction : IUiAction | ||
{ | ||
private readonly FlowerSpiral flowerSpiral; | ||
private readonly IImageHolder imageHolder; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не используется
public MenuCategory Category => MenuCategory.Types; | ||
public string Name => "Цветок"; | ||
public string Description => ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Неконсистентно немного получилось, в других экшнах эти свойства стояли до Perform
var excludedSpeeches = WordAnalyzerHelper.GetConvertedSpeeches(Settings.ExcludedSpeeches); | ||
var selectedSpeeches = WordAnalyzerHelper.GetConvertedSpeeches(Settings.SelectedSpeeches); | ||
var morphAnalyzer = new MorphAnalyzer(true); | ||
var morphInfos = Result.Of(() =>morphAnalyzer.Parse(words)); | ||
return !morphInfos.IsSuccess ? Result.Fail<IEnumerable<string>>("Ошибка внутренней библиотеки Deep Morphy") : Result.Ok(morphInfos.Value.Where(morphInfo => | ||
!Settings.BoringWords.Any(item => | ||
item.Equals(morphInfo.BestTag.Lemma, StringComparison.OrdinalIgnoreCase)) && | ||
!excludedSpeeches.Contains(morphInfo.BestTag["чр"]) && | ||
(selectedSpeeches.Count == 0 || | ||
selectedSpeeches.Contains(morphInfo.BestTag["чр"]))) | ||
.Select(info => info.BestTag.HasLemma ? info.BestTag.Lemma : info.Text)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не стесняйся давать коду "подышать". Ставь пустые строки между логическими блоками кода. Потому что сейчас это выглядит как лютое месиво)) Давай попробуем тут прибраться
- Тернарики принято разносить построчно, так проще читается
- Сложные конструкции и проверки из экшна функций (всяких Where и Select) лучше выносить в отдельные методы также в угоду читаемости
var selectedSpeeches = WordAnalyzerHelper.GetConvertedSpeeches(Settings.SelectedSpeeches); | ||
var morphAnalyzer = new MorphAnalyzer(true); | ||
var morphInfos = Result.Of(() =>morphAnalyzer.Parse(words)); | ||
return !morphInfos.IsSuccess ? Result.Fail<IEnumerable<string>>("Ошибка внутренней библиотеки Deep Morphy") : Result.Ok(morphInfos.Value.Where(morphInfo => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ошибка так же не подсказывает пользователю, что с ней делать. Быть может библиотека возвращает свою собственную ошибку, которая будет лежать в твоем Result? А если ещё и ссылку на документацию добавим в текст, то вообще загляденье будет
@desolver