Ardis

Persoonlijke goesting:
Ikzelf zou de method in twee methods splitsen: eentje dat enkel het BO/MO B/M gedeelte afhandelt en eentje dat het DV gedeelte bepaalt.

Dan kan je in je oproepende method beide callen (en op dat hogere niveau het haakje toevoegen). Nu zit dat zo allemaal wat verstopt in deze ene (langere) method en maakt het voor volgenden wat moeilijker om te lezen. Je zou dan in je caller iets krijgen als
Set ..LadeOnderdeelInfo = ..LadeOnderdeelInfo _ ..GeefBodemInfoVoorKoppeling
Set ..LadeOnderdeelInfo = ..LadeOnderdeelInfo _ ")"
Set ..LadeOnderdeelInfo = ..LadeOnderdeelInfo _ ..WatGedoeVoorDiepteVerstelling

Dat is een beetje meer tekst, maar maakt het onderscheid wel wat duidelijker

Aangezien de VakBepaler hier nergens meer gebuikt wordt, mag de injectie weg hé.
Je had dit kunnen weten hé, want je hebt de UnitTest die dit test wèl weggegooid (zie: vhUnitTest.APPS.Halux.PPS.Activiteit.impl.TAOR.common.EtiketInfoBepaler.BepaalEtiketData.Test)

Je hebt perfect uitgevoerd wat op het Jira-kaartje is aangegeven (Sofie heeft het jou gemakkelijk gemaakt ), maar Sofie is een analiste en haar taak bestaat eruit te kijken welke wijzigingen nodig zijn. Onze taak als programmeur gaat dan nog een stapje verder...
Wij moeten waken over de integriteit van de code, op het vlak van (zonder hier volledig te zijn):

• Juistheid.
• Consistentie.
• Uitbreidbaarheid.
• Leesbaarheid.
  Die uitbreidbaarheid is een deur die in beide richtingen draait. Dat impliceert dat je soms de code terug een stuk moet vereenvoudigen, als er implementatie mag verwijderd worden.
  Als ingewikkelde logica door een change request herleid wordt tot het eenvoudig teruggeven van één enkele, onveranderlijke waarde, dan moet je je afvragen wat het nut van die afgezonderde implementatie nog is (moet die nog wel afgezonderd blijven, of mag die terug herleid worden tot het hogere niveau? Dat hogere niveau kan dan een andere private method, een public method of zelfs een method in een andere klasse zijn) . En soms resulteert dit zelfs in het weggooien van een volledige klasse, wat niet erg is. Mocht het ooit nodig zijn dat er toch weer logica bij moet komen, het is allemaal terug te vinden in svn.

Algemeen beschouwd moet je dus, bij het uitvoeren van een change request, jezelf nadien altijd de vraag stellen: "Is de code nu nog wel goed, of is een refactor aangewezen?"
Dikwijls is die refactor dan enkel wat cleanup, maar ook dat is zeker noodzakelijk, want draagt vaak geweldig bij aan de leesbaarheid van de code.

Dit allemaal maar ter inleiding van alle verdere review-opmerkingen

De private method IsVerlaagdeRug wordt dan niet meer gebruikt, dus die mag weg.
De parameter TAORKenmerken is niet meer nodig, dus die mag weg.
Idem in de method GeefVakNummer, waar bovendien ook nog de parameter Rol nu ongebruikt is en dus weg mag.
Idem in de method HeeftVolgendVakNodig, waar bovendien ook nog de parameter Rol nu ongebruikt is en dus weg mag.
Bovendien kan deze method (HeeftVolgendVakNodig) nu vereenvoudigd worden, want:

• (..GeefAantalVakkenNodig(TAORKenmerken) > 1) gaat nu ALTIJD false zijn.
• (..GeefVakNummer(Rol, TAORKenmerken) = ..HuidigVakNummer) gaat nu ALTIJD true zijn.

En als we dan toch bezig zijn...
Deze klasse wordt enkel nog gebruikt door APPS.Halux.common.impl.ProductieSequentie.Taor.AssemblageKar.KarBepaler.
Daarop staat een method VerhoogVakNummer en dat is de enige user van de method VerhoogVakNummer op de VakBepaler.
M.a.w. omdat GeefAantalVakkenNodig de parameter TAORKenmerken niet meer nodig heeft, heeft de method VerhoogVakNummer die ook niet meer nodig, zowel in de VakBepaler als in de VakBepaler.
De KarBepaler wordt enkel gebruikt door de klasse APPS.Halux.common.impl.ProductieSequentie.Taor.AssemblageKarGenerator, waarin de call van VerhoogVakNummer ook voorzien is van de parameter TAORKenmerken, die dus weg mag.
Bovendien kan de method VerhoogVakNummer nu ook eenvoudiger, want zal nu ALTIJD zijn: Set ..HuidigVakNummer = ..HuidigVakNummer + 1.

En ook nog... Omdat in de VakBepaler dan nergens nog gebruik gemaakt wordt van de TAORKenmerken, mag de super DOM.PM.Maatwerk.TAX.impl.Base ook weg.

Na dit alles moet ge maar eens kijken wat er nog overschiet van de VakBepaler, eens ge die volledig gerefactord hebt tot zijn essentie. Waarschijnlijk komt het er dan op neer dat ge ook de KarBepaler serieus kunt refactoren en het gebruik van een VakBepaler dan een beetje idioot gaat zijn (een method in een afgezonderde klasse, gewoon om "1" of "1 + 1" te quiten is nogal onzinnig hé), dus dat de VakBepaler dan momgelijks helemaal verwijderd mag worden. Dat gaat de KarBepaler dan ook weer ten goede komen qua leesbaarheid.

Verdere opkuis nodig!! De abstracte private methods GeefTitelVoorBalikoRugWandHouders en GeefKolommenDefinitieBalikoRugWandHouder mogen dan ook weg hé, want nergens anders nog gebruikt. Ze zijn abstract, dus niet vergeten checken in de derived classes (waar ze dus implementatie zullen hebben, die dus wellicht ook weg mag). Sowieso checken of ze nergens elders voor nodig zijn natuurlijk, maar voor zover als ik al heb gecheckt niet dus.

Aangezien de VakBepaler hier nergens meer gebuikt wordt, mag de parameter weg hé.

Aangezien de VakBepaler hier nergens meer gebuikt wordt, mag de property weg hé.