Conversation
add result pattern in all outer methods
SquirrelLeonid
left a comment
SquirrelLeonid
left a comment
There was a problem hiding this comment.
К подходу с точки зрения result вопросов нет, но есть несколько небольших замечаний к коду
TagsCloudContainer.Cli/Client.cs
Outdated
|
|
||
| public class Client | ||
| { | ||
| [SuppressMessage("Interoperability", "CA1416:Проверка совместимости платформы")] |
There was a problem hiding this comment.
Лучше не подавлять сообщения таким образом, а в csproj явно указать платформу как net8.0-windows. Так ты четко сообщишь, что твой код не кроссплатформенный.
А так ты рискуешь в лучшем случае словить ошибки при компиляции под unix. В худшем - развалится в рантайме
There was a problem hiding this comment.
Подобные вхождения убрал, везде вписал net8.0-windows
| .AddVisualizationOptions(options.BackgroundColor, options.TextColor, options.FontSize, | ||
| options.OutputWidthPx, options.OutputHeightPx, options.FontFamily) |
There was a problem hiding this comment.
Небольшое отступление - некоторые моменты будут не по самой задаче с добавлением Result. но комментарии я по ним все же оставлю.
Можно подумать над тем, чтобы ввести промежуточный класс с опциями. В нем можно было бы провести группировку отдельных опций, которые управляют конкретной частью. В данном случае это могут быть VisualizationOptions.
TagsCloudContainer.Cli/Startup.cs
Outdated
| .AddFileReaders() | ||
| .AddWordsFilter(options.FilterFilePath) | ||
| .AddWordsProcessor() | ||
| .AddCoordinateGenerators(options.OutputWidthPx ?? 100, options.OutputHeightPx ?? 100, options.AngleStepRadians) |
There was a problem hiding this comment.
Я бы не оставлял значения по умолчанию в логике инициализации контейнера.
There was a problem hiding this comment.
Убрал центр из конструктора координатных генераторов
TagsCloudContainer.Cli/Client.cs
Outdated
| }) | ||
| .Then(data => FileSaver.SaveFile(data.bitmap, data.output)) | ||
| .Then(output => Console.WriteLine($"Visualization saved to file {output}")) | ||
| .OnFail(Console.WriteLine); |
There was a problem hiding this comment.
Тут вот не совсем прозрачно что выведется. Просто пустая строка?
There was a problem hiding this comment.
Сделал более явный вывод ошибок, заменив на
error => Console.WriteLine(error);
TagsCloudContainer.Cli/Client.cs
Outdated
| { | ||
| var output = options.OutputFilePath ?? | ||
| Path.Combine(Environment.CurrentDirectory, | ||
| $"TagsCloud_{DateTime.UtcNow:yyyy-MM-dd_HH-mm-ss}.png"); |
There was a problem hiding this comment.
Кажется здесь можно писать не строго .png, а выводить формат из bitmap (раз он приходит на вход)
There was a problem hiding this comment.
Не совсем понял откуда взять формат у bitmap, формат изображения вычисляется в FileSaver на основе расширения пути создаваемого файла, а у битмапы не нашел никакого поля с форматом.
There was a problem hiding this comment.
У Bitmap есть поле RawFormat, а в нем хранится ImageFormat. Думаю можно оттуда взять
There was a problem hiding this comment.
Добавил метод для вычисления extension на основе ImageFormat, теперь беру его из Bitmap.RawFormat
| var bitmap = CalculateCloudBounds(wordLayouts).AsResult() | ||
| .Then(bounds => CreateBitmapForCloud(wordLayouts, visualizationOptions.ImageWidthPx, | ||
| visualizationOptions.ImageHeightPx, visualizationOptions.Padding, bounds)) | ||
| .Then(bitmap => DrawWordsOnBitmap(bitmap, wordLayouts, visualizationOptions)); | ||
| return bitmap; |
There was a problem hiding this comment.
Здесь хочу еще раз подсветить тебе момент владения объектом. Битмап возникает здесь и передается куда-то наружу. Здесь выходит неявная передача владения.
Вот тут я сошлюсь на комментарий про умный указатель.
Реализовывать его не нужно, но внимательнее с такими местами в будущем
|
|
||
| if (bounds.Width > imageWidthPx.Value || bounds.Height > imageHeightPx.Value) | ||
| return Result.Fail<Bitmap>( | ||
| "Cloud bounds are larger than image size. Try to increase imageSize, or leave them empty for auto calculation"); |
There was a problem hiding this comment.
Тут кстати по разному можно трактовать. Для рисования это не то чтобы ошибка, потому что объекты вне границ изображения будут просто обрезаны
| } | ||
| else | ||
| { | ||
| var cloudBounds = CalculateCloudBounds(wordLayouts); |
There was a problem hiding this comment.
Можно считать перед if, тогда внутри DrawWordsWithFixedSIze можно этот вызов убрать
| foreach (var (word, frequency) in sortedWords) | ||
| { | ||
| var fontSize = CalculateFontSize(frequency, maxFrequency); | ||
| var wordSize = GetWordSize(word, fontSize, _visualizationOptions.FontFamily); |
There was a problem hiding this comment.
Здесь потенциально NullReferenceException, если _visualizationOptions не был задан
There was a problem hiding this comment.
Сделал string вместо string?, оказывается там и так все это время было значение по умолчанию внутри самого VisualizationOptions
| public static Result<string> SaveFile(Bitmap bitmap, string path) | ||
| { | ||
| var extension = Path.GetExtension(path).ToLower(); | ||
| var imageFormat = extension.GetImageFormat(); | ||
| if (!imageFormat.IsSuccess) | ||
| return Result.Fail<string>(imageFormat.Error); | ||
| bitmap.Save(path, imageFormat.Value); | ||
| return Result.Ok(path); | ||
| } |
There was a problem hiding this comment.
С текущим подходом в этом методе должен быть безусловный вызов Dispose для bitmap
There was a problem hiding this comment.
Если вызов extension.GetImageFormat не будет успешным, то bitmap мы не освободим. Освобождать его надо в любом случае
There was a problem hiding this comment.
Стал собирать Result через условия, потом освобождать битмап, и только потом возвращать result
|
Задачу засчитываю на полный балл |
2 participants
No description provided.