PM

Opgelet met de uitdrukking %ClassName(1) = "..."
want dit zal falen op afgeleide klassen --> beter object.%Extends(...) alsook gebruik maken van #class(DOM.PM.Maatwerk.Calc.Common.impl.TBXKenmerken).%ClassName($$$True)

De method naam kan wel tot verwarring leiden, want wat met een LBX-kenmerk?
Ik zou het stukje "Tbx" weglaten en de implementatie lichtjes herschijven, met een lokale variabele en een lijn "If ... %Extends (TBXKenmerken) then ... "

Domme opmerking, maar om de "issen" te onderscheiden zet ik meestal haakjes rond de conditie. LES 1 in caché : haakjes is altijd veiliger dan geen haakjes

LegacyPartijApi :
Door deze implementatie wordt de method GeefLegacyPartijAPI() van de context minstens 5x opgeroepen. Dit valt nu eenmaal op door de aanpassing in deze method.
Dus boyscout-gewijs : graag de LegacyPartijApi aanmaken in de constructor, op de gekende wijze. Voor mij hoeft er nog geen $$$Inject() te staan, gewoon newen is oké.

VHoss : A of B
Misschien VerpakkingType eerst in een variabele steken. Dit maakt de "OF"-condities dan leesbaarder.

VerpakkingType Probox : X of Y

Heb je na de wijziging in de EXTERN... de multicalc voor LBX nog eens getest?

In de LBXLangtekstCreator kan dan een tijdje na de livegang de overbodige code nog wat opgekuist worden maar dat is natuurlijk voor later

De methodnaam 'BevatRol is niet zo terecht. Het zou beter zijn BevatVerwijderdeRol of iets in die aard. Met BevatRol zou ik meteen denken aan de rol op de boom zelf, maar niet dat het eigenlijk gaat over de verwijderde items

Ik ben nogal gevoelig voor overdreven veel keer herhaling van dezelfde GetAt's en dergelijke.

Hierboven staan meer dan 10x KenmernNamen.GetAt(i ) en KenmernWaarden.GetAt(i )
Binnen de for-loop in een lokale variabele steken, zou dit al een stuk rustiger (en beetje performanter) maken.

Idem voor GekoppeldeIK.GetAt(##class(DOM.PM.Maatwerk.Calc.Common.enu.GekoppeldIKType).SpaceStep()) --> SstKenmerken (+ voordeel van code-completion)

Nog een kleine optimalisatie mogelijk :
De uitdrukking If $IsObject(ArrayVanTralala.GetAt(key)) kan je ook vervangen door iets leesbaardere uitdrukking If ArrayVanTralala.IsDefined(key)
N.B.: dit is niet 100% equivalent, want dit covert niet wanneer een array-element met "" (leeg) is ingevuld. Maar in dit geval, bij gekoppeldeMatten is dat normaal gezien niet het geval.
Ik geef in dit geval de voorkeur aan .IsDefined(key)

Hier kan je ook perfect de variabele SstKenmerken = IngegevenKenmerken.GekoppeldeIngegevenKenmerken.GetAt(##class(DOM.PM.Maatwerk.Calc.Common.enu.GekoppeldIKType).SpaceStep())
binnen deze If definieren.

Ik denk dat deze lijn (en de 7 volgende lijnen) duidelijker en copy-paste-"veiliger" zullen worden, wanneer je van dit lijntje een private methodje maakt.
Bvb. Do ZetGekoppeldeIKKenmerkWaarde(GekoppeldeIK.GetAt(##class(DOM.PM.Maatwerk.Calc.Common.enu.GekoppeldIKType).MatY()), "Kleur", KenmerkWaarden.GetAt(i ) )

Cosmetica = hoofdletters, spaties, accolades, open lijnen, en haakjes (dit laatste is soms een randgeval, wanneer het om complexe condities gaat)
Aan de lijnen hierboven is niks anders gewijzigd.
Probeer de cosmetica in een aparte commit te doen, dan kan de reviewer die commit skippen indien gewenst.

Clean code principle : het aspect "Visitor" verbergen/wegwerken en de Accept-method niet rechtstreeks vanuit uw implementatie oproepen
Hiervoor zijn volgende stappen nodig :

• klassenaam wijzigen van "HalffabrikatenVisitor" naar "HalffabItemNaarDtoConverter", idem voor KostItemNaarDtoConverter
• de visitor/converter heeft 1 public method, die de .Accept() implementeert en het resultaat rechtstreeks teruggeeft.
  In dit geval wordt dit dan logischerwijs "ConverteerNaar...Dto" (of zoiets)

  Method ConverteerNaarHalffabItemDto(DomHallfabItem As DOM.PM.Maatwerk.Calc.HF.IHalffabItem) As DOM.PM.Maatwerk.Calc.HF.dto.HalffabItem
  {
  	Do DomHallfabItem.Accept(this)
  	Quit ..HalffabItem.HalffabItems.GetAt(1)
  }
  
  en : 
  	Set MaatwerkDetails.Halffabrikaten = HalffabItemNaarDtoConverter.ConverteerNaarHalffabItemDto(HalffabItemsBoom.Wortel)
  

• de method GeefHalffabItem() mag dan weg

Zowel uw oproepende code als uw visitor zullen hierdoor properder en duidelijker worden, en de bijhorende UT kan zeer eenvoudig mee aangepast worden.

N.B. ik weet dat de visitor reeds bestond en in het gedeelte daaronder reeds werd gebruikt, dus deze review-opmerking was in feite aan uw voorganger bestemd :-P

$$$HasLength() is alleen zinvol op een %String.
"Problemen" is een object --> $IsObject() en/of List.Count() gebruiken

Balans tussen YAGNI en self-documenting code.
Wanneer de MaatwerkService vanuit een andere omgeving opgeroepen wordt, dan kan onderscheid gemaakt worden door de IsFactoryViaVBA op false te zetten ... of zoiets :-P