DoosFactory

De optimizer gaat data zoeken met dikte 3 voor kraft klein en 4.2 voor bv halux. Nadat data gevonden is moeten we er een geheel getal van maken, anders kan het Kraftmachine dit niet interpreteren. Er is dus een verschil tussen voor en na de optimalisatie

Ik snap niet wat je hiermee wilt zeggen. De set BreedtePlano wordt in deze optimizer maximum maar 1 keer gezet per recept?

Dit allemaal bevat enorm veel logica en komt nogal overweldigend over (net zoals Wim aanhaalt in zijn review hierboven) en maakt het vrij moeilijk om te lezen + onder UT te plaatsen. Ik persoonlijk zou dit allemaal wat meer afzonderen en toch wat meer onder unittest plaatsen. Zeker omdat als hier ooit een wijziging aan gebeurt, je mooi kan zien waarom iets faalt. (Ik vind het persoonlijk ook moeilijk hoor om hier een goede structuur voor te bedenken, maar indien nodig wil ik gerust eens mee nadenken )

Ik zou dit ook in een aparte method plaatsen (Do ..Swap(...)) opdat dit alles iets beter leesbaar wordt

Waarom staat deze hier? Is dat ook niet recept specifiek?

EDIT: Dit wordt ook in de optimizer aangepast, dus misschien is het logisch om deze naar daar / aparte klasse te plaatsen?

Als algemene opmerking voor deze klasse zou ik toch een aantal dingen afzonderen in aparte methods / klasses. Zoals deze kan gerust in een aparte method (Set Prioriteit = ..GeefPrioriteit(MachineTechnologie).

Lijntje commentaar mag weg?

Is maar een idee, maar aangezien er hier veel 'code duplication' is, kan je dit eventueel in een aparte method steken a la '..GeefParam(Key, Value)

Ik zou dit als property in de recepten apart plaatsen en opvullen zoals hieronder. Als dit later zou wijzigen, of er meerdere nodig zijn, moet je die sowieso ergens ophalen lijkt mij

Ik zou dit en de overigen in een aparte folder plaatsen als dat nog lukt, iets met dto of iets dergelijks.

EDIT: Ik zie dat Wim deze opmerking ook bij de 'Doos.cls' heeft geplaatst, negeer deze dan maar

Deze, en volgende grote methods, zal ik niet reviewen ... :-P

Bij sommige OptimizeTralala methods staat enkel een IF (...) then set BreedtePlano = iets
Wanneer deze method een tweede keer wordt opgeroepen, en de IF conditie is niet voldaan, dan kon je in een gevaarlijke situatie.

Mogelijke oplossing : in t begin van de Optimize() method de waarde resetten.

ErrorHandling binnen deze story (en in 't algemeen) zouden we toch eens onder het vergrootglas moeten nemen.

Global Mappings :
Best dat je de global naam laat beginnen met LOG.* dan wordt de data in de L-database opgeslagen. Da's beter dan in G0 of G1
--> ^LOG.LogKartonOpdrachten2 gaat naar f:\datam\database\admin\l\0\

Zorg je er ook voor dat deze logging niet tot in de eeuwigheid blijft aangroeien? Tenzij dat de impact minimaal is.

Not(SnijData = "") vervangen door $IsObject(SnijData)

Waarom is dit hard-coded? Niet via config-item?
En is dit geen rare plaats om die location in te stellen? Kan volgens mij even goed in de %OnNew() gezet worden.
Eerste stap naar clean-code en single-responsibility gewijs : dit stukje alvast afzonderen in een method

EARLY QUIT !!!
Verdere refactoring van deze method dringt zich op.
Ik geef je graag nog enkele inzichten mee, maar dan liever mondeling want anders te veel uitleg nodig in deze comment :-P

Dit is voor mij een typisch voorbeeld waar je de conditie eerst in een boolean variabele steekt (cfr. Behavior).
Geef deze variabele een voldoende duidelijke naam, dan kan je alvast een groot deel van de commentaar weglaten.
In dit geval kan het misschien zinvol zijn om zelfs meerdere variabelen te gebruiken. (met de nadruk op "misschien" )

Moeten deze blokken commentaar nog opgekuist worden? Of is dit ter voorbereiding van toekomstige uitbreidingen? Of om een andere reden.
Als reviewer moet ik dit toch melden, hé

"Voor DIT maatwerktype" --> op z'n minst het Product of ProductID vermelden om te kunnen troubleshooten.

In t algemeen : Altijd kritisch nakijken of de foutmelding voldoende informatie bevat indien er iets misloopt.

Idem : StatusException).ThrowIndienNietOK(SaveStatus, ...