Comment Search

Terechte opmerking.
Early Quit als eerste lijn van een method, dat is nog wel aanvaardbaar.
Wat echter niet aanvaardbaar zou zijn, is wanneer die Quit middenin een blok code staat.

Hoe dan ook, deze lijn code bevat een tweede probleem, namelijk dat het teruggeven van "" (leeg) zou de code verderdoor doen crashen, wat enigzins mijn bedoeling was, bij gebrek aan een "Empty ResultSetIterator".
Daarom heb ik het aangepast van early Quit "" naar Throw Exception.
Bovendien heb ik de method private gemaakt, want ik zie zie geen andere gebruikers dan in deze klasse.

Style:

• Codebase is volledig in het Nederlands, waarom deze comment dan in het Engels?

No big deal.
Codebase in t Nederlands, maar StringUtils.Replace() is Engels, dus wellicht geen taal-reset van mijn brains

Maar in feite zetten we geen commentaar in onze code (= clean code principle!)
Hier, in dit specifiek geval is het echter om te benadrukken dat ALLE spaties moeten vervangen worden.
Uitzondering bevestigt de regel.

Als reviewer zie ik staan : "zijkanten toegevoegd", dan verwacht ik geen wijzigingen aan de Bodem-component.
ik vermoed dat je het nulpunt of de orientatie (van bodem) hebt aangepast om beter uit te komen met de zijkanten.
Dat kan je dan ook best kort omschrijven in de commit-message van de zijkanten.

FYI : Commit-messages aanpassen, dat kan, maar wel met enkele rand-opmerkingen :-P

De wijziging zelf is inderdaad de oriëntatie van de bodem veranderen van 'vooruit' naar 'achteruit' om conform te zijn met de zijkanten.
Dit was zonder de zijkanten lastig te controleren omdat er geen ander 'frame of refference' is.

Je opmerking over de commit-message is geheel terecht.
Ik kijk aan om dit in SVN aan te vullen, maar wat ik begrepen heb van de korte uitleg van Tom is dat dit enkel binnen SVN-log zichtbaar is.
Andere tools zoals Crucible hier blijven de originele message tonen.

Verwijder gerust ook deze en volgende method aangezien deze niet meer gebruikt worden

Om later geen verwarring te hebben met de uitsparing voor sifonlade, zou ik deze variabelen nu al iets concreter benoemen.
Wat denk je van "OnderkantBodemUitfrezingBreedte" en "OnderkantBodemUitfrezingDiepte" ?

De lijst van mappings zal steeds groter en onoverzichtelijker worden.
Ik probeer me in te beelden of het beter is om alle BOM_Components samen te zetten; idem voor characteristics van ConfiguratorTAB en voor losse parameters (e.g. DiktePlaatmateriaal)

Tip om de dependencies juister te leggen :
maak een variabele genaamd "AfstandZijkantRechts" of "VerplaatsingZijkantRechts" (mijn voorkeur) en stel deze gelijk aan BodemBreedte.
Dit lijkt banaal, maar het wordt wellicht meteen duidelijk :
Voor alle onderdelen die je (fysiek) vastklikt op de ladezijkant, geef je de rechtse component een Translate X in functie van VerplaatsingZijkantRechts i.p.v. BodemBreedte
Zo zullen de afdekkappen/reling/inschuifElement/rugwandhouder/ ... bij eventuele latere aanpassingen aan Bodembreedte steeds op de rechtse zijkant blijven zitten.

moet dit ook op facturen? Geen idee meer of dit direct in orde komt (als het al nodig is).

Vergeet zeker het logo niet op de server te zetten

Is deze URL die naar www-test wijst wel correct in productie?
Om te ontwikkelen en voor validatie kan ik deze begrijpen, maar voor productie vind ik dit vreemd.
In het Jira ticket staat hier ook niets over vermeld.

Productie-omgeving nog niet beschikbaar bij iO (of toch nog niet gemeld aan mij :P )

-> Staat niet vermeld in jira :
Klopt, enerzijds heb ik wel iets geschreven in de svn commit message; anderzijds beetje bewust gedaan omdat ik zelf wist dat er nog andere review-opmerkingen zouden zijn.
Verdere afhandeling in vervolg-kaartje jira ICT-3847

Een kleine praktische overweging:

Boven de scheidingslijn van "BomComponenten" staat nog "CB_VW_BOM_Components"
dat maakte mij eerst effe verward ... en toen zag ik dat het om Submodule characteristics gaat :-P
Misschien te overwegen : Submodule characteristics ook groeperen, met extra scheidingslijn. Dit is puur convenience, gevoelsmatig, heeft uiteraard geen impact op de correcte werking van de 3D-visu.

Belangrijk : VoorwandTypeVanMBinnenlade kan mogelijk ook ingevuld zijn wanneer user eerst M-hoogte kiest, en daarna switcht naar C-hoogte.
In encoway geen garantie dat VoorwandTypeVanMBinnenlade dan leeggemaakt wordt.
Dus HoogteGlas beter bepalen op basis van LadeHoogte en VoorwandVerhogingHoogte, en eventueel in combinatie met "HeeftInschuifelementVoor".

Opmerking 2 : HoogteGlas (onderscheid maken tussen FrontGlas en ZijwandFree) : Suggestie "HoogteFrontGlas"

Ook hier zou ik een conditie maken op basis van "VoorwandType" i.p.v. VoorwandTypeVanCBinnenlade.

"x" : MiddelpuntBreedte

Het lijkt te werken, maar ik vraag me af of het niet veiliger is met volgende conditie :
Ladehoogte C + "VoorwandType" i.p.v. VoorwandTypeVanCBinnenlade

Niet mogelijk.
Hoewel aanwezig en zichtbaar in de 'state' van het model is er geen waarde aan verbonden.
Dus niet mogelijk om deze mapping te gebruiken.

Ik heb nog gezocht naar andere eigenschappen die aangeven of het om een binnenlade gaat, maar deze lijkt mij het beste.

Ik heb het aangepast en getest, dus zet het op resolved.
Als ik terug denk aan de vorige review waar het ging om de dependencies juist te leggen zie ik niet goed hoe dit hier van toepassing is.
Ook een ander duidelijk voordeel zie ik niet meteen.

Zou graag leren waarom je dit hebt voorgesteld.

Voorwandtype bevat enkel vulling of reling.
We moeten specifiek weten dat het om laag glas gaat, dus hier is VoorwandTypeVanCBinnenlade correcter.

Deze property wordt nergens gebruikt, kan weg volgens mij

Dit is goed: een specifieke implementatie is (nog) niet nodig.

Niet zo veelzeggende foutomschrijving? 😋