-
Notifications
You must be signed in to change notification settings - Fork 320
Нухов Руслан #254
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?
Нухов Руслан #254
Conversation
cs/Markdown/TypeTag.cs
Outdated
| @@ -0,0 +1,11 @@ | |||
| namespace Markdown; | |||
|
|
|||
| public enum TypeTag | |||
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.
Обычно Type у enum-ов в конце названия
cs/Markdown/Md.cs
Outdated
|
|
||
| public class Md | ||
| { | ||
| private readonly ParserMarkdown parser = 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.
В типе переменной лучше всегда использовать интерфейс, чтобы в будущем что-то лишнее случайно не "протекло" из реализации.
IParser parser = new ParserMarkdown()
cs/Markdown/Parser/ParserMarkdown.cs
Outdated
| @@ -0,0 +1,9 @@ | |||
| namespace Markdown; | |||
|
|
|||
| public class ParserMarkdown: IParser | |||
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.
Название интерфейса обычно пишут в конце реализации MarkdownParser
cs/Markdown/Parser/MarkdownParser.cs
Outdated
| //{ TagType.Link, "" }, | ||
| }; | ||
|
|
||
| private static readonly HashSet<char> escapeSymbols = ['\\', '#', '_']; |
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.
Просто для ознакомления. На маленьких размерах массивы эффективнее HashSet
https://stackoverflow.com/questions/150750/hashset-vs-list-performance
cs/Markdown/Parser/MarkdownParser.cs
Outdated
| if (text.Length <= position + 1) return false; | ||
|
|
||
| foreach (var symbol in escapeSymbols) | ||
| if (text[position + 1] == symbol) |
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.
Почему не isCanEscaping = escapeSymbols.Contains(text[position + 1])?
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.
Не увидел, что можно так написать. Исправил.
cs/Markdown/Parser/MarkdownParser.cs
Outdated
| { | ||
| var possibleTags = new Dictionary<TagType, MarkdownTag>(); | ||
|
|
||
| foreach (var keyValuePair in tags) |
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.
keyValuePair ничего не говорит о внутренности переменной, кроме того что это Tuple. Это как var list = new List<T>()
markdownTagByType / etc
Саму переменную tags я бы тоже предложил ренеймнуть. Например как MarkdownTagsByType / etc
cs/Markdown/Parser/MarkdownParser.cs
Outdated
|
|
||
| private static bool IsHeader(string text, int position, Stack<OpenToken> tokensWithOpenTag) | ||
| { | ||
| var tagLength = tags[TagType.Header].Content.Length; |
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.
Странно опять доставать тег из коллекции, если у тебя выше по стеку только что было это в виде отдельной переменной. Можно просто прокинуть сверху
cs/Markdown/Parser/MarkdownParser.cs
Outdated
| var tagLength = tags[TagType.Header].Content.Length; | ||
| var tagContent = tags[TagType.Header].Content; | ||
| var isNewParagraph = position == 0 || | ||
| (position >= 2 && text.AsSpan(position - 2, 2).Equals("\n\n", StringComparison.Ordinal)); |
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.
Equals("\n\n"
Не будет работать на разных платформах.
https://stackoverflow.com/questions/1015766/difference-between-n-and-environment-newline
cs/Markdown/Renderer/HtmlRenderer.cs
Outdated
|
|
||
| public string Render(IEnumerable<Token> tokens) | ||
| { | ||
| var stringBuilder = 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.
Можно сразу передавать ему буфер чтобы не аллоцировать лишний раз память. Как минимум должно быть text.Length
|
|
||
| public static IEnumerable<TestCaseData> CasesWhenListWithOneToken() | ||
| { | ||
| yield return new TestCaseData(new List<Token> { new (TagType.None, "Human") }, "Human"); |
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.
Не хватает более комплексных тестов, но для первого варианта норм
| @@ -0,0 +1,451 @@ | |||
| namespace Markdown; | |||
|
|
|||
| public class MarkdownParser : IParser | |||
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.
В целом, парсер тяжко читать и осознавать. Попробуй как-нибудь декомпозировать, упростить логику
cs/Markdown/Parser/MarkdownParser.cs
Outdated
|
|
||
| private static List<Token> ProcessBorderlineCases(List<Token> tokens) | ||
| { | ||
| var result = ProcessEmptyUnderscores(tokens); |
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.
Что-то не понял зачем в каждом из этих методов делать ToList и аллоцировать память?
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.
Сначала у меня там был void и данные менялись по ссылке, но чтобы методы были более читаемыми, сделал, чтобы методы что-то возвращали, Также решил сделать, чтобы первоначальная коллекция не менялась. Но да в целях оптимизации это нужно убрать.
cs/Markdown/Parser/MarkdownParser.cs
Outdated
|
|
||
| private static Token ProcessTokenWithNumbers(Token token) | ||
| { | ||
| if (token.TagType is TagType.Bold or TagType.Italic && token.Content.Any(char.IsDigit)) |
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 override bool IsTag(string text, int position, bool isSameTagAlreadyOpen) | ||
| { | ||
| var doubleNewLineUnix = "\n\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.
Лучше использовать предназначенную константу из окружения
Environment.NewLine
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.
Чтобы ниже брать корректную подстроку можно доставать из неё Length
|
|
||
| var isNewParagraph = isNewParagraphWindows || isNewParagraphUnix; | ||
|
|
||
| return text.AsSpan(position, TagText.Length).Equals(TagText, StringComparison.Ordinal) && isNewParagraph && |
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 {набор bool-опреаторов}
Если нужно что-то посчитать(Например isNewParagraph), то можно выделить это в отдельный метод. и вызывать его.
Так можно улучшить:
- производительность - по первой false не будет считаться всего остального
- композицию - всё будет лежать в своих отдельных методах
| { | ||
| public override bool IsTag(string text, int position, bool isSameTagAlreadyOpen) | ||
| { | ||
| if (TagType != TagType.Bold && TagType != TagType.Italic) |
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.
Как-то странно в публичном методе проверять внутреннее неизменяемое поле
cs/Markdown/Parser/MarkdownParser.cs
Outdated
|
|
||
| private static MarkdownTag GetTag(string text, int position, Stack<OpenToken> tokensWithOpenTag) | ||
| { | ||
| var possibleTags = new List<MarkdownTag>(); |
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.
На каждом символе из текста аллоцировать новый массив слишком затратно
cs/Markdown/Parser/MarkdownParser.cs
Outdated
|
|
||
| foreach (var tag in markdownTagsByType.Values) | ||
| { | ||
| if (tag.TagText.Length + position > text.Length) continue; |
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.
Я бы сказал что это скорее логика тега - проверить, что он влазит в текст
cs/Markdown/Parser/MarkdownParser.cs
Outdated
| var result = new Token(openToken.OpenTag.TagType, text.Substring(openToken.TextStartPosition, length), | ||
| openToken.NestedTokens); | ||
|
|
||
| if (CheckOpenTokenTagBoldOrItalicLocatedInsideWords(text, endPosition, openToken) || |
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.
Это выглядит как логика. которая должна лежать в {Name}Tag или какой-то альтернативной сущностью {Name}Token. Но лежать прямо в Parser мне кажется не лучший вариант. Потому что при росте кол-ва тегов у тебя будет столько развилок в логике, что читать и поддерживать будет невозможно
cs/Markdown/Parser/MarkdownParser.cs
Outdated
| { | ||
| var length = endPosition - openToken.TextStartPosition; | ||
|
|
||
| var result = new Token(openToken.OpenTag.TagType, text.Substring(openToken.TextStartPosition, length), |
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.
Почему тут решил использовать Substring? Ты на каждый токен аллоцируешь новую строку. Скорость работы будет не лучшей. По идее везде можно обойтись AsSpan
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.
AsSpan возвращает ReadOnlySpan<char>. А в Token нужен string. То есть либо ReadOnlySpan<char>.ToString() (Но это тоже самое), либо хранить сразу ReadOnlySpan<char>, но это проблематично, так как у меня в Token есть List<Token>.
cs/Markdown/Parser/MarkdownParser.cs
Outdated
|
|
||
| private static Token ProcessEmptyUnderscores(Token token) | ||
| { | ||
| if (token is { TagType: TagType.Bold, Content.Length: 0, Children: null }) |
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.
А Italic и другие могут в себе пустоту хранить?
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.
Теперь отметаю эти вариант на моменте создания токенов.
| @@ -0,0 +1,6 @@ | |||
| namespace Markdown; | |||
|
|
|||
| public interface IParser | |||
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.
Ещё для ознакомления.
Сейчас у тебя структура файлов в проекте - Layer to folder. То есть есть разные слои приложения. По этим слоям мы раскладываем файлы.
Есть ещё один вариант - Feature to folder. Здесь файлы группируются по идейному смыслу, а не по слою. В твоём случае может быть что-то такое
Markdown/
-/MarkdownParser
-/Tags/
--/ItalicMarkdwonTag
...
Html/
-/HtmlRenderer
-/HtmlTag
IParser
IRenderer
Как по мне Feature to folder выглядит более выигрышным вариантом, тк в большинстве случаев ты работаешь именно с фичей, а не слоем. То есть нужно добавить/измнеить логику Markdown - ты открыл папку Markdown и работаешь только с ней. Конечно сейчас ещё может понадобиться потрогать HtmlRenderer, но в реальных ситуациях это редкость.
В Layer to folder у тебя может быть открыто большое кол-во верхнеуровневых папок из-за чего может быть тяжело ориентироваться.
Я не призываю это исправлять. Просто говорю как можно поступить ещё.
cs/Markdown/PlatformType.cs
Outdated
| @@ -0,0 +1,8 @@ | |||
| namespace Markdown; | |||
|
|
|||
| public enum PlatformType | |||
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.
Какой-то рудимент
Идея такая: