-
Notifications
You must be signed in to change notification settings - Fork 319
Д/з Markdown #266
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
base: master
Are you sure you want to change the base?
Д/з Markdown #266
Conversation
Markdown/Md.cs
Outdated
| return BuildConvertedString(text, onlyValidatedMdSymbols); | ||
| } | ||
|
|
||
| private IEnumerable<ReadOnlyMemory<char>> SplitIntoSubstrings(ReadOnlyMemory<char> text, Func<char, bool> predicate) |
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.
Почему решил использовать Memory?
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.
Чтобы не плодить большое количество подстрок, так как это дорого, и в этом я не увидел необходимости
Markdown/Md.cs
Outdated
| return BuildConvertedString(text, onlyValidatedMdSymbols); | ||
| } | ||
|
|
||
| private IEnumerable<ReadOnlyMemory<char>> SplitIntoSubstrings(ReadOnlyMemory<char> text, Func<char, bool> predicate) |
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.
Для предикатов кстати можно использовать соответствующий тип Predicate<char>
| { | ||
| var sb = new StringBuilder(); | ||
|
|
||
| var mdSymbols = SplitIntoSubstrings(text.AsMemory(), t => t == '\n') |
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.
А как нам быть с CRLF?
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.
Всё будет штатно работать, так как у меня метод не убирает символы, по которым разбивка происходит. Они остаются в подстроках. Останется строка с crlf. Когда собираю html эти же переносы и использую
Markdown/MdKeySymbol.cs
Outdated
|
|
||
| public class MdKeySymbol | ||
| { | ||
| private static readonly string keySymbols = @"_#\"; |
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.
Не очень хорошо получается, что теги у нас лежат в одном месте, а допустимые символы в другом. Велик риск при расширении добавить только в одном месте и словить ошибки
А ещё возникнут сложности, если я захочу добавить обработку ```. Мне придётся добавить в качестве символа `, а тег существует только с тремя.
Markdown/MdKeySymbol.cs
Outdated
| public class MdKeySymbol | ||
| { | ||
| private static readonly string keySymbols = @"_#\"; | ||
| private static readonly Dictionary<MdKeySymbol, string> mdSymbolToHtmlTag = new() |
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.
А вот располагать тут же мапинг в HTML очень плохо. Это делает код жестким. Представь, что потребуется написать ещё транслятор из MD в какой-нибудь транспортный XML. В этом случае понадобится здесь же дописывать мапинги из md в XML. А потом попросят из HTML в md и появится ещё один мапинг. Обычно делают некоторым промежуточный язык, в который умеют транслировать и из которого умеют читать
Markdown/MdKeySymbol.cs
Outdated
| isFirstNonSpace = isFirstNonSpace && IsWhiteSpace(word); | ||
| } | ||
|
|
||
| symbols.Sort((x, y) => x.Position - y.Position); |
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.
В программе пары символов ищутся на стеке, поэтому нет никаких гарантий, что они попадут в список в порядке их возникновения в исходной строке
Markdown/MdKeySymbol.cs
Outdated
| MdSymbol.Quantity, | ||
| MdSymbol.Position) { } | ||
|
|
||
| public int CalculateSymbolLength() |
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.
Выглядит так, что при добавлении каждого нового тега придётся переписывать этот метод. Не получится вынести расширяющие изменения в одно место?
Markdown/MdKeySymbol.cs
Outdated
|
|
||
| public override int GetHashCode() | ||
| { | ||
| return HashCode.Combine(Type, RelativePosition, Quantity); |
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.
А вот здесь опасно их так использовать. Это случится при добавлении например в словарь.
var symbol = MdKeySymbol.Create('#', 0, Open);
dict[symbol] = "NumberSign";
symbol.RelativePosition = Close;
dict[symbol] // throw KeyNotFoundException
Markdown/MdKeySymbol.cs
Outdated
| yield return mdSymbol; | ||
| } | ||
|
|
||
| private static MdSymbolRelativePosition DetermineRelativePosition(int positionInWord, int wordLength) |
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.
А выделение внутри слова нормально сработает?
Markdown/MdSymbolQuantity.cs
Outdated
| @@ -0,0 +1,7 @@ | |||
| namespace Markdown; | |||
|
|
|||
| public enum MdSymbolQuantity | |||
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.
Кажется это отсылка к частным случаям, у нас всё-таки есть отдельно теги _ и __. Это не повторение одного и того же. Но пока пусть будет
GlazProject
left a comment
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 string Render(string text) | ||
| { | ||
| var sb = new StringBuilder(); |
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.
Он нам нужен?
| .SelectMany(MdKeySymbolOperations.GetMdKeySymbolsInLine) | ||
| .ToArray(); | ||
|
|
||
| MdKeySymbol.Validate(mdSymbols); |
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.
А не хочешь вынести это в MdKeySymbolValidator?
| return BuildConvertedString(text, onlyValidatedMdSymbols); | ||
| } | ||
|
|
||
| private IEnumerable<ReadOnlyMemory<char>> SplitIntoSubstrings(ReadOnlyMemory<char> text, Predicate<char> predicate) |
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.
Я бы заменил фразу predicate на separator
| return BuildConvertedString(text, onlyValidatedMdSymbols); | ||
| } | ||
|
|
||
| private IEnumerable<ReadOnlyMemory<char>> SplitIntoSubstrings(ReadOnlyMemory<char> text, Predicate<char> predicate) |
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.
Можно сделать статичными (https://rules.sonarsource.com/csharp/RSPEC-2325). А по хорошему этот код не относится непосредственно к MD, поэтому есть смысл вынести в методы расширения (extensions)
| } | ||
|
|
||
| var mdSymbol = mdSymbols[mdSymbolIndex++]; | ||
| sb.Append(mdSymbol.Render(MdToHtmlMapping.Mapping)); |
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.
Смотри, это всё ещё жесткая связка между MD и HTML. Вот если бы были теги по типу жирный, курсив, заголовок - тогда их можно транслировать в любой другой язык. И точно так же сравнительно легко можно было сделать парсер html
А ещё это усложнит тестирование при расширении по тем же причинам
|
|
||
| public class MdKeySymbolOperations | ||
| { | ||
| public static List<MdKeySymbol> GetMdKeySymbolsInLine(IEnumerable<ReadOnlyMemory<char>> line) |
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.
Не сходу понятно, как можно представить строку перечислением строк. Давай назовём lineWords
| symbols.Sort((x, y) => x.Position - y.Position); | ||
| if (symbols.Count > 0 && symbols[0].Type == MdSymbolType.NumberSign) | ||
| { | ||
| var mdSymbol = MdKeySymbol.Create("#", position, MdSymbolRelativePosition.Close); |
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.
Здесь мы уже работаем на уровне символов и выглядит странно, когда для MdSymbolType.NumberSign нам нужно вспомнить, что такой создаётся через #. Давай создавать уже с нужным типом?
| bool isFirstNonSpace) | ||
| { | ||
| var index = 0; | ||
| var relativePositions = DetermineRelativePosition(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.
Нам точно нужно генерировать столько объектов? При том что он используется только для управляющих символов
| return str[..(index + 1)]; | ||
| } | ||
|
|
||
| private static MdSymbolRelativePosition[] DetermineRelativePosition(ReadOnlyMemory<char> 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.
А если будет два выделения в одной части слова, они оба пометятся как открывающие или оба как закрывающие?
| { | ||
| if (peekSymbol?.Type != MdSymbolType.Backslash || peekSymbol?.Position < position - 1) | ||
| { | ||
| var readSymbol = GetMdKeySymbol(word[index..]); |
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.Concat(Enumerable.Repeat("#НеЗаголовок_с_разными_символами_", 50000)), то он просидит 10 минут и не завершит выполнение
@GlazProject