-
Notifications
You must be signed in to change notification settings - Fork 300
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
Попов Захар #235
base: master
Are you sure you want to change the base?
Попов Захар #235
Conversation
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.
Итого
Здорово, что теперь ветка заведена специально под задачу!
Сейчас нужно чуть улучшить декомпозицию, конкретно к сущности "Токен" у меня есть некоторые вопросы, которые написаны ниже.
А также реализовать оставшиеся методы и написать к ним тесты. То есть довести задачу до её финального вида и не забыть про предупреждения от IDE, хех :)
Кстати, некоторые комментарии ниже хоть и оставлены к конкретным строчкам, но от них ожидаются исправления во всём коде.
cs/Markdown/Md.cs
Outdated
namespace Markdown | ||
{ | ||
internal class Md(IParser parser, IConverter converter, IWriter writer) |
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 должен обязывать пользователя создавать для него правила?
Нужно оставить только минимально необходимый набор параметров, которые должны быть получены извне.
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/Converters/IConverter.cs
Outdated
internal interface IConverter | ||
{ | ||
IList<Token> Convert(IList<Token> tokensToConvert); | ||
} |
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.
Добавлю тут про второй пункт из описания.
Хочется услышать мнение о целесообразности приведения конкретных реализаций парсера, конвертера и врайтера в статические, а не динамические классы. Тут больше вопрос в том, как их тогда обязать реализовывать интерфейсы?
Если реализация у метода/класса одна, и в будущем не предвидится появление новых реализаций - то это можно реализовать статическим классом без дополнительного интерфейса.
Если же реализаций точно будет несколько, то в таком случае статика подойдёт тут в меньшей степени.
Нормально сделать интерфейс, который объявляет какие-либо методы и иметь динамические классы для него.
Что касается конкретно IConverter
, IWriter
и IParser
, то наверное вопрос к тебе, что по твоему мнению никогда не потребует дополнительной реализации? А что может потребовать?
…ставдения токена.
1. Выполнен рефакторинг тестов. В связи с тем, что одно и тоже визуальное представление переданного текста можно получить из разных комбинаций токенов, я решил оставить в ParserMdTests только тривиальные ситуации, а в MdTests перенести все тесты, какие были раньше, но проверять именно корректность получения визуального представления. В связи с этим из ParserMd был убран избыточный функционал.
namespace Markdown.Tests.ParsersTests | ||
{ | ||
[TestFixture] | ||
public class ParserMdTests |
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.
Тест __Hello_ __world_
не совсем релевантен, потому что по спецификации __hello __
не может быть превращён в <strong>hello </strong>
из-за пробела.
За подчерками, начинающими выделение, должен следовать непробельный символ. Иначе эти_ подчерки_ не считаются выделением и остаются просто символами подчерка.
1. Тест Md_ShouldWorkInLinearTime теперь работает исходя из такой логики: Если мы увеличим длину входной строки в 2 раза, то и время выполнения работы, по сравнению с прошлой длиной входной строки, увеличится в 2 раза (+ погрешность). 2. Рефакторинг тестов.
Действительно, поскольку у нас конкретный класс Md, то он должен принимать только соответсвующий себе типа парсера. К renderer это не относится.
@Folleach
Проведено начальное проектирование архитектуры программы.
Идея:
Сначала входная строка разбивается на упорядоченный список токенов. Токен - сущность, хранящая в себе часть строки и соответствующий этой части тег.
Есть три типа токенов:
Затем, в списке токенов подменяются теги из markdown разметки на теги HTML разметки и список токенов собирается обратно в строку. В Program.cs привёл 4 примера токенов для разных ситуаций, что бы было наглядней.
Пара мыслей:
Не отпускает идея того, что бы токен в ToString() сам возвращал своё строковое представление. Я понимаю, что это как раз работа Writer'a, но я сам как-то зациклился на этой идее. В черновой реализации каждый раз, когда составной токен возвращает своё строковое представление, создаётся новый экземпляр StringBuilder(), что расточительно. Может быть просто использовать один статический StringBuilder() для всех ToString() в ComplexSringToken, который по концу ToString() будет очищаться?