-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Попов Захар #18
base: master
Are you sure you want to change the base?
Попов Захар #18
Conversation
Она хранит в себе строку, которая будет написана внутри прямоугольника.
Содержимое произведения братьев Гримм "Белоснежка и семь гномов", которое я использовал при разработке.
1. Использование контейнера зависимостей Autofac. 2. Использование CommandLine для парсинга аргументов командной строки.
// который содержит слова, для иссключения из фильтра скучных слов | ||
// 2. Нужно добавить опцию для указания имени файла, | ||
// который содержит слова, для добавления в фильтр скучных слов | ||
public class CommandLineOptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не очень расширяемо получается)
Давай представим, что у нас появятся новые клиенты, например, десктоп и веб.
Нам тогда для каждого нужно будет заново описывать содержимое этого класса.
А если мы добавим какой-то новый параметр? Надо будет не забыть добавить его во все такие настройки.
Я к тому, что лучше всего делать настройки клиентов как интерфейс :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Еще сюда же
Очень удобно работать с "Нормальными" настройками. То есть не когда к конструктор передается string smth
, а условные ImageSaverSettings settings
, у которых внутри лежат настройки. Так и DI проще конфигурировать и в других случаях (особенно с горячими настройками) работать гораздо проще (я бы даже сказал, что в такой картине мира это невозможно, потому что они хардкодятся при запуске программы)
TagCloud/ProgramExecutor.cs
Outdated
{ | ||
public void Execute(string resultFormat) | ||
{ | ||
var normalizedWordWeights = normalizer.Normalize(wordCounter.Values); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А чего он нигде не используется?)
TagCloud/ProgramExecutor.cs
Outdated
var tags = new List<Tag>(); | ||
foreach (var rectangleProperty in worker.GetNextRectangleProperties()) | ||
{ | ||
tags.Add(new Tag(rectangleProperty.word, layouter.PutNextRectangle(rectangleProperty.size))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Вложенные вызовы методов очень тяжело читать.
Мы привыкли читать слева направо. Также, в идеале, должно быть и с кодом)
А здесь приходится сначала прочитать правую часть, потом центр, а потом левую
TagCloud/WordFilters/WordFilter.cs
Outdated
|
||
public void Clear() => bannedWords.Clear(); | ||
|
||
public HashSet<string> BannedWords => bannedWords; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Почти)
Именно в таком решении вообще не будет разницы между
public HashSet<string> BannedWords => bannedWords;
и public HashSet<string> BannedWords {get;set;}
Чтобы от этого был больший смысл, можно сделать public IReadOnlySet<string> BannedWords => bannedWords
. Тогда множество нельзя будет менять из-вне)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Я тут имел ввиду public HashSet<string> BannedWords => bannedWords.ToHashSet();
, но что-то забылся на пол пути и показал жёсткое ООП!)
Не лучшее решение, так как приводит к выделению новой памяти, но с сутью задачи справляется - происходит перекопированние значений в новый HashSet(можно было и просто массив возвращать) и возвращается уже новый, независимый HashSet.
|
||
public bool Add(string word) => bannedWords.Add(word); | ||
|
||
public bool Remove(string word) => bannedWords.Remove(word); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А зачем эти методы нужны?)
Ты ведь их только внутри используешь, и так все хорошо, вроде бы
TagCloud/Parsers/BoolParser.cs
Outdated
{ | ||
public static bool ParseIsSorted(string value) | ||
{ | ||
if (value == false.ToString() || value == true.ToString()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Есть
bool.FalseString
иbool.TrueString
- Convert.ToBoolean(value) придумали несколько десятилетий назад. Люди до: 😃
TagCloud/Parsers/FontParser.cs
Outdated
{ | ||
throw new ArgumentException($"Неизвестный шрифт {font}"); | ||
} | ||
return font; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А почему возвращаешь строку?
Тут как будто гораздо проще будет возвращать FamilyFont, с ним удобнее и понятнее работать
TagCloud/Program.cs
Outdated
{ | ||
internal class Program | ||
{ | ||
private static IContainer container; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А зачем он нужен?)
[assembly: InternalsVisibleTo("TagCloud.Tests")] | ||
namespace TagCloud | ||
{ | ||
internal class Program |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
В целом, это бы разделить на разные проекты.
В одном у тебя бы лежала Core логика, а в другой уже клиенты к нему
TagCloud/DIContainer.cs
Outdated
var normalizer = c.Resolve<INormalizer>(); | ||
var wordFilter = c.Resolve<IWordFilter>(); | ||
|
||
foreach (var word in wordReader.ReadByLines(options.DataFileName)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ойойой
Это же бизнесовая логика. Типа. К DI она никакого отношения не имеет...
Такое надо выносить в отдельные классы.
TagCloud/ProgramExecutor.cs
Outdated
ICloudLayouterWorker worker, | ||
IImageSaver imageSaver) | ||
{ | ||
public void Execute(string resultFormat) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Почему ResultFormat закидываешь сюда? Скорее так, почему все остальное прокидывается внутрь, а это осталось здесь?)
// который содержит слова, для иссключения из фильтра скучных слов | ||
// 2. Нужно добавить опцию для указания имени файла, | ||
// который содержит слова, для добавления в фильтр скучных слов | ||
public class CommandLineOptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Еще сюда же
Очень удобно работать с "Нормальными" настройками. То есть не когда к конструктор передается string smth
, а условные ImageSaverSettings settings
, у которых внутри лежат настройки. Так и DI проще конфигурировать и в других случаях (особенно с горячими настройками) работать гораздо проще (я бы даже сказал, что в такой картине мира это невозможно, потому что они хардкодятся при запуске программы)
FontParser теперь возвращает FontFamily
1. Добавлена фабрика по созданию WordFilter. 2. Добавлена фабрика по созданиюCloudLayouterWorker. 3. Рефакторинг DIContainer.
1. wordsToIncludeFile - добавление слов в фильтр "скучных слов" из файла. 2. wordsToExcludeFile- исключение слов из фильтра "скучных слов" из файла. 3. Рефакторинг WordFilter.
@masssha1308
Черновой вариант решения пятой домашней работы.
Продублирую сюда примеры расположения прямоугольников из второй домашней работы.
Примеры работ с разными параметрами:
1.
Принцип расположения прямоугольников:
Прямоугольники ставятся на окружности увеличивающегося радиуса с центром в точке, совпадающей с центром первого поставленного прямоугольника.