Чечулин Антон #166
Чечулин Антон #166
Conversation
There was a problem hiding this comment.
В целом, неплохой код. Но видимо тебе уж слишком сильно понравился Result. :)
Стоит поправить его использование, следуя следующим правилам:
- Не хранить Result в полях класса
- Не возвращать перечисление Result (то есть, IEnumerable не надо) ((вообще, такое иногда можно, когда объекты Result это какие-то таски, треды или что-то подобное, что может сфейлиться. Но объект не может сфейлиться, фелится логика.))
| pointsOnSpiral = pointCreator.GetPoints().GetEnumerator(); | ||
| } | ||
|
|
||
| public IList<Rectangle> Rectangles { get => rectanglesInLayout; } |
There was a problem hiding this comment.
Можно чуть проще public IList<Rectangle> Rectangles => rectanglesInLayout;
There was a problem hiding this comment.
Ещё странно, что мы достали это поле наружу только ради тестов. Это не очень хорошая практика.
| public IEnumerable<Result<Point>> GetPoints() | ||
| { |
There was a problem hiding this comment.
Очень странно возвращать IEnumerable<Result>
Нам чтобы узнать ошибку, которая произошла, надо пройтись по коллекции, после чего проход все равно дальше первого элемента не пойдет, и мы будем дальше прокидывать ошибку.
Кажется, лучше будет Result<IEnumerable>
| pointsOnSpiral.MoveNext(); | ||
| if (!pointsOnSpiral.Current.IsSuccess) | ||
| return Result.Fail<Rectangle>(pointsOnSpiral.Current.Error); |
There was a problem hiding this comment.
Немного неочевидно, вытекает из того, что у нас IEnumerable
| graphics = Graphics.FromHwnd(IntPtr.Zero); | ||
| } | ||
|
|
||
| public IEnumerable<Result<Tag>> GetTags() |
There was a problem hiding this comment.
Та же самая история, IEnumerable вместо Result
| } | ||
| yield return new Tag(wordWithCount.Key, | ||
| fontSize, | ||
| rectangle.GetValueOrThrow(), |
There was a problem hiding this comment.
А почему тут GetValueOrThrow вместо просто Value?
| if (isMatch) | ||
| color = Color.FromName(settings.Color); |
There was a problem hiding this comment.
Немного странный подход к выбору из коллекции. Кажется, что объекты должны хранить в себе свое имя и совпадение должно проверяться вне объектов, а не изнутри.
| private readonly Result<Point> center; | ||
| private readonly Result<double> deltaAngle; | ||
| private readonly Result<double> deltaRadius; |
|
|
||
| public class BackgroundSettings | ||
| { | ||
| public Result<Color> BackgroundColor { get; } |
| private readonly Result<IEnumerable<string>> words; | ||
| private readonly Result<HashSet<string>> boringWords; |
There was a problem hiding this comment.
В общем, по сути, мы никогда не должны хранить Result как поле. Мы либо получаем нужное значение и присваиваем его полю, либо кидаем ошибку. То есть Result у нас должен только возвращаться из методов, но никогда не должен быть активной частью внутренней логики. Это как вешать TryCatch на поля.
|
|
||
| namespace TagsCloudVisualization.CommandLine; | ||
|
|
||
| public class CommandLineOptions |
There was a problem hiding this comment.
По хорошему, все, что работает с командной строкой, должно лежать в отдельном проекте. Но раз уж прошлый наставник это допустил, то я не буду просить переделывать. :)
| public Result<bool> Check() | ||
| { | ||
| if (Width <= 0) | ||
| return Result.Fail<bool>($"Width must be positive, but {Width}"); | ||
| else if (Height <= 0) | ||
| return Result.Fail<bool>($"Height must be positive, but {Height}"); | ||
| return Result.Ok(true); |
There was a problem hiding this comment.
Не очень хорошо. В настройках должны лежать только корректные данные, ошибки надо прокидывать на моменте создания/присваивания полей. Либо ошибки кидать на моменте использования, потому что некоторые ошибки только в процессе работы программы становятся явными. И в любом случае - проверка корректности настроек вне настроек, а не в них.
| public Result<bool> Check() | ||
| { | ||
| if (Enum.IsDefined(typeof(KnownColor), BackgroundColor)) | ||
| return Result.Ok(true); | ||
| return Result.Fail<bool>($"Can't find color with name {BackgroundColor} for background"); | ||
| } |
| public Result<bool> CheckForCorrectness() | ||
| { | ||
| var imageSettingsCheck = imageSettings.Check(); | ||
| if (!imageSettingsCheck.IsSuccess) | ||
| return Result.Fail<bool>(imageSettingsCheck.Error); | ||
| var spiralSettingsCheck = spiralSettings.Check(); | ||
| if (!spiralSettingsCheck.IsSuccess) | ||
| return Result.Fail<bool>(spiralSettingsCheck.Error); | ||
| return Result.Ok(true); | ||
| } |
| var checkSettings = settings.Check(); | ||
| if (!checkSettings.IsSuccess) | ||
| return Result.Fail<IList<Tag>>(checkSettings.Error); |
| var isPointCreatorCorrect = pointCreator.CheckForCorrectness(); | ||
| if (!isPointCreatorCorrect.IsSuccess) | ||
| return Result.Fail<Rectangle>(isPointCreatorCorrect.Error); |
| public Result<bool> Check() | ||
| { | ||
| if (DeltaRadius <= 0) | ||
| return Result.Fail<bool>($"Delta radius must be positive, but {DeltaRadius}"); | ||
| if (DeltaAngle <= 0) | ||
| return Result.Fail<bool>($"Delta angle must be positive, but {DeltaAngle}"); | ||
| return Result.Ok(true); | ||
| } |
| public Result<bool> Check() | ||
| { | ||
| if (!IsCanGetFontFamily(FontFamily)) | ||
| return Result.Fail<bool>($"Font with name {FontFamily} doesn't supported"); | ||
| else if (MaxSize < MinSize) | ||
| return Result.Fail<bool>($"Max font size can't be less then min font size"); | ||
| else if (MaxSize <= 0) | ||
| return Result.Fail<bool>($"Font sizes must be positive, but max size: {MaxSize}"); | ||
| else if (MinSize <= 0) | ||
| return Result.Fail<bool>($"Font sizes must be positive, but min size: {MinSize}"); | ||
| return Result.Ok(true); | ||
| } |
| public Result<bool> Check() | ||
| { | ||
| if (!File.Exists(PathToBoringWords)) | ||
| return Result.Fail<bool>($"Cant't find file with this path {Path.GetFullPath(PathToBoringWords)}"); | ||
| if (!File.Exists(PathToText)) | ||
| return Result.Fail<bool>($"Cant't find file with this path {Path.GetFullPath(PathToText)}"); | ||
| return Result.Ok(true); | ||
| } |
@MrTimeChip