-
Notifications
You must be signed in to change notification settings - Fork 319
Копытов Михаил #257
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?
Копытов Михаил #257
Conversation
cs/Markdown/Block/BlockProcessor.cs
Outdated
| IEnumerable<Block> SplitToBlocks(string text); | ||
| } | ||
|
|
||
| public class BlockProcessor : IBlockProcessor |
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.
Да, решил убрать блоки
cs/Markdown/Md.cs
Outdated
| { | ||
| } | ||
|
|
||
| public Md(BlockProcessor blockProcessor, TokenProcessor tokenProcessor, Parser parser, |
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.
Все классы которые принимаются внутри конструктора имеют интерфейсы. Корректным способом взаимодействия будет через них.
| private readonly Parser parser; | ||
| private readonly HtmlRenderer htmlRenderer; | ||
|
|
||
| public 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.
Зачем нужен пустой конструктор?
cs/Markdown/Md.cs
Outdated
| public string Render(string markdownText) | ||
| { | ||
| var blocks = blockProcessor.SplitToBlocks(markdownText); | ||
| var renderedBlocks = new List<string>(); |
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/Block/Block.cs
Outdated
|
|
||
| namespace Markdown; | ||
|
|
||
| public class Block |
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.
Сейчас Block(который по сути не имеет смысла сам по себе) может быть инициализирован. Может это должно быть какой-то другой сущностью? Почему тут к примеру не использовал интерфейс?
cs/Markdown/LineNode/LineNode.cs
Outdated
|
|
||
| namespace Markdown; | ||
|
|
||
| public class LineNode |
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/LineNode/LineNode.cs
Outdated
| public class LinkNode : LineNode | ||
| { | ||
| public string Href; | ||
| public List<LineNode> Label; |
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.
Почему у LinkNode есть какие-то дочерние элементы? Насколько я понимаю у него должен быть текст+ссылка и всё
| @@ -0,0 +1,14 @@ | |||
| namespace Markdown; | |||
|
|
|||
| public interface IRenderer | |||
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/LineNode/LineNode.cs
Outdated
| public List<LineNode> Children; | ||
| } | ||
|
|
||
| public class LinkNode : LineNode |
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.
Есть LinkNode, но нет соответствующего токена. Почему?
Ну а ещё, не находишь неудобным, что если добавляешь новый тип тега, то придётся - cоздать новый TokenType/Block, создать новый LinkNode, засунуть куда-то логику по их взаимодействию(пока вообще не понятно, как конкретные ноды будут понимать, как они будут взаимодействовать с другими нодами).
cs/Markdown/Md.cs
Outdated
| { | ||
| } | ||
|
|
||
| public Md(BlockProcessor blockProcessor, TokenProcessor tokenProcessor, Parser parser, |
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. Лучше использовать стандартный от Microsoft. Почитай доку по тому как это делать
@tripples25
Идея решения: