-
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
Калентьев Илья tg:@m1nus0ne #241
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
using Markdown.Tokens; | ||
using Markdown.Tokens.HtmlToken; | ||
|
||
namespace Markdown; | ||
|
||
public static class Extantions | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Методы класса совсем не покрыты тестами, из-за этого к ним очень мало доверия |
||
{ | ||
public static bool IsOnlyDight(this IEnumerable<BaseHtmlToken> tokens) | ||
{ | ||
if (tokens.Count() < 3) | ||
throw new ArgumentException("Method takes at least three node"); | ||
return tokens | ||
.Skip(1) | ||
.Take(tokens.Count() - 2) | ||
.SelectMany(node => node.Value) | ||
.All(char.IsDigit); | ||
} | ||
public static IEnumerable<BaseHtmlToken>? TextifyTags(this IEnumerable<BaseHtmlToken> nodes) | ||
{ | ||
foreach (var node in nodes) | ||
{ | ||
if (node is TextToken or HtmlTokenWithTag) | ||
yield return node; | ||
else | ||
yield return new TextToken(node.Value); | ||
} | ||
} | ||
public static IEnumerable<T> InnerElements<T>(this IEnumerable<T> enumerable) | ||
{ | ||
if (enumerable.Count() < 2) | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Снова появились не самые понятные константы, о которых известно только внутри методов. Это плохой паттерн проектирования, потому что о том, как работает код, знаешь только ты, и стороннему наблюдателю (в частности ревьюеру) будет очень сложно разобраться в том, как он работает. Да и спустя время тебе самому будет сложно понять, для чего были нужны эти константы, и почему у них именно такое значение В промышленной разработке код, который тяжело сходу понять, очень быстро становится легаси, потому что его очень сложно поддерживать и изменять |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
namespace Markdown; | ||
|
||
public interface ITranslationResult | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Интерфейс нигде не используется |
||
{ | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,244 @@ | ||
using System.Net.NetworkInformation; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Лишние юзинги |
||
using Markdown.Tokens; | ||
using Markdown.Tokens.HtmlToken; | ||
using Markdown.Tokens.HtmlToken.Header; | ||
using Markdown.Tokens.HtmlToken.Italic; | ||
using Markdown.Tokens.HtmlToken.ListItem; | ||
using Markdown.Tokens.HtmlToken.Strong; | ||
using Markdown.Tokens.HtmlToken.UnorderedList; | ||
using Markdown.Tokens.StringToken; | ||
|
||
namespace Markdown.Maker; | ||
|
||
public class HtmlMaker : IMaker<RootToken> | ||
{ | ||
private Stack<BaseHtmlToken> rootChildren = new Stack<BaseHtmlToken>(); | ||
private Stack<(int pos, BaseHtmlToken token)> temporaryStack = new Stack<(int, BaseHtmlToken)>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rootChildren и temporaryStack - не очень удачные названия. В первом случае без глубокого чтения кода непонятно, чьи именно дети элементы в коллекции, что за Root такой, который их в себе содержит. Во втором в целом очень общее и непонятное название, почти ничего не говорящее о назначении поля. Общих слов, которые ничего не говорят о специфике переменной/метода лучше в названиях избегать, в идеале по сигнатуре должно быть сразу понятно, что это за метод/переменная, и для чего и когда они используются There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Точно ли стек - подходящая структура как для rootChildren, так и для temporaryStack? |
||
private StringToken[] tokens; | ||
private int position; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Поля класса не очищаются после вызова метода MakeFromTokens, что делает класс одноразовым, это нигде не указано и в целом является антипаттерном |
||
|
||
|
||
private StringToken Peek(int offset) | ||
{ | ||
var index = position + offset; | ||
if (index >= tokens.Length) | ||
return tokens[^1]; | ||
return index < 0 ? tokens[0] : tokens[index]; | ||
} | ||
|
||
private StringToken Next => Peek(1); | ||
private StringToken Current => Peek(0); | ||
private StringToken Previous => Peek(-1); | ||
|
||
public RootToken MakeFromTokens(IEnumerable<StringToken> input) | ||
{ | ||
tokens = input.ToArray(); | ||
List<BaseHtmlToken>? children; | ||
while (position < tokens.Length) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Кажется, что тут можно легко можно перейти к циклу foreach. Если встает выбор между тем, использовать for/foreach или while, то лучше выбирать первый, потому что он более предсказуемый, и с ним проще работать |
||
{ | ||
switch (Current.Type) | ||
{ | ||
case StringTokenType.Unexpected: | ||
case StringTokenType.Text: | ||
rootChildren.Push(new TextToken(Current.Value)); | ||
break; | ||
case StringTokenType.WhiteSpace: | ||
var unclosedTags = new Stack<(int, BaseHtmlToken)>(); | ||
while (temporaryStack.Count > 0) | ||
{ | ||
var (nodeTokenIndex, node) = temporaryStack.Pop(); | ||
switch (node) | ||
{ | ||
case ItalicOpenToken: | ||
case StrongOpenToken: | ||
if ((nodeTokenIndex == 0 || | ||
tokens[nodeTokenIndex - 1].Type == StringTokenType.WhiteSpace) && | ||
tokens[nodeTokenIndex + 1].Type != StringTokenType.WhiteSpace) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Довольно тяжело было было разобраться в этом условии, в таких громоздких логических выражениях лучше отдельные части складывать в именованные переменные, чтобы можно было по их названию ориентироваться в условии |
||
unclosedTags.Push((nodeTokenIndex, node)); | ||
break; | ||
default: | ||
unclosedTags.Push((nodeTokenIndex, node)); | ||
break; | ||
} | ||
} | ||
|
||
foreach (var tag in unclosedTags) | ||
temporaryStack.Push(tag); | ||
rootChildren.Push(new TextToken(Current.Value)); | ||
break; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Весь этот блок (строчки 46-68) лучше вынести в отдельный метод |
||
case StringTokenType.NewLine: | ||
if (temporaryStack.Any(pair => pair.token is HeaderOpenToken)) | ||
{ | ||
temporaryStack.Clear(); | ||
rootChildren.Push(new HeaderCloseToken(Current.Value)); | ||
children = UnitTokenEnds<HeaderOpenToken>().ToList(); | ||
rootChildren.Push(new HeaderToken(children)); | ||
rootChildren.Push(new TextToken(Current.Value)); | ||
break; | ||
} | ||
|
||
if (temporaryStack.Any(pair => pair.token is ListItemOpenToken)) | ||
{ | ||
temporaryStack.Clear(); | ||
rootChildren.Push(new ListItemCloseToken(Current.Value)); | ||
children = UnitTokenEnds<ListItemOpenToken>().ToList(); | ||
rootChildren.Push(new ListItemToken(children)); | ||
rootChildren.Push(new TextToken(Current.Value)); | ||
break; | ||
} | ||
|
||
rootChildren.Push(new TextToken(Current.Value)); | ||
break; | ||
|
||
case StringTokenType.Dash: | ||
if (Next.Type == StringTokenType.WhiteSpace && | ||
(Previous.Type == StringTokenType.NewLine || position == 0)) | ||
{ | ||
if (TryOpenBodyWith(new ListItemOpenToken(Current.Value + Next.Value))) | ||
{ | ||
position++; | ||
break; | ||
} | ||
} | ||
|
||
rootChildren.Push(new TextToken(Current.Value)); | ||
|
||
break; | ||
|
||
case StringTokenType.Hash: | ||
if (Next.Type != StringTokenType.WhiteSpace || | ||
(Previous.Type != StringTokenType.NewLine && position != 0)) | ||
{ | ||
rootChildren.Push(new TextToken(Current.Value)); | ||
break; | ||
} | ||
|
||
if (TryOpenBodyWith(new HeaderOpenToken(Current.Value + Next.Value))) | ||
{ | ||
position++; | ||
break; | ||
} | ||
|
||
rootChildren.Push(new TextToken(Current.Value)); | ||
break; | ||
case StringTokenType.SingleUnderscore: | ||
if (TryOpenBodyWith(new ItalicOpenToken(Current.Value))) | ||
break; | ||
|
||
rootChildren.Push(new ItalicCloseToken(Current.Value)); | ||
children = UnitTokenEnds<ItalicOpenToken>() | ||
.Select(token => token is StrongToken ? new TextToken(token.Value) : token).ToList(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Если мы подразумеваем, что один токен может трансформироваться в другой, то кажется, что эта логика должна лежать в самом классе токена |
||
temporaryStack.Pop(); | ||
|
||
if (children.IsOnlyDight()) | ||
rootChildren.Push(new TextToken(string.Join("", children.Select(child => child.Value)))); | ||
else | ||
rootChildren.Push(new ItalicToken(children)); | ||
break; | ||
case StringTokenType.DoubleUnderscore: | ||
if (TryOpenBodyWith(new StrongOpenToken(Current.Value))) | ||
break; | ||
|
||
rootChildren.Push(new StrongCloseToken(Current.Value)); | ||
children = UnitTokenEnds<StrongOpenToken>().ToList(); | ||
temporaryStack.Pop(); | ||
|
||
if (children.Count == 2) | ||
rootChildren.Push(new TextToken(children[0].Value + children[1].Value)); | ||
else if (children.IsOnlyDight()) | ||
rootChildren.Push(new TextToken(string.Join("", children.Select(child => child.Value)))); | ||
else | ||
rootChildren.Push(new StrongToken(children)); | ||
|
||
break; | ||
default: | ||
throw new Exception("Unimplemented token"); | ||
} | ||
|
||
position++; | ||
} | ||
|
||
if (temporaryStack.Any(pair => pair.token is HeaderOpenToken)) | ||
{ | ||
children = new List<BaseHtmlToken>() { new HeaderCloseToken("") }; | ||
while (true) | ||
{ | ||
var child = rootChildren.Pop(); | ||
children.Add(child); | ||
if (child is HeaderOpenToken) | ||
break; | ||
} | ||
|
||
children.Reverse(); | ||
|
||
rootChildren.Push(new HeaderToken(children)); | ||
} | ||
|
||
if (temporaryStack.Any(pair => pair.token is ListItemOpenToken)) | ||
{ | ||
children = new List<BaseHtmlToken>() { new ListItemCloseToken("") }; | ||
while (true) | ||
{ | ||
var child = rootChildren.Pop(); | ||
children.Add(child); | ||
if (child is ListItemOpenToken) | ||
break; | ||
} | ||
|
||
children.Reverse(); | ||
|
||
rootChildren.Push(new HeaderToken(children)); | ||
} | ||
|
||
|
||
return new RootToken(rootChildren.Reverse().TextifyTags().ToList()); | ||
} | ||
|
||
private bool TryOpenBodyWith(BaseHtmlToken token) | ||
{ | ||
//проврка на закрытие | ||
if (!temporaryStack.Any(pair => pair.token.GetType() == token.GetType())) | ||
{ | ||
rootChildren.Push(token); | ||
temporaryStack.Push((position, token)); | ||
return true; | ||
} | ||
|
||
//проверка на неверную вложенность | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Комменатрии в коде являются хорошим маркером того, что метод нужно либо декомпозировать, либо переписать, раз без них трудно понять алгоритм |
||
if (temporaryStack.Peek().token.GetType() != token.GetType()) | ||
{ | ||
var last = temporaryStack.Pop().token; | ||
rootChildren.Push(new TextToken(Current.Value)); | ||
while (last.GetType() != token.GetType()) | ||
last = temporaryStack.Pop().token; | ||
return true; | ||
} | ||
|
||
|
||
if (Previous.Type == StringTokenType.WhiteSpace) | ||
{ | ||
rootChildren.Push(new TextToken(token.Value)); | ||
return true; | ||
} | ||
|
||
return false; | ||
} | ||
|
||
private IEnumerable<BaseHtmlToken> UnitTokenEnds<TOpenToken>() where TOpenToken : BaseHtmlToken | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Название метода не самое удачное, сходу непонятно, что он делает |
||
{ | ||
var list = new List<BaseHtmlToken>(); | ||
while (true) | ||
{ | ||
var child = rootChildren.Pop(); | ||
list.Add(child); | ||
if (child is TOpenToken) | ||
break; | ||
} | ||
|
||
list.Reverse(); | ||
yield return list.First(); | ||
foreach (var node in list.Skip(1).Take(list.Count() - 2).TextifyTags()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Эта строчка очень тяжело воспринимается, непонятно, зачем нам пропускать один элемент, а потом брать только list.Count() - 2, а не все. В конце метода мы еще и последний элемент списка добавляем, что тоже не является очевидным. Переменные на каждую непонятную часть очень пригодились бы, сильно упростили бы понимание того, что этот метод делает |
||
yield return node; | ||
yield return list.Last(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
using Markdown.Tokens.StringToken; | ||
|
||
namespace Markdown.Maker; | ||
|
||
public interface IMaker<T> | ||
{ | ||
public T MakeFromTokens(IEnumerable<StringToken> tokens); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
<Project Sdk="Microsoft.NET.Sdk"> | ||
|
||
<PropertyGroup> | ||
<OutputType>Exe</OutputType> | ||
<TargetFramework>net8.0</TargetFramework> | ||
<ImplicitUsings>enable</ImplicitUsings> | ||
<Nullable>enable</Nullable> | ||
</PropertyGroup> | ||
|
||
</Project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
using Markdown.Maker; | ||
using Markdown.Parser; | ||
using Markdown.Rendered; | ||
using Markdown.Tokens; | ||
using Markdown.Tokens.HtmlToken; | ||
|
||
namespace Markdown; | ||
|
||
public class Md(IParser parser, IMaker<RootToken> maker, IRenderer<RootToken> renderer) | ||
{ | ||
|
||
|
||
public string Render(string input) | ||
{ | ||
var tokens = parser.Parse(input); | ||
var model = maker.MakeFromTokens(tokens); | ||
return renderer.Render(model); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
using Markdown.Tokens.StringToken; | ||
|
||
namespace Markdown.Parser; | ||
|
||
public interface IParser | ||
{ | ||
public IEnumerable<StringToken> Parse(string input); | ||
} |
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.
Очень неочевидное название у класса. Классы-расширения принято называть на основе того, к какому элементу написаны расширения, или для какого сценария