Бессараб Дмитрий #31
Бессараб Дмитрий #31
Conversation
|
|
||
| namespace TagsCloudContainer.TagGenerator | ||
| { | ||
| public interface ITagsGenerator |
There was a problem hiding this comment.
Не совсем понимаю, что будет делать этот элемент. Можешь рассказать подробнее?
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Text; | ||
| using System.Threading.Tasks; |
There was a problem hiding this comment.
Видимо остались старые ненужные зависимости. Их лучше очищать, чтобы не засорять код
| { | ||
| public interface ITextProcessor | ||
| { | ||
| public Dictionary<Word, int> Words(string path, ITextProvider provider, params IWordFilter[] filters); |
There was a problem hiding this comment.
Кажется фильтры - это всё-таки информация для конструктора, а не конкретного метода. Почему выбрал делать именно так?
Хотел предложить сделать обработку уже прочитанного текста, а не читать внутри этого же класса из файла. Получается очень жёсткая связь в поведении. У нас есть штука, которая умеет класса подготавливать слова, но получить их может только из файла
| var words = new Dictionary<Word, int>(); | ||
| foreach (var word in GetWordsFromString(provider.ReadFile(path))) | ||
| { | ||
| var flag = true; |
There was a problem hiding this comment.
Очень абстрактное название для переменной. Давай придадим смысла
| foreach (var filter in filters) | ||
| { | ||
| if (flag == false) break; | ||
| if (filter.Skips(word)) continue; | ||
| flag = false; | ||
| } | ||
| if (flag == false) continue; |
There was a problem hiding this comment.
Как сложно получилось 😨
Есть предложение реализовать здесь паттерн фильтра (немного отличается от привычного значения) и упростить этот код. А так же дать возможность легко расширять набор этих предусловий
Вот здесьможно почитать про паттерн фильтра. Нашёл хорошую статью только для Java, но она очень похожа на C#. Если будут вопросы, разберём
|
|
||
| namespace TagsCloudContainer.WordFilters | ||
| { | ||
| public class SimpleFilter : IWordFilter |
| { | ||
| public class SimpleFilter : IWordFilter | ||
| { | ||
| private readonly HashSet<string> _words = |
There was a problem hiding this comment.
Дадим более узкое название этому списку исключений? А то только по операциям над ним становится понятно, что это такое
TagsCloudContainer/Tag.cs
Outdated
| Font = font; | ||
| Color = color; | ||
| var rect = g.MeasureString(Value, Font).ToSize(); | ||
| Frame = new Size(rect.Width + 2, rect.Height + 2); |
There was a problem hiding this comment.
Подскажи, для чего нужно добавлять магическое число к размерам?
There was a problem hiding this comment.
Кстати, размеры можно считать "снаружи" и передавать уже готовый size. Тогда это станет record
TagsCloudContainer/Program.cs
Outdated
| var provider = new TxtTextProvider(); | ||
| var filter = new SimpleFilter(); | ||
| var processor = new TextProcessor.TextProcessor(); |
There was a problem hiding this comment.
Комментарий на будущее. DI контейнер заменит это на красивый блок кода, где не нужно будет задумываться над тем, как кого создать
| } | ||
|
|
||
| [Test] | ||
| public void ThrowException() |
There was a problem hiding this comment.
Не хватает в названии условия, при котором бросать исключение
|
|
||
| public class Config | ||
| { | ||
| public Type PointGenerator { get; set; } |
There was a problem hiding this comment.
Хранить типы в конфигурации - не есть хорошо. Лучше использовать фабрики, если это действительно необходимо. Иначе поддержка такого кода станет очень сложной. Начиная отсутствием проверки типов и заканчивая банальной проблемой поиска использований по коду
| public bool RandomColor; | ||
| public Color Color { get; set; } |
There was a problem hiding this comment.
Названия у этих свойств не очень отражают суть. Получится придать им смысла? И ещё не очень понятно, как быть в ситуации, когда RandomColor=true и цвет тоже указан. Можно посмотреть в сторону nullable типов
| var container = new ContainerBuilder(); | ||
|
|
||
| container.RegisterType(config.PointGenerator) | ||
| .AsImplementedInterfaces() |
There was a problem hiding this comment.
Писать один раз удобно, но в поддержке тяжело. Поиском не найти, где этот интерфейс применяется и с кем связан. А ещё возможны потенциальные конфликты, так как иногда один класс реализует несколько интерфейсов, и не ясно, к какому его использовать. А ещё вложенное наследование
| .AsImplementedInterfaces() | ||
| .SingleInstance(); | ||
| else | ||
| { |
There was a problem hiding this comment.
Фигурные скобки сильно путают. Если у if их нет, то скорее всего в else тоже они не нужны
|
|
||
| namespace TagsCloudContainer.PointGenerators | ||
| { | ||
| [Label("Спираль")] |
There was a problem hiding this comment.
Это интересная техника, хорошо что прочитал. Но здесь на мой взгляд это перебор. Если что-то подобное делать, то нужно использовать локализации, а это уже совсем другая история. Предлагаю остановиться просто на названиях классов или даже цифрах при выборе. Это уже будет определяться реализацией клиента (ui или console)
TagsCloudContainer/Program.cs
Outdated
| { | ||
| internal class Program | ||
| { | ||
| static void Main(string[] args) |
There was a problem hiding this comment.
Если код ниже - эксперименты - хорошо. Но в итоговом решении его стоит вынести в отдельную реализацию клиента, и в Main вызывать уже его запуск
TagsCloudContainer/Program.cs
Outdated
|
|
||
| } | ||
|
|
||
| private static void ConfigureColor(Config config) |
There was a problem hiding this comment.
А вот вручную вызывать Assembly не нужно. Это ровно противоположное действие использованию DI контейнера. Задать возможные цвета через консоль поможет одна из библиотек парсинга аргументов. Например такой. Это заметно упростит обработку и поддержку кода. Чем более строгая типизация - тем лучше. Думаю, можно прямо на этапе разработки добавить нужные значения в аргументы. Они редко будут меняться. А что самое главное, при их изменении IDE подскажет, где есть использование, что позволит явно проконтролировать обратную совместимость (новая версия не ломает старое использование)
К тому же всё это теряет смысл в момент парсинга цвета, который производится вручную через switch
TagsCloudContainer/Program.cs
Outdated
| var pointGenerators = FindImplemetations<IPointGenerator>(); | ||
| foreach (var point in pointGenerators) | ||
| Console.WriteLine(point.Key); | ||
| Console.WriteLine("Введите, соблюдая орфографию"); |
There was a problem hiding this comment.
Аналогично предыдущему. Если хочется дать такую гибкую возможность конфигурирования, то можно воспользоваться функцией di возвращать набор зарегистрированных реализаций (container.Resolve<IEnumerable<IPointGenerator>>()). Тогда пользователь сможет выбрать только из того, что заведомо работает и поддерживается.
А ещё писать целиком - это опасное дело. Приятнее для человека указать одну цифру, чем вручную писать целом слово
TagsCloudContainer/Tag.cs
Outdated
| Font = font; | ||
| Color = color; | ||
| var rect = g.MeasureString(Value, Font).ToSize(); | ||
| Frame = new Size(rect.Width + 2, rect.Height + 2); |
There was a problem hiding this comment.
Кстати, размеры можно считать "снаружи" и передавать уже готовый size. Тогда это станет record
TagsCloudContainer/Tag.cs
Outdated
|
|
||
| public Tag(Graphics g, Word word, Font font, Color color) | ||
| { | ||
| Value = word.Value; |
There was a problem hiding this comment.
Точно нужно доставать значение из Word? Можно его сохранить как самодостаточную единицу, вдруг что-то понадобится в него добавить
@glazz200