Conversation
…ттерна Result<T>, поправил тесты, добавил новые тесты
…сс, исправил ошибку
w1jtoo
left a comment
w1jtoo
left a comment
There was a problem hiding this comment.
Не хватило выполнения такого требования: у тебя в коде не должно быть исключений, совсем. И в тестах это должно быть отражено.
Конечно, за исключением GetValueOrThrow :)
- по идее, везде, где у тебя используется
Resultу тебя должен быть тест на состояние Failed. Но это не обязательно, но хотелось бы)
TagCloud/TagCloud.csproj
Outdated
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <ProjectReference Include="D:\.NET_projects\fp\ErrorHandling\ErrorHandling.csproj" /> |
There was a problem hiding this comment.
У меня не сработает, да и почти у всех кроме тебя => даже запустить не получится
There was a problem hiding this comment.
Поменял на относительный путь
TagCloud/ReadingFiles/TxtReader.cs
Outdated
|
|
||
| public class TxtReader : IReader | ||
| { | ||
| public ImmutableHashSet<string> AvailableExtensions { get; } = ImmutableHashSet.Create<string>(".txt"); |
There was a problem hiding this comment.
nit: по смыслу скорее Supported. Available вроде типа "доступный" (как кабинка туалета), а "supported" типа поддержанный.
| var fileExtension = Path.GetExtension(pathToFile); | ||
| if (!_readers.TryGetValue(fileExtension, out var reader)) | ||
| return Result.Fail<Func<IEnumerable<string>>>($"Не найден подходящий {nameof(IReader)} для файла с расширением {fileExtension}"); | ||
| if (!Path.Exists(pathToFile)) |
There was a problem hiding this comment.
У тебя здесь пара не логичных моментов:
GetReaderсодержит в себе логику из любогоIReaderTxtReader.ReadTextByLine(...)кидает исключение
И это плохо. Представь, что у тебя будет низкоуровневое исключение, оно будет падать только в момент чтения файла (например закончили дескрипторы). Тогда твоя программа развалится с треском.
Аналогично, представь, что завтра у тебя появится чтение из сетевой папки, например, по FTP. Там вообще свой вагон и маленькая тележка исключительных ситуаций. И твой код будет очень сложно расширить.
Подумай, как исправить это
There was a problem hiding this comment.
- Добавил функцию для валидации чтения файла, которая возвращает ActionStatus
- Избавился от исключений в TxtReader.ReadTextByLine(...)
| Arguments = $"-ling {tempFile}", | ||
| } | ||
| }; | ||
| process.Start(); |
There was a problem hiding this comment.
Вот в этой строчке скрывается куча исключений, на самом то деле. По хорошему, их бы тоже обработать.
Самая понятная из них, что есть mystem упал. Его же тоже программисты писали, могли где-нибудь ошибиться. Можно такой кейс обработать и, например, сообщить пользователю в UI, что литературная обработка не доступна.
There was a problem hiding this comment.
А ещё процессов может не быть свободных. Или mystem будет работать оч. долго... Кейсов куча, все из них сейчас не покрыть, но можно уже заложиться на будущее.
There was a problem hiding this comment.
- Добавил обработку исключений в работе mystem, читаю поток вывода ошибок процесса
- Добавил таймаут 5 сек. на парсинг файла, если работает дольше возвращается ActionStatus с сообщением об ошибке
| @@ -0,0 +1,7 @@ | |||
| namespace TagCloud; | |||
|
|
|||
| public interface ICrrectnessChecker | |||
There was a problem hiding this comment.
А яндекс переводчик так не считает
There was a problem hiding this comment.
Должно быть cOrrectness, у тебя crrectness :)
TagCloud/CrrectnessChecker.cs
Outdated
| @@ -0,0 +1,13 @@ | |||
| namespace TagCloud; | |||
|
|
|||
| public abstract class CrrectnessChecker : ICrrectnessChecker | |||
There was a problem hiding this comment.
А почему бы тогда сюда DTOшку сразу не положить? Я бы конечно отдельно сделал, без наследований (отдельно валидаторы, отдельно коробочки с данными). Но в твоём кейсе неплохо в таком месте сразу данные хранить
There was a problem hiding this comment.
Переименовал на CrrectnessCheckerBase
TagCloud.Tests/WordInfoTests.cs
Outdated
| { | ||
| var partOfSpeech = elem; | ||
| var initialization = () => new WordInfo(word, partOfSpeech, count); | ||
| initialization.Should().Throw<ArgumentException>(); |
There was a problem hiding this comment.
Мы же как раз от исключений отказываемся. Может Result должен быть?
There was a problem hiding this comment.
Сделал конструктор WordInfo приватным, создание объекта теперь происходит через статический метод Create с возвращением в качестве результата Result
ErrorHandling/ActionStatus.cs
Outdated
| @@ -0,0 +1,17 @@ | |||
| namespace ErrorHandling; | |||
|
|
|||
| public struct ActionStatus(string error) | |||
There was a problem hiding this comment.
А чем это отличается от условного Result без параметра ? Зачем оно нужно
There was a problem hiding this comment.
Ничем, просто класс Result без параметра не удается создать, т.к существует класс расширение с таким же названием
There was a problem hiding this comment.
nit: лучше тогда класс расширение переименовать, ибо в нём даже название нигде не будет фигурировать
| } | ||
|
|
||
| public struct Result<T> | ||
| public readonly struct Result<T>(string error, T value = default) |
There was a problem hiding this comment.
А можешь раскрыть, зачем тут struct? Ещё и readonly) И чем это лучше record?
ErrorHandling/ActionStatus.cs
Outdated
| @@ -0,0 +1,17 @@ | |||
| namespace ErrorHandling; | |||
|
|
|||
| public struct ActionStatus(string error) | |||
There was a problem hiding this comment.
nit: лучше тогда класс расширение переименовать, ибо в нём даже название нигде не будет фигурировать
2 participants
No description provided.