-
Notifications
You must be signed in to change notification settings - Fork 319
Ситдиков Сергей #259
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?
Ситдиков Сергей #259
Conversation
|
|
||
| public class Md | ||
| { | ||
| public string Render(string markdownString) |
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 знает слишком много о внутренней архитектуре (напрямую вызывает ParagraphCreator, Tokenizer, HtmlCreator). Точно ли так должно быть?
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.
Так быть не должно, поэтому выделю отдельный класс MarkdownProcessor, в котором уже инициализация происходит других нужных классов, и из этого класса в Render будет вызван один метод конвертации в HTML
cs/Markdown/HtmlCreator.cs
Outdated
| @@ -0,0 +1,14 @@ | |||
| namespace Markdown; | |||
|
|
|||
| public static class HtmlCreator | |||
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.
Точно ли оба метода должны относиться к этому классу? Может разбить на 2 класса?
| @@ -0,0 +1,10 @@ | |||
| namespace Markdown; | |||
|
|
|||
| public static class ParagraphCreator | |||
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.
У тебя все классы содержат сразу реализацию, но что если мне понадобится ее поменять?
Давай для основных классов, например для ParagraphCreator, Tokenizer и других создадим интерфейсы типа IParagraphCreator, чтобы в будущем можно было проще поменять или добавить новую реализацию
cs/Markdown/Token/Token.cs
Outdated
| @@ -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.
Почему не используешь свойства?
Может сделать его не класс а record?
Все ли поля должны быть доступны для записи из других классов?
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.
Можно сделать record, потому что не планируется изменение значений уже установленного токена, с параграфом такая же ситуация
cs/Markdown/Token/Tokenizer.cs
Outdated
| @@ -0,0 +1,9 @@ | |||
| namespace Markdown; | |||
|
|
|||
| public static class Tokenizer | |||
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.
Для обработки добавил парсер токенов в дерево, в котором будет удобно работать с вложенностью, и экранирование будет обрабатываться тоже при парсинге, чтобы дерево уже было с верными значениями
|
|
||
| public static class ParagraphCreator | ||
| { | ||
| public static List<Paragraph> CreateParagraphs(string markdownString) |
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.
Вопрос по неймингу - точно ли тут слово create подходит?
cs/Markdown/Token/Tokenizer.cs
Outdated
|
|
||
| public static class Tokenizer | ||
| { | ||
| public static List<Token> CreateTokens(Paragraph paragraph) |
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.
Вопрос по неймингу - точно ли тут слово create подходит?
cs/Markdown/Token/TokenType.cs
Outdated
| @@ -0,0 +1,10 @@ | |||
| using System.Security.AccessControl; | |||
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
|
|
||
| var paragraphs = ParagraphCreator.CreateParagraphs(markdownString); | ||
|
|
||
| foreach (var paragraph in paragraphs) |
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.
Пользователь не поймет в чем ошибка. Нужно обработать входные данные и в случае чего выкинуть уже конкретное исключение?
| @@ -0,0 +1,6 @@ | |||
| namespace Markdown.Node; | |||
|
|
|||
| public class ItalicNode | |||
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.
Должно наследоваться от INode?
| @@ -0,0 +1,6 @@ | |||
| namespace 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.
неймспейс неправильный
| @@ -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.
Может экранирование как токен вынести?
| @@ -0,0 +1,3 @@ | |||
| namespace Markdown; | |||
|
|
|||
| public record Token(int Position, int Length, string Value, TokenType Type); No newline at end of file | |||
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.
Точно ли нужно Value?
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 MarkdownProcessor() | ||
| { | ||
| _paragraphCreator = new ParagraphCreator(); |
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.
А может все классы передавать в конструкторе?
| _htmlCreator = new HtmlCreator(); | ||
| } | ||
|
|
||
| public string ConvertToHtml(string markdownString) |
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.
Нужна какая-то проверка markdownString? Что если она будет null или пустой? Или слишком длинной?
|
|
||
| namespace Markdown.TokenParser; | ||
|
|
||
| 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.
ITokenParser
@PeterMotorniy
Идея такая:
Текст разбивается на параграфы
Параграфы на токены(обычный текст, подчеркивание, двойное подчеркивание)
Потом анализ списка токенов и сбор в html строку