Галичев Артем #20
Галичев Артем #20
Conversation
|
Зачастую в командах ставят себе валидации на лишние пробелы в пустых строках, подсвечивают их зеленым
В райдере это решается одной настройкой Делать не обязательно (пока в команду не попадешь), но вдруг тебе будет интересно |
| using var reader = new StreamReader(filePath); | ||
|
|
||
| return reader.ReadToEnd(); |
There was a problem hiding this comment.
Можно без стрима, просто File.ReadAllText(Async)
| var brush = new SolidBrush(colorFactory.GetColor()); | ||
| var font = new Font(tag.FontFamily, tag.FontSize); |
TagsCloudVisualization/Program.cs
Outdated
There was a problem hiding this comment.
Раз ты уже относишься к этому проекту, как к библиотеке, давай сделаем его библиотекой полностью - удалим Program.cs и удалим <OutputType>Exe</OutputType> из csproj
| Size.Ceiling(_graphics.MeasureString(word, new Font(_fontFamily, fontSize))); | ||
| } | ||
|
|
||
| public class TagLayouterOptions(int minFontSize, int maxFontSize, string fontName) |
There was a problem hiding this comment.
Давай в отдельный файл вынесем, как и в других местах: один файл-один класс/интерфейс/...
| { | ||
| public int MinFontSize { get; } = minFontSize; | ||
| public int MaxFontSize { get; } = maxFontSize; | ||
| public FontFamily FontFamily { get; } = ConvertFontNameToFontFamily(fontName); |
There was a problem hiding this comment.
Давай чуть оптимизируем и посчитаем это в конструкторе или вообще извне закинем, чтобы при каждом обращении к свойству если что не ловить исключения
There was a problem hiding this comment.
так оно и так получается через конструктор задается, , было бы написано как FontFamily => ConvertFontNameToFontFamily(fontName);, то да, падало бы при обращении к свойству
|
|
||
| namespace TagsCloudVisualization; | ||
|
|
||
| public class TagsCloudImageCreator( |
There was a problem hiding this comment.
Давай тоже просто чтобы следовать общей структуре решения создадим для этого класса интерфейс и будем работать через него
| var text = textReaders | ||
| .First(x => x.IsCanRead(pathToText)) | ||
| .ReadWords(pathToText); |
There was a problem hiding this comment.
А если путь до файла передан неверно и файла не существует? Стандартная ошибка может быть недостаточно информативной и упасть в неподходящий момент
Как будто до выполнения основной нагрузки не хватает какого-то валидатора переданных опций при старте программы, который бы понятным языком расшифровывал, что идет не так в случае чего
Это бы позволило убрать валидации из конструкторов, например, из TagLayouter и try-catch по названию шрифта из TagLayouterOptions
| { | ||
| public static ContainerBuilder RegisterWordAnalytics(this ContainerBuilder builder) | ||
| { | ||
| builder.RegisterInstance(WordList.CreateFromFiles("Dictionaries/ru/ru.dic")); |
There was a problem hiding this comment.
Не обработал кейс, когда словарь не смогли подгрузить, надо пользователю сказать, что не так и что нужно сделать
|
|
||
| public static ContainerBuilder RegisterImageSavers(this ContainerBuilder builder, TagsCloudVisualizationOptions options) | ||
| { | ||
| builder.RegisterType<PngSaver>().WithParameter("path", options.OutputFilePath).As<IImageSaver>(); |
There was a problem hiding this comment.
Строковые константы - почти всегда очень хрупкое решение: любой ренейм при рефакторинге проекта приведет к тому, что придется править регистрации в контейнере и хорошо ещё, если ошибку заметят сразу и она не попадет на прод. Нужно избавиться от всех строковых констант в регистрации, чтобы заранее защититься от таких проблем
| foreach (var rectangle in rectangles) | ||
| { | ||
| rectangle.Offset(_centerOffset); | ||
| graphics.DrawRectangle(GetRandomPen(), rectangle); |
|
|
||
| namespace TagsCloudVisualizationTests.Base; | ||
|
|
||
| [TestFixture] |
| namespace TagsCloudVisualizationTests.Base; | ||
|
|
||
| [TestFixture] | ||
| [Parallelizable(ParallelScope.Self)] |
There was a problem hiding this comment.
Это ведь значение по умолчанию, если не ошибаюсь, зачем явно прописывать?
| return actualColor; | ||
| } | ||
|
|
||
| private static TestCaseData[] _getColorsTestCases = [ |
There was a problem hiding this comment.
Сделал поле, но назвал с _get, как будто это метод
There was a problem hiding this comment.
ну тут нейминг идет по такому правилу <имя тестируемого метода>TestCases. Стоит как то по другому?
There was a problem hiding this comment.
Понял мысль, но не придумал ничего лучше, как заменить, давай так оставим)
| private readonly Random _random = new(Seed); | ||
|
|
||
| [Test] | ||
| [Repeat(10)] |
There was a problem hiding this comment.
Репит в тестах не очень хорошая штука, если это задумано, чтобы сделать тест более стабильным, чтобы он не падал в рандомные промежутки времени. Нужно идти именно от проблемы - почему тест падает и можно ли его сделать стабильным. Вижу, что ты используешь сид для рандома, т.е. он должен всегда одинаковые значения давать, почему тогда требуется репит?
There was a problem hiding this comment.
Репит сделан, чтобы убедиться, что в тесте будут рассмотрены разнообразные случаи и метод работает на всех правильно. Сид нужен, чтобы воссоздать упавший кейс и задебажить его (значения в каждом репите не будут повторятся).
|
|
||
| [TestFixture] | ||
| [Parallelizable(ParallelScope.All)] | ||
| [FixtureLifeCycle(LifeCycle.InstancePerTestCase)] |
There was a problem hiding this comment.
А это разве не значение по умолчанию?
There was a problem hiding this comment.
нет, по умолчанию один экземпляр на все тесты в классе.
| } | ||
|
|
||
| [Test] | ||
| [Repeat(10)] |
@desolver