Калентьев Илья tg:@m1nus0ne #241
Conversation
cs/Markdown/Maker/IMaker.cs
Outdated
|
|
||
| public interface IMaker<T> | ||
| { | ||
| public T MakeFromTokens(List<StringToken> tokens); |
There was a problem hiding this comment.
Мелочь. Так же как и вместо собственных классов лучше использовать интерфейсы, так и тут лучше использовать IEnumerable, чтобы не завязываться на конкретную реализацию коллекции, если нам нужно просто по ней проитерироваться
cs/Markdown/Md.cs
Outdated
|
|
||
| public class Md<TResult> | ||
| { | ||
| public string Render(string input, IParser parser, IMaker<TResult> maker, IRenderer<TResult> renderer) |
There was a problem hiding this comment.
Довольно много параметров в этом методе передается. Подскажи, почему решил конструктор не использовать?
cs/Markdown/Md.cs
Outdated
| { | ||
| public string Render(string input, IParser parser, IMaker<TResult> maker, IRenderer<TResult> renderer) | ||
| { | ||
| var tokens = new MarkdownParser().Parse(input); |
There was a problem hiding this comment.
Тут тоже не в конструкторе и не в методе параметром передаем. Подскажи, почему именно так решил сделать?
| public abstract class BaseHtmlToken | ||
| { | ||
| public List<BaseHtmlToken>? Children { get; private protected set; } | ||
| public string Value { get; private protected set; } |
There was a problem hiding this comment.
Подскажи, ты осознанно использовал private protected set? Я не против, но если осознанно, то можешь объяснить мотивацию?
There was a problem hiding this comment.
Всегда использую private, по необходимости расширяю область видимости. Без protected сеттер из дочерних классов становится недоступным
| public List<BaseHtmlToken>? Children { get; private protected set; } | ||
| public string Value { get; private protected set; } | ||
|
|
||
| public BaseHtmlToken(List<BaseHtmlToken>? children) |
There was a problem hiding this comment.
В абстрактном классе у тебя 2 поля, но в конструкторе принимается только одно. Так и задумано?
There was a problem hiding this comment.
Например в токене, который будет просто хранить текст Children = {StrongCloseToken(), TextToken(), StrongOpenToken()}
|
|
||
| public abstract class BaseHtmlToken | ||
| { | ||
| public List<BaseHtmlToken>? Children { get; private protected set; } |
There was a problem hiding this comment.
А точно ли нам нужен здесь nulluble тип? Может нас устроит и просто пустая коллекция, когда дочерних тегов нет?
cs/Markdown/Md.cs
Outdated
|
|
||
| namespace Markdown; | ||
|
|
||
| public class Md<TResult> |
There was a problem hiding this comment.
Честно говоря пока не очень понимаю мотивации использовать тут дженерик. Можешь комментарием к треду или прямо в коде объяснить вкратце мотивацию?
| @@ -0,0 +1,12 @@ | |||
| namespace Markdown.Tokens; | |||
|
|
|||
| public abstract class BaseHtmlToken | |||
There was a problem hiding this comment.
Точно ли этот класс должен содержать "Html" в названии? Мы же парсим Markdown текст, который уже в последствии преобразуем в HTML, но мы можем захотеть и не только в него преобразовать, а и в другой язык разметки. Получится ли быстро это реализовать в текущей архитектуре?
Если я не понял идеи алгоритма, то можешь вкратце ее объяснить, пожалуйста?
There was a problem hiding this comment.
Изначально я хотел использовать модель-посредник для трансляции любого формата разметки текста в другой, но решил оставить IEnumerable ее упрощенной версией. Строка -> IEnumerable -> RootToken (репрезентация DOM)
| } | ||
|
|
||
| public abstract Type OpenTypeToken { get; } | ||
| public abstract Type CloseTypeToken { get; } |
There was a problem hiding this comment.
Для таких полей обычно используют Enum, подскажи, почему именно Type выбрал?
There was a problem hiding this comment.
После паринга StringToken будет преобразовываться в Open/CloseToken, далее в цельный Token
There was a problem hiding this comment.
В коде плохо соблюдаются правила написания чистого кода, которые вы проходили на лекциях. Есть хорошая декомпозиция у классов, но очень плохая у методов, в особенности у MakeFromTokens, стоит еще раз пересмотреть материалы из блока и порефакторить код на основе них и моих комментариев
| public class StringToken(string value, int offset, StringTokenType type) | ||
| { | ||
| public int Length = value.Length; | ||
| public int Offset = offset; |
There was a problem hiding this comment.
Length и Offset не используются в коде
| yield return new StringToken(Current.ToString(), position, StringTokenType.NewLine); | ||
| break; | ||
| case ' ': | ||
| case '\t': |
There was a problem hiding this comment.
Текущая реализация покрывает не все пробельные символы, рекомендую проверку делать через IsWhitespace
| switch (Current) | ||
| { | ||
| case '\n': | ||
| case '\r': |
There was a problem hiding this comment.
В текущей реализации перенос строки в Windows будет считаться за 2 переноса, т.к. он состоит из 2-х символов "\r\n", да и в целом вычитывание переносов строки кажется немного излишним. Мне кажется, что лучше было бы перед вызовом метода Parse разбить исходный текст на абзацы, и проверять именно их, а не весь текст целиком
|
|
||
| public IEnumerable<StringToken> Parse(string text) | ||
| { | ||
| this.text = text; |
There was a problem hiding this comment.
Не очень круто, что метод меняет состояние полей в классе (в данном случае присваивает значение), хочется, чтобы методы класса были максимально чистыми, а все присваивания были в конструкторе
| { | ||
| var start = position; | ||
| while (Next == ' ' || Next == '\t') | ||
| position++; |
There was a problem hiding this comment.
Охх, цикл внутри цикла, да еще и меняющий указатель. Такого точно лучше избегать, потому что очень легко себе в ногу выстрелить в таком коде, слишком много в голове держать надо, чтобы знать, как именно работает код, а при добавлении новых сценариев запомнить будет только сложнее
Вообще вижу у тебя почти что ДКА, и мне кажется, что переделать код так, чтобы метод стал полноценным ДКА - не очень сложно. Нужно:
- сохранять состояние(в теге ли мы, в тексте или внутри пробельных символов)
- предыдущую позицию, чтобы подстроки генерировать
| var result = new Md(new MarkdownParser(), new HtmlMaker(), new HtmlRenderer()) | ||
| .Render(md); | ||
| result.Should() | ||
| .Be(html); |
There was a problem hiding this comment.
Здесь и в других тестах не соблюдается паттерн AAA
| var result = new Md(new MarkdownParser(), new HtmlMaker(), new HtmlRenderer()) | ||
| .Render(md); | ||
| result.Should() | ||
| .Be(html); |
There was a problem hiding this comment.
Если ты решил переносить элементы цепочки вызовов Fluent методов, то переносить нужно кажый такой вызов, начиная с самого первого, как это сделано, например, в ParseWordsFromText
Заметил не только тут, но и в других местах в коде
|
|
||
| namespace Markdown; | ||
|
|
||
| public static class Extantions |
There was a problem hiding this comment.
Очень неочевидное название у класса. Классы-расширения принято называть на основе того, к какому элементу написаны расширения, или для какого сценария
|
|
||
| namespace Markdown; | ||
|
|
||
| public static class Extantions |
There was a problem hiding this comment.
Методы класса совсем не покрыты тестами, из-за этого к ним очень мало доверия
| throw new ArgumentException("Should have at least 2 elements"); | ||
| if (enumerable.Count() == 2) | ||
| return Enumerable.Empty<T>(); | ||
| return enumerable.Skip(1).Take(enumerable.Count() - 2); |
There was a problem hiding this comment.
Снова появились не самые понятные константы, о которых известно только внутри методов. Это плохой паттерн проектирования, потому что о том, как работает код, знаешь только ты, и стороннему наблюдателю (в частности ревьюеру) будет очень сложно разобраться в том, как он работает. Да и спустя время тебе самому будет сложно понять, для чего были нужны эти константы, и почему у них именно такое значение
В промышленной разработке код, который тяжело сходу понять, очень быстро становится легаси, потому что его очень сложно поддерживать и изменять
No description provided.