Skip to content
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

Кирилл Зарипов #232

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions cs/Markdown/Converters/HtmlConverter.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
using System.Text;

namespace Markdown.Converters;

public class HtmlConverter : IConverter
{
private static readonly Dictionary<TokenType, string> HtmlTag = new()
{
{ TokenType.Italic, "em" },
{ TokenType.Strong, "strong" },
};

public string Convert(List<Token> tokens)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Давай принимать обобщенные типы)
В этом случае вообще IEnumerable достаточно

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{
var html = new StringBuilder();
var isClosed = new Dictionary<TokenType, bool>
{
{TokenType.Italic, true},
{TokenType.Text, true},
{TokenType.Strong, true},
};

foreach (var token in tokens)
{
html.Append(token.Type switch
{
TokenType.Text => token.Content,
TokenType.Italic or TokenType.Strong when token.Pair != null =>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Нарушение SRP. Класс должен нагенерить html по входным данным, а он еще что-то проверяет

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

убрал проверку

isClosed[token.Type] ? Tag.Open(HtmlTag[token.Type]) : Tag.Close(HtmlTag[token.Type]),
_ => token.Content
});

if (token.Pair != null)
{
isClosed[token.Type] = !isClosed[token.Type];
}
}

return html.ToString();
}
}
6 changes: 6 additions & 0 deletions cs/Markdown/Converters/IConverter.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
namespace Markdown.Converters;

public interface IConverter
{
public string Convert(List<Token> tokens);
}
10 changes: 10 additions & 0 deletions cs/Markdown/Markdown.csproj
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>net9.0</TargetFramework>
<ImplicitUsings>enable</ImplicitUsings>
<Nullable>enable</Nullable>
</PropertyGroup>

</Project>
51 changes: 51 additions & 0 deletions cs/Markdown/Md.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
using System.Text;
using Markdown.Converters;
using Markdown.Tokenizers;

namespace Markdown;

public class Md(ITokenizer tokenizer, IConverter converter)
{
public string Render(string markdown)
{
if (string.IsNullOrEmpty(markdown))
return string.Empty;

var result = new StringBuilder();
var paragraphs = markdown.Split(Environment.NewLine);

foreach (var paragraph in paragraphs)
{
var htmlLine = ProcessParagraph(paragraph);
result.AppendLine(htmlLine);
}

return result
.ToString()
.TrimEnd(Environment.NewLine.ToCharArray());
}

private string ProcessParagraph(string line)
{
var trimmedLine = line.TrimEnd(Environment.NewLine.ToCharArray());

if (trimmedLine.StartsWith("# "))
{
var headerContent = trimmedLine[2..];
var htmlContent = ParseMarkdown(headerContent);
return Tag.Wrap("h1", htmlContent);
}
else
{
var htmlContent = ParseMarkdown(trimmedLine);
return htmlContent;
}
}

private string ParseMarkdown(string markdown)
{
var tokens = tokenizer.Tokenize(markdown);
var html = converter.Convert(tokens);
return html;
}
}
15 changes: 15 additions & 0 deletions cs/Markdown/Program.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
using Markdown;
using Markdown.Converters;
using Markdown.Tokenizers;

var tokenizer = new KonturMdTokenizer();
var converter = new HtmlConverter();
var markdown = new Md(tokenizer, converter);

const string input = """
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Советы категории Б: можешь вынести эту строчку в константу, чтобы потом, при добавлении новых примеров, можно было легко между ними переключаться :)

        const string firstExample = "This _is_ a __sample__ markdown _file_.\n";
        const string secondExample = "#This is another __sample__ markdown _file_";
        var input = firstExample;

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А еще давай сюда буду накидывать, в каких случаях все ломается:

  1. ___Some Text___ - должно стать <strong><em>Some</strong></em>
  2. __Some_ - должно стать _<em>Some</em>
  3. Some_Another_Text - не должно измениться
  4. 123\456 - не должно измениться

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

исправил эти случаи

Подчерки внутри текста c цифрами_12_3 не считаются выделением и должны оставаться символами подчерка.
""";

var result = markdown.Render(input);

Console.WriteLine(result);
10 changes: 10 additions & 0 deletions cs/Markdown/Tag.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
namespace Markdown;

