-
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
Заколюкин Степан #230
base: master
Are you sure you want to change the base?
Заколюкин Степан #230
Conversation
cs/Markdown/StringExtension.cs
Outdated
internal static class StringExtension | ||
{ | ||
public static string PerformTextFormatting( | ||
this string text, Func<string, int, PortionText> getNextFragmentToFormat, |
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.
Делать экстеншны от базовых типов нужно только если очень хочется
Почему: если напишешь var a = "text".
и попросишь IDE предложить варианты после точки, то она предложит тебе кучу самописных методов, бесполезных тебе в контексте. Конкретно этот метод предложится в методе Main, в котором он скорее всего не нужен
Исключения: штуки, которые могут пригодиться во всём проекте. Например, для стринга ты написал метод, который какой-то магией делит текст на массив предложений. Если пишешь парсер текстов, то этот метод будет использоваться часто, и его можно обернуть в экстеншн
Можно просто класс сделать internal, и тогда страдать будут только разработчики конкретного пакета, но я бы вообще убрал метод в реализацию Md, тк кроме того класса этот метод не нужен
cs/Markdown/PortionText.cs
Outdated
@@ -0,0 +1,18 @@ | |||
namespace Markdown; | |||
|
|||
internal class PortionText |
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.
А здесь если хотелось назвать "Порцаия текста", то правильнее будет "TextPortion"
InputOpeningTag = inputOpeningTag ?? throw new ArgumentNullException(nameof(inputOpeningTag)); | ||
InputClosingTag = inputClosingTag ?? inputOpeningTag; | ||
OutputOpeningTag = outputOpeningTag ?? throw new ArgumentNullException(nameof(outputOpeningTag)); | ||
OutputClosingTag = outputClosingTag ?? outputOpeningTag[0] + "/" + outputOpeningTag.Substring(1); |
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.
И кста стоит понатыкать вопросов у нулабельных типов
OutputClosingTag = outputClosingTag ?? outputOpeningTag[0] + "/" + outputOpeningTag.Substring(1); | ||
|
||
if (invalidSubstringsInMarkup.Any(substring => substring.Length > inputOpeningTag.Length + 2)) | ||
throw new ArgumentException("Запрещенные подстроки не могут быть длиннее inputOpeningTag.Length + 2"); |
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
result.Add(new SingleReplacementTagSpecification([], | ||
"# ", "<h1>")); | ||
|
||
return result; |
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.
Идея выделить правила обработки в отдельный объект тоже неплохая, я бы даже выделил этот метод в отдельный класс типа MdToHtmlSpecificationBuilder и вызывал его отсюда
По хорошему видится такая структура: есть ISpecificationProvider с методом GetMarkupSpecification, который возвращает массив спецификаций, его реализует MdToHtmlSpecificationBuilder с кодом из этого метода, а Md имеет в конструкторе Md(ISpecificationProvider specificationProvider) { /* тут задать приватное ридонли поле*/}
, а метод GetMarkupSpecification вызывается внутри Render, и так ты сможешь в любой момент всунуть ему другую спецификацию
cs/Markdown/Md.cs
Outdated
var markupSpecification = GetMarkupSpecification().ToArray(); | ||
|
||
return RemoveEscapingOfControlSubstrings(PerformTextFormatting(markdown, | ||
FindAllSubstringsForFormatting(markdown, markupSpecification)), markupSpecification); |
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.
Тут бы навыделять переменных:
var markupSpecification = GetMarkupSpecification().ToArray();
var fragments = FindAllSubstringsForFormatting(markdown, markupSpecification);
var renderedString = PerformTextFormatting(markdown, fragments);
return RemoveEscapingOfControlSubstrings(renderedString, markupSpecification);
Так легче читается
{ | ||
public SingleReplacementTagSpecification(IEnumerable<string> invalidSubstringsInMarkup, | ||
string inputOpeningTag, string outputOpeningTag) | ||
: base(invalidSubstringsInMarkup, inputOpeningTag, outputOpeningTag, "\n\r") |
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.
Докопаюсь до \n\r: это для винды такой перенос. Для линуха: \n. В шарпе есть Evironment.NewLine, который в зависимости от системы разный
cs/Markdown/TagReplacementOptions.cs
Outdated
public readonly string OldTag; | ||
public readonly string NewTag; | ||
public readonly int StartIndex; |
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 readonly:
- либо private type fieldName для изменяемых полей в реализации, что встречается редко
- либо private readonly type fieldName для неизменяемых полей (чаще всего зависимостей, например, ISpecificationProvider, описанный выше)
- либо public type PropertyName { get; } для данных, которые нужны снаружи, с добавлением
set;
если надо уметь изменять данные извне илиprivate set;
если извне изменять нельзя, а изнутри надо
Вообще классы у нас чаще всего делятся на те, что хранят данные, и те, что совершают работу с данными. Этот класс хранит данные, поэтому должен быть утыкан свойствами, а не полями
А вот класс Md совершает работу, и там будет не круто видеть публичные поля или свойства, тк его дело - сделать работу и попасть под каток сборщика мусора. А IScpecificationProvider призван помогать выполнять работу, поэтому он называется зависимостью Md и чаще всего помечается как private readonly
public readonly string OutputOpeningTag; | ||
public readonly string OutputClosingTag; |
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.
Вот эти поля можно объединить в объект Tag:
public record Tag(string Opening, string Closing);
а тут будет:
public Tag InputTag { get; }
public Tag OutputTag { get; }
cs/Markdown/Md.cs
Outdated
foreach (var tagSpecific in markupSpecification) | ||
{ | ||
foreach (var fragment in FindAllFragmentsHighlightedByTag(tagSpecific, text)) | ||
yield return fragment; | ||
} |
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.
Так мы ж всё равно для каждого нового тега дополнительный раз гоняем по тексту. Тогда алгоритм работает за O(textLength * tagsCount), то есть каждый новый тег будет чуть замедлять работу.
Я думал цикл поиска будет выглядеть так:
- циклом гонимся по тексту
- Нашли что какие-то теги из списка подходят к символу по индексу. Если только один, то норм, если больше , то берём тот, что длинее (то есть из одинарного и двойного прочерка берётся двойной)
- Если в этот момент был открыт контекст другого тега (например, жирный тег найден в рамках заголовка), то смотрим, не соответствует ли он invalidSubstrings этого контекста. Соответствует - пропускаем
- Если был открыт контекст этого же тега (то есть найден закрывающий, а ранее был найден открывающий), пишем, что всё норм, найден открывающий тут, закрывающий тут, а контекст отпускаем
- Открываем этот контекст, чтобы на будущих итерациях закрыть
- Добежали до конца текста - смотрим, какой контекст остался открыт (не нашлось закрытия), дописываем их в конец, чтобы вёрстка не поплыла
Тогда тебе не понадобится (вроде бы 🌚) самописное дерево, а понадобится встроенный в систему Stack, в который ты будешь писать активные на конкретную итерацию контексты и смотреть по ним invalidSubstring
И ещё не стоит забывать про обработку пересечений тегов типа #text _text \n text_ text text"
, в котором на какой-то итерации найдётся, что закрывается контекст, внутри которого ещё контекст. Тогда по идее надо просто дропать незакрытый контекст, потому что кто-то накосячил в тексте, не наша проблема. В примере первый курсив просто не будет считаться
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.
Если в этот момент был открыт контекст другого тега (например, жирный тег найден в рамках заголовка), то смотрим, не соответствует ли он invalidSubstrings этого контекста. Соответствует - пропускаем
Есть ощущение, что упадет на вот таком случае: "__пересечения _двойных__ и одинарных_ подчерков"
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.
В спецификации к заданию сказано:
В случае пересечения двойных и одинарных подчерков ни один из них не считается выделением.
A здесь выделение должно сработать по твоему алгоритму
private bool CheckForbiddenPrefixes(string substring, IEnumerable<string> forbiddenPrefixes) | ||
{ | ||
if (forbiddenPrefixes.Any(substring.EndsWith)) | ||
return false; | ||
return substring[1] != '\\' || substring.StartsWith(@"\\"); | ||
} |
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.
Возвращаясь к делению объектов на хранилки и работников, хранилка может иметь какую-то простую логику, но если операция сложнее простейшей арифметики или навешивания единичных методов, то лучше такое либо выносить в вызывающий код, либо в TagreplacementSpecificationExtensions
@Inree