Зиновьева Милана #13
Зиновьева Милана #13
Conversation
| public static bool IsExcludedWord(string word) | ||
| { | ||
| var mystem = new MyStem(); | ||
| mystem.PathToMyStem = "/Users/milana/di-updated/TagsCloudVisualization/mystem"; //перенести |
There was a problem hiding this comment.
Ага, правильно написано "перенести", желательно прямо в папку с программой, чтобы не приходилось задавать PathToMyStem
| if (res.Contains("CONJ") || res.Contains("INTJ") | ||
| || res.Contains("PART") || res.Contains("PR") || res.Contains("SPRO")) | ||
| return true; | ||
| return false; |
There was a problem hiding this comment.
- Можно написать
return res.Contains("CONJ") || res.Contains("INTJ")
|| res.Contains("PART") || res.Contains("PR") || res.Contains("SPRO")
- Можно вынести инициализацию mystem в конструктор или даже прямо в di контейнер. У многих контейнеров есть функция инициализации зависимости от кастомной фабрики. Из жизни пример: надо объект telegram bot клиента, ему нужен токен доступа, и мы написали контейнеру "Когда кто-то попросит телеграм клиента, вызови его конструктор, передав ему вот такую строку" и в итоге прямо рядом с точкой входа есть место, в котором настраивается зависимость
| { | ||
| private static readonly Dictionary<string, int> keyValueWords = []; | ||
|
|
||
| public static Dictionary<string, int> ProcessFile(IFileProcessor fileProcessor, string filePath) |
There was a problem hiding this comment.
статичные классы делать не принято если у него есть зависимости. А IFileProcessor это зависимость.
И ещё не принято зависимости кидать в метод, тк если их больше одной, то мы потом концов не сыщем где какая используется, а di контейнер как раз нужен чтобы в одном месте регулировать, что кому надо отдать
…red of FractalPainter
ConsoleClient/Examples/example6.png
Outdated
ConsoleClient/Options.cs
Outdated
| public int CenterY { get; set; } | ||
|
|
||
| [Option('l', "lenght", Default = 400, HelpText = "Длина изображения")] | ||
| public int Lenght { get; set; } |
ConsoleClient/Program.cs
Outdated
| return new DrawingTagsCloud(arrRect); | ||
| }).As<IDrawingTagsCloud>().InstancePerDependency(); | ||
|
|
||
| var container = builder.Build(); |
There was a problem hiding this comment.
Принято инициализацию контейнера уносить в отдельный класс. У тебя тут зависимостей чуть-чуть, но это уже занимает места на полтора экрана
ConsoleClient/Program.cs
Outdated
| { | ||
| return new ArchimedeanSpiral(new Point(options.CenterX, options.CenterY), 1); | ||
| } | ||
| return new FermatSpiral(new Point(options.CenterX, options.CenterY), 20); |
There was a problem hiding this comment.
Тут бы switch подошёл. Алгоритм А - А, алгоритм Б - Б, а какой-то другой - throw ArgumentOutOfRangeException или просто ошибку вывести вместо страшного ексепшна
Вообще по идее валидацией должен заниматься CommandLineParser, но раз не завезли валидацию значений, надо самим валидировать
|
|
||
| private static void HandleErrors(IEnumerable<Error> errors) | ||
| { | ||
| Console.WriteLine("Ошибка при обработке аргументов командной строки."); |
There was a problem hiding this comment.
Вот тут бы обработать коллекцию ошибок, чтобы пользователя не оставить в недоумении
|
|
||
| public interface IDrawingTagsCloud | ||
| { | ||
| public void SaveToFile(string filePath, int lenght, int width, string color); |
There was a problem hiding this comment.
По именованию
- файл обработчик должен называться как файл обработчик. А здесь название будто класс хранит данные
- Имя метода не соответствует имени класса. Вроде бы мы рисуем, а по факту ещё и сохраняем куда-то
Для обработчиков есть окончание-заглушка: Handler. Если ты начнёшь читать чужой код и увидишь это слово, значит автор не смог придумать внятное название
В твоём случае ты генерируешь файл и сохраняешь. Тогда можешь создать ITagsCloudDrawer и IImageSaver, в одном метод Draw, а во втором SaveToFile, и оба по очереди вызываются из Program
А ну и принято интерфейсы в отдельные файлы убирать, но это по вкусу. Просто в контуре все так делают
| public class RectangleInformation(Rectangle rectangle, string word) | ||
| { | ||
| public readonly Rectangle rectangle = rectangle; | ||
| public readonly string word = word; |
There was a problem hiding this comment.
Можно написать record RectangleInformation(Rectangle Rectangle, string Word)
ConsoleClient/Program.cs
Outdated
| var frequencyRectangles = wordHandler.ProcessFile(options.InputFilePath); | ||
| var arrRect = c.Resolve<IRectangleGenerator>().ExecuteRectangles(frequencyRectangles, new Point(options.CenterX, options.CenterY)); | ||
| return new DrawingTagsCloud(arrRect); | ||
| }).As<IDrawingTagsCloud>().InstancePerDependency(); |
There was a problem hiding this comment.
А, ещё тут прямо логика в инициализации. Так нельзя. У тебя в отрисовщике в зависимости данные. Для ISpiral так можно делать, тк нам надо понимать, кого конкретно использовать. А здесь надо чтобы либо IWordHandler был в зависимости RectangleGenerator, а он в зависимости у отрисовщика, либо чтобы сама Program резолвила все зависимости по очереди и в нужном порядке обрабатывала. Я бы вообще сделал чтобы был какой-то TagsCloudDrawingFacade, у которого в зависимостях вообще все модули, он в нужном порядке их вызывает, а в Program просто была строчка c.Resolve<ITagsCloudDrawingFacade>().DrawRectangle(options)
| if (option.Equals("all")) | ||
| return res.Contains("CONJ") || res.Contains("INTJ") || res.Contains("PART") | ||
| || res.Contains("PR") || res.Contains("SPRO"); | ||
| else | ||
| return !res.Contains(option) || res.Contains("CONJ") || res.Contains("INTJ") || res.Contains("PART") | ||
| || res.Contains("PR") || res.Contains("SPRO"); |
There was a problem hiding this comment.
Тут исключаются некоторые части речи по умолчанию, но вручную можно исключить только основные
И ещё поиск contains("A") исключит заодно наречия, тк обработка слова "громко" выдаст громко{громко=ADV=|громкий=A=ед,кр,сред}, судя по результату поиска, думаю, лучше искать по регулярному выражению {word}={option}\W, где \W - не буква
| @@ -15,10 +10,14 @@ public MorphologicalProcessing(MyStem mystem) | |||
| _mystem = mystem; | |||
| } | |||
|
|
|||
| public bool IsExcludedWord(string word) | |||
| public bool IsExcludedWord(string word, string option) | |||
There was a problem hiding this comment.
И имя option напрягает, тк непонятно что ещё за option. Тут подойдёт excludedPartOfSpeech. И тогда ещё надо будет изменить поведение, тк excludedPartOfSpeech = all для пользователя означает, что исключатся все части речи. Надо будет в атрибуте Option() добавить Required = false и здесь проверить что значение задано и в этом случае исключать. Заодно не надо будет обрабатывать ситуацию, когда юзер ввёл -p All (регистр другой, поведение станет непредсказуемым)
No description provided.