public static class Tag
{
public static string Open(string tagName) => $"<{tagName}>";

public static string Close(string tagName) => $"</{tagName}>";

public static string Wrap(string tagName, string content) => $"{Open(tagName)}{content}{Close(tagName)}";
}
11 changes: 11 additions & 0 deletions cs/Markdown/Token.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
namespace Markdown;

public class Token
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Мб сделать record, раз это Data class?
  2. Слишком много полей, это указывает на две вещи: размытую логику => нарушение SRP и недостаточную декомпозицию

Условно, мне все еще с трудом понятно зачем иметь bool IsClosing и IsOpening, особенно, когда есть Token? Pair

Непонятно, как работать с этим классом, без полного прочтения кода.

Copy link
Author

@kokodio kokodio Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

переделал логику с IsClosing & IsOpening

{
public TokenType Type { get; init; }
public required string Content { get; init; }
public bool IsClosing { get; init; }
public bool IsOpening { get; init; }
public int Position { get; init; }
public Token? Pair { get; set; }
}
8 changes: 8 additions & 0 deletions cs/Markdown/TokenType.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
namespace Markdown;

public enum TokenType
{
Text,
Italic,
Strong
}
6 changes: 6 additions & 0 deletions cs/Markdown/Tokenizers/ITokenizer.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
namespace Markdown.Tokenizers;

public interface ITokenizer
{
public List<Token> Tokenize(string markdown);
}
201 changes: 201 additions & 0 deletions cs/Markdown/Tokenizers/KonturMdTokenizer.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,201 @@
using System.Text;

namespace Markdown.Tokenizers;

public class KonturMdTokenizer : ITokenizer
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В этом классе намешано все и сразу.
Скажем так, даже после прочтения через 5 минут забываешь, что и зачем эти методы делают.
На Clean code это точно не похоже)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Давай начнем отсюда: почему _Kontur_MdTokenizer ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Я часть спецификации не так понял и реализовал ее не так как она работает в гитхабе, поэтому подумал это контуровский Md)

{
public List<Token> Tokenize(string markdown)
{
var text = new StringBuilder();
var tokens = GetTokens(markdown, text);

CreateTokenPairs(tokens, TokenType.Italic, markdown);
CreateTokenPairs(tokens, TokenType.Strong, markdown);
FilterStrongInsideItalic(tokens);
FlushText(tokens, text);

return tokens;
}

private void FlushText(List<Token> tokens, StringBuilder text)
{
if (text.Length > 0)
tokens.Add(CreateTextToken(text.ToString()));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Непонятное название метода
  2. Метод меняют данные внутри себя, а не возвращают ответ (Pure functions передают привет)
  3. Три метода вызываются в одной строчке, тяжело читать/понимать, что в какой последовательности вызовется.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

метода больше нету

}

private void FilterStrongInsideItalic(List<Token> tokens)
{
var isItalicOpen = false;
Token? currentItalicOpening = null;

foreach (var token in tokens)
{
if (token.Pair == null)
continue;

switch (token.Type)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Виднеется закономерность: как только в коде появляется switch/case он становится менее читаемым.
Как раз для таких вещей придумали полиморфизм)

{
case TokenType.Italic:
isItalicOpen = !isItalicOpen;
currentItalicOpening = isItalicOpen ? token : null;
break;
case TokenType.Strong when isItalicOpen:
RemoveTokenPair(currentItalicOpening);
RemoveTokenPair(token);
isItalicOpen = false;
break;
}
}
}

private void RemoveTokenPair(Token? token)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Метод можно унести в сам Token

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{
if (token?.Pair == null)
return;

token.Pair.Pair = null;
token.Pair = null;
}

private List<Token> GetTokens(string text, StringBuilder sb)
{
var tokens = new List<Token>();

for (var index = 0; index < text.Length;)
{
switch (text[index])
{
case '\\':
index = HandleEscape(text, index, sb);
break;

case '_':
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

При добавлении новых тэгов нужно будет расширять этот Switch до гигантских масштабов. В общем, главная проблема - что оно не очень масшабируемое. Да и понять, где нужно масштабировать довольно тяжело (нужно прочитать весь код)
Сократить можно либо через switch expression, либо через использование словаря

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

И вынести теги в константы, они сейчас по всему коду разбросаны

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

index = HandleUnderscore(text, index, sb, tokens);
break;

default:
sb.Append(text[index++]);
break;
}
}

return tokens;
}

