You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
ich habe mal etwas gründlicher über den Code geschaut und fasse in diesem Issue ein paar Anmerkungen und Verbesserungsvorschläge zusammen. Wir können gern morgen zu allen Punkten ausführlich sprechen :)
Wenn eine Funktion oder Methode nichts zurück gibt, dann bitte nach Möglichkeit immer void als Return Type angeben
Glossary
Nach DDD-Begriffen ist die Klasse Glossary sowas wie Dein Repository. D.h. Methoden wie beforeGlossaryPublished, saveGlossaryInCache und readGlossaryIndexFromCache sind hier sehr gut aufgehoben.
Die Methoden extractGlossaryIndexFromNode und glossaryDuplicates hingegen gehören eher in eine Factory. Ich denke, dass das Domänenmodell von einer Entity GlossaryIndex profitieren würde, die quasi als AggregateRoot fungiert. GlossaryIndex könnte eine static factory method fromNode beinhalten, die die Funktion von extractGlossaryIndexFromNode übernimmt und zugleich den Integritätscheck über glossaryDuplicates durchführt.
GlossaryEntryFactory
Die GlossaryEntryFactory könnte durch eine static factory method auf der Klasse GlossaryEntry eingespart werden:
Die FlowQuery Operation find ist relativ teuer. Lass uns hier morgen mal nach Möglichkeiten suchen, wie wir diesen Query-Algorithmus optimieren können, sodass der zweite find-Aufruf eingespart werden kann.
Darüber hinaus denke ich, dass das Auffinden dieser Nodes eine Aufgabe ist, die die Klasse Glossary als Repository übernehmen könnte.
Anstelle des custom HTML parsing Algorithmus, der in dieser Zeile anfängt, empfehle ich den Einsatz einer Third-Party Library. Hervorragend geeignet für dieses Problem ist z.B. diese hier: https://github.com/Masterminds/html5-php
Der Grund: Das Parsen von HTML ist nicht-trivial und es ist in der Tat unmöglich HTML korrekt mit Regular Expressions zu parsen (dieser Beitrag erklärt das Problem sehr gut: https://stackoverflow.com/a/6751339/15130211). Mit Regular Expressions kann das Problem nur sehr grob gelöst werden und zahlreiche Edge-Cases werden dann nicht beachtet. Was passiert z.B. wenn der Begriff bereits anderweitig verlinkt ist? Mit einem echten HTML-Parser können Probleme dieser Art ausgeschlossen werden.
Es würde sich außerdem lohnen, den Algorithmus in eine eigene Utility-Klasse auszulagern und mit reichlich Unit Tests zu versehen. Aber auch mit HTML-Parser wird der Algorithmus am Ende komplexer aussehen. Hier ist eine Skizze, wie ich mir das vorstellen könnte:
useMasterminds\HTML5;
// ...class HTMLTermReplacer
{
publicstaticfunctionreplaceTerms(string$markup, array$terms, callable$onReplaceTerm): string
{
$html5 = newHTML5();
$doc = $html5->loadHTML($markup);
$xpath = new \DOMXPath($doc);
// Namespace needs to be registered due to limitations in Masterminds\HTML5// see: https://github.com/Masterminds/html5-php/issues/57$xpath->registerNamespace('x', 'http://www.w3.org/1999/xhtml');
// The XPath Query selects all text nodes that are not// descendants of links$nodes = $xpath->query('//text()[not(ancestor::x:a)]');
foreach ($nodesas$node) {
foreach ($termsas$term) {
// Regex left out for simplicity's sake$matches = preg_split('/(...)/', $node->nodeValue, -1, PREG_SPLIT_DELIM_CAPTURE);
if (count($matches) > 1) {
$fragment = $doc->createDocumentFragment();
foreach ($matchesas$match) {
if ($match === $term) {
$link = $onReplaceNode($doc, $term);
$fragment->appendChild($link);
} else {
$text = $doc->createTextNode($match);
$fragment->appendChild($text);
}
}
$node->parentNode->replaceChild($fragment, $node);
}
}
}
return$html5->saveHTML($doc->childNodes[1]->childNodes);
}
}
Lass uns gern morgen über die Umsetzung und Teststrategie sprechen :)
The text was updated successfully, but these errors were encountered:
Hi Masoud,
ich habe mal etwas gründlicher über den Code geschaut und fasse in diesem Issue ein paar Anmerkungen und Verbesserungsvorschläge zusammen. Wir können gern morgen zu allen Punkten ausführlich sprechen :)
REST-Controller
https://github.com/hedayati-m/Sitegeist.Nomenclator/blob/e7d868b556a53fe95b6683fc7560fb6c6182ae81/Classes/Application/Controller/GlossaryEntryController.php#L28
JsonView::class
zu referenzieren, anstatt den Klassennamen auszuschreibenhttps://github.com/hedayati-m/Sitegeist.Nomenclator/blob/e7d868b556a53fe95b6683fc7560fb6c6182ae81/Classes/Application/Controller/GlossaryEntryController.php#L30
void
als Return Type angebenGlossary
Nach DDD-Begriffen ist die Klasse
Glossary
sowas wie Dein Repository. D.h. Methoden wiebeforeGlossaryPublished
,saveGlossaryInCache
undreadGlossaryIndexFromCache
sind hier sehr gut aufgehoben.Die Methoden
extractGlossaryIndexFromNode
undglossaryDuplicates
hingegen gehören eher in eine Factory. Ich denke, dass das Domänenmodell von einer EntityGlossaryIndex
profitieren würde, die quasi als AggregateRoot fungiert.GlossaryIndex
könnte eine static factory methodfromNode
beinhalten, die die Funktion vonextractGlossaryIndexFromNode
übernimmt und zugleich den Integritätscheck überglossaryDuplicates
durchführt.GlossaryEntryFactory
Die
GlossaryEntryFactory
könnte durch eine static factory method auf der KlasseGlossaryEntry
eingespart werden:GlossaryEntry
https://github.com/hedayati-m/Sitegeist.Nomenclator/blob/e7d868b556a53fe95b6683fc7560fb6c6182ae81/Classes/Domain/GlossaryEntry.php#L11
final
markiert werdenLinkTermsToGlossaryImplementation
https://github.com/hedayati-m/Sitegeist.Nomenclator/blob/e7d868b556a53fe95b6683fc7560fb6c6182ae81/Classes/Fusion/LinkTermsToGlossaryImplementation.php#L45-L51
Die FlowQuery Operation
find
ist relativ teuer. Lass uns hier morgen mal nach Möglichkeiten suchen, wie wir diesen Query-Algorithmus optimieren können, sodass der zweitefind
-Aufruf eingespart werden kann.Darüber hinaus denke ich, dass das Auffinden dieser Nodes eine Aufgabe ist, die die Klasse
Glossary
als Repository übernehmen könnte.https://github.com/hedayati-m/Sitegeist.Nomenclator/blob/e7d868b556a53fe95b6683fc7560fb6c6182ae81/Classes/Fusion/LinkTermsToGlossaryImplementation.php#L61
https://github.com/hedayati-m/Sitegeist.Nomenclator/blob/e7d868b556a53fe95b6683fc7560fb6c6182ae81/Classes/Fusion/LinkTermsToGlossaryImplementation.php#L63
return $this->fusionValue('value') ?? '';
)https://github.com/hedayati-m/Sitegeist.Nomenclator/blob/e7d868b556a53fe95b6683fc7560fb6c6182ae81/Classes/Fusion/LinkTermsToGlossaryImplementation.php#L69
@return null|TraversableNodeInterface
sein - Context Values aus Fusion sind nicht null-safehttps://github.com/hedayati-m/Sitegeist.Nomenclator/blob/e7d868b556a53fe95b6683fc7560fb6c6182ae81/Classes/Fusion/LinkTermsToGlossaryImplementation.php#L71
https://github.com/hedayati-m/Sitegeist.Nomenclator/blob/e7d868b556a53fe95b6683fc7560fb6c6182ae81/Classes/Fusion/LinkTermsToGlossaryImplementation.php#L100
Anstelle des custom HTML parsing Algorithmus, der in dieser Zeile anfängt, empfehle ich den Einsatz einer Third-Party Library. Hervorragend geeignet für dieses Problem ist z.B. diese hier: https://github.com/Masterminds/html5-php
Der Grund: Das Parsen von HTML ist nicht-trivial und es ist in der Tat unmöglich HTML korrekt mit Regular Expressions zu parsen (dieser Beitrag erklärt das Problem sehr gut: https://stackoverflow.com/a/6751339/15130211). Mit Regular Expressions kann das Problem nur sehr grob gelöst werden und zahlreiche Edge-Cases werden dann nicht beachtet. Was passiert z.B. wenn der Begriff bereits anderweitig verlinkt ist? Mit einem echten HTML-Parser können Probleme dieser Art ausgeschlossen werden.
Es würde sich außerdem lohnen, den Algorithmus in eine eigene Utility-Klasse auszulagern und mit reichlich Unit Tests zu versehen. Aber auch mit HTML-Parser wird der Algorithmus am Ende komplexer aussehen. Hier ist eine Skizze, wie ich mir das vorstellen könnte:
Lass uns gern morgen über die Umsetzung und Teststrategie sprechen :)
The text was updated successfully, but these errors were encountered: