-
Notifications
You must be signed in to change notification settings - Fork 319
Спроектировал парсер Markdown #256
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?
Conversation
Markdown/Markdown/Md.cs
Outdated
| @@ -0,0 +1,24 @@ | |||
| namespace Markdown; | |||
|
|
|||
| public class Md | |||
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,9 @@ | |||
| namespace Markdown; | |||
|
|
|||
| public class Token | |||
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 int StartIndex { get; set; }
Markdown/Markdown/TokenType.cs
Outdated
|
|
||
| public enum TokenType | ||
| { | ||
| Italics = 0, |
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
- Обычно нулевой элемент оставляют для значений по типу
Unknown- это помогает избежать ошибок, связанных с проставлением значения по умолчанию. Давай здесь сделаем также
| @@ -0,0 +1,8 @@ | |||
| namespace Markdown; | |||
|
|
|||
| public enum TokenType | |||
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.
Enum автоматически номеруется с нуля и далее по натуральным числам, обычно так явно не прописывают, только если не хотят явно изменить нумерацию. Здесь явно указана стандартная нумерация, поэтому, в целом, можно убрать. Править не заставляю - опционально. Если так привычнее - оставь
Markdown/Markdown/Md.cs
Outdated
| @@ -0,0 +1,24 @@ | |||
| namespace Markdown; | |||
|
|
|||
| public class Md | |||
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 (не забудь поменять название) имплементацией этого интерфейса. Придумай соответствующее название для интерфейса
Markdown/Markdown/IParser.cs
Outdated
| @@ -0,0 +1,10 @@ | |||
| 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.
IParser - обобщенное название, но методы выставлены довольно специфичные, ещё и метод Parse, который, логично, должен выставлять парсер, отсутствует, зато есть метод Render) В реальном проекте это будет страшно путать других разработчиков, которые впервые видят этот код. Давай разберёмся с названиями
Markdown/Markdown/MarkdownParser.cs
Outdated
|
|
||
| public class MarkdownParser : IParser | ||
| { | ||
| private static Dictionary<string, TokenType> _pairMarkdownToTag = 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.
Сделай readonly
Markdown/Markdown/MarkdownParser.cs
Outdated
|
|
||
| private string _markdown; | ||
|
|
||
| public void SetMarkdownText(string markdown) => this._markdown = markdown; // Только для теста обёртки токенов в теги |
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 явно в WrapTokensWithTags или что-то другое придумай
Markdown/Markdown/MarkdownParser.cs
Outdated
| } | ||
| else | ||
| currentTag = markdown[i].ToString(); | ||
| if (currentTag == "#") |
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.
Это условие никогда не выполняется - в _pairMarkdownToTag отсутствует #
Markdown/Markdown/MarkdownParser.cs
Outdated
| token.StartIndex - currEndIndex)); | ||
| if (token.StartIndex >= currEndIndex) | ||
| htmlString.Append(wrappedTags[i]); | ||
| else |
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.
Код в блоке else работает неэффективно и реализация через replace может быть довольно хрупкой. Лучше строить HTML за один проход без замен, подумай, как избавиться от этих проблем
| && (start > 1 && _markdown[start - 2] != '\\'); | ||
| if (isOpeningTag) | ||
| return !isTagScreened && _markdown[start + length] != ' ' | ||
| || (_markdown[start] == '#' && _markdown[start] == '#' && _markdown[start + 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.
Аналогично, условие `_markdown[start] == '#' бесполезное
| return !isTagScreened && _markdown[start + length] != ' ' | ||
| || (_markdown[start] == '#' && _markdown[start] == '#' && _markdown[start + 1] == ' '); | ||
| return !isTagScreened && _markdown[start - 1] != ' ' | ||
| || (_markdown[start] == '#' && start + length <= _markdown.Length && _markdown[start + 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.
Аналогично
| return !string.IsNullOrEmpty(_markdown.Substring(start, end - start + 1)); | ||
| } | ||
|
|
||
| public bool HasTagDigitsInside(int start, int end) |
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. Ничего не смущает?)
| @@ -0,0 +1,7 @@ | |||
| // public 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.
На всякий случай напишу: в реальный проект такие изменения закомментированные без пояснений лучше не заливать)
Markdown/Markdown/TagBuilder.cs
Outdated
| case "Title": | ||
| return (Header.OpeningTag, Header.ClosingTag); | ||
| case "ListItem": | ||
| return (ListItem.OpeningTag, ListItem.OpeningTag); |
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.
Дважды ListItem.OpeningTag - очепятка
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.
Кстати в действительности этот сценарий - мертвый код, т.к. списки обрабатываются в ProcessUnpairLineTags. Ещё один минус в копилку ProcessUnpairLineTags
Написал первоначальную предполагаемую структуру парсера @OvchinnikovNikita