private int HandleEscape(string markdown, int index, StringBuilder text)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Он удаляет одиночные \, так не должно быть)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{
if (index + 1 < markdown.Length)
{
text.Append(markdown[index + 1]);
return index + 2;
}

text.Append(markdown[index]);
return index + 1;
}

private int HandleUnderscore(string markdown, int index, StringBuilder text, List<Token> tokens)
{
var count = CountUnderscores(markdown, index);
var tokenType = count == 2 ? TokenType.Strong : TokenType.Italic;

if (IsValidDelimiter(markdown, index, count, out var isOpening, out var isClosing))
{
if (text.Length > 0)
{
tokens.Add(CreateTextToken(text.ToString()));
text.Clear();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Три метода вызываются в одной строчке.
Без доп. погружения непонятно, что тут происходит, сильно нарушен SRP

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

tokens.Add(new Token
{
Type = tokenType,
Content = new string('_', count),
IsOpening = isOpening,
IsClosing = isClosing,
Position = index
});

return index + count;
}

text.Append(new string('_', count));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new string('_', count) дублируется.
Да и я бы сказал, что завязка на _ слишком сильная. Это ведь может быть любой другой символ)
Я, например, обычно использую **Text** вместо __Text__. А так md поддерживает и тот, и другой.
Я к тому, что будет тяжело масшабировать

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

переделал в dictionary

return index + count;
}

private int CountUnderscores(string text, int index)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Попробую описать, что делает метод: Считает идет 1 или 2 _ подряд.
Название CountUnderscores точно не подходит.
И, кажется, проблема не в названии, а в сути метода. Мне кажется, тут больше подойдет метод, который определяет "что за токен перед нами", в который можно вынести подобную логику.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{
var result = 0;

if (text[index] == '_') result++;
if (index + 1 < text.Length && text[index + 1] == '_') result++;

return result;
}

private bool IsValidDelimiter(string markdown, int index, int length, out bool isOpening, out bool isClosing)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Столько входящих параметров тяжело переварить человеческому мозгу... Точнее, ответить на вопрос: "Зачем для проверки на валидность нужно 5 аргументов"
А еще out параметры имеются...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

тоже удалил

{
var before = index > 0
? markdown[index - 1]
: '\0';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Почему именно \0?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Это Null символ, в C используется как конец строки

var after = index + length < markdown.Length
? markdown[index + length]
: '\0';

isClosing = IsDelimiter(before);
isOpening = IsDelimiter(after);

return isOpening || isClosing;
}

private bool IsDelimiter(char value) => value != '_' && (char.IsLetter(value) || char.IsPunctuation(value));

private Token CreateTextToken(string content) => new() { Type = TokenType.Text, Content = content };
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Такое можно вынести в сам Token. Или даже лучше в его Extension методы


private void CreateTokenPairs(List<Token> tokens, TokenType type, string markdown)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Еще перед прочтением кода - метод капец какой длинный, его тяжело читать. Нужно держать слишком много логики в голове

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

После прочтения - да, слишком много логики.
И switch pattern в комбинации с IsOpening, IsClosing добивает это все.
Опять изменение состояний объекта.
Изначально подход сомнительный и не оптимальный, потому что после приходится бегать и удалять "неправильные" пары. Такое обычно делается при помощи КДА

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ДКА интрересная штука, но сейчас уже что есть то есть

{
var pairableTokens = new Stack<int>();

for (var i = 0; i < tokens.Count; i++)
{
var token = tokens[i];
if (type != token.Type) continue;

switch (token.IsOpening, token.IsClosing)
{
case (true, false):
pairableTokens.Push(i);
break;

case (false, true):
if (pairableTokens.Count > 0)
{
var openingIndex = pairableTokens.Pop();
tokens[openingIndex].Pair = tokens[i];
tokens[i].Pair = tokens[openingIndex];
}
break;

case (true, true):
if (pairableTokens.Count > 0)
{
var openingIndex = pairableTokens.Peek();
var lenght = token.Position - tokens[openingIndex].Position;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

length

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var slice = markdown.AsSpan().Slice(tokens[openingIndex].Position, lenght);

if (!slice.Contains(' '))
{
pairableTokens.Pop();
tokens[openingIndex].Pair = tokens[i];
tokens[i].Pair = tokens[openingIndex];
}
}
else
{
pairableTokens.Push(i);
}
break;
}
}
}
}
Loading