This is a list of all comments for CR-2135. Review Summary: No summary ---------------------------------------- File: WSimpl/Vhisie4/Winkelkar/MultipersonalityPrijsWebservice/BerekenProductPrijzen.cls.xml Revision Comment by Tom Vermeulen on 08 September 2020, 10:10 defect http://subversion02:8060/cru/CR-2135#c3475 normaalgezien zetten we geen comments in de code. In principe wil dat zeggen dat het niet duidelijk is wat de code doet. Misschien is het beter om de verschillende blokken in de method op te splitsen in private methods en in je main method krijg je dan iets als: Do ..HaalMarkupOpVoorPersonality Do ..GeefMessageId Do ..MaakRequest ... enz enz Op die manier is de BerekenProductPrijzen-method korter en leesbaarder. Heeft ook het voordeel dat, bij een mogelijke latere uitbreiding de wijziging vrij gelokaliseerd zit in 1 (private) method die indien nodig eenvoudig in een helperklasse met zijn eigen tests afgezonderd kan worden. Revision Comment by Tom Vermeulen on 08 September 2020, 10:15 http://subversion02:8060/cru/CR-2135#c3476 weet ge al of het nog nodig is of niet? Indien niet nodig -> wegsmijten Revision Comment by Tom Vermeulen on 08 September 2020, 10:15 defect http://subversion02:8060/cru/CR-2135#c3477 #dim A As X = ##class(X).%New() vervangen door Set A = ##class(X).%New() Die stijl heeft de voorkeur bij constructie van nieuwe objecten, zeker als ze een lange naam hebben. Stel dat de %New() nog wat parameters nodig heeft, dan is de kans groot dat ze rechts van het scherm vallen, wat de code minder leesbaar maakt. Deze opmerking kunt ge hieronder nog een paar gebruiken (en misschien in andere klasses ook, zover ben ik nog niet) Revision Comment by Tom Vermeulen on 08 September 2020, 10:19 defect http://subversion02:8060/cru/CR-2135#c3478 Logger injecteren zodat ge in uw tests kunt verifiëren dat ie opgeroepen wordt als er iets is misgegaan. Bij nakijken van tests gerealiseerd dat dat misschien voor problemen zorgt als er gerethrowed wordt, maar ik weet het niet exact meer. ---------------------------------------- File: vhUnitTest/WSimpl/Vhisie4/Winkelkar/MultipersonalityPrijsWebservice/BerekenProductPrijzen/Test.cls.xml Revision Comment by Tom Vermeulen on 08 September 2020, 10:22 http://subversion02:8060/cru/CR-2135#c3481 algemene opmerking over onderstaande tests: Ze bevinden zich op het randje van 'te lang'. Misschien zou je kunnen overwegen om de setup van de tests (zeker het maken van de mockobjecten, die op het eerste zicht overal dezelfde zijn), in een BeforeOneTest te steken. Alle Verifieers kan je ook in een private method kwijt. Soit, dit is maar een voorstel hoor. De lijn is soms moeilijk te bepalen. Stukken code herhalen is op zich niet fout hé, zeker als de rest van de (test)method vrij kort is. Het voordeel van er een beetje generieke dingen uit te peuteren is dat de verschillen tussen de overgebleven code duidelijk kunnen opvallen. Revision Comment by Tom Vermeulen on 08 September 2020, 10:21 defect http://subversion02:8060/cru/CR-2135#c3480 typo: Service ipv Serive ---------------------------------------- File: APPS/EC/Winkelkar/WinkelkarService/impl/PrijsFactorBepalerFactory.cls.xml Revision Comment by Tom Vermeulen on 08 September 2020, 10:03 defect http://subversion02:8060/cru/CR-2135#c3474 als iets al overerft van TECH.RegisteredObject en je maakt het mockable, mag de TECH.RegisteredObject weg uit de parents-lijst, want TECH.Mockable is zelf een TECH.RegisteredObject ---------------------------------------- File: APPS/EC/Winkelkar/WinkelkarService/impl/PrijsBepaler/HaefeleDEPrijsFactorBepaler.cls.xml Revision Comment by Tom Vermeulen on 08 September 2020, 10:02 defect http://subversion02:8060/cru/CR-2135#c3472 aparte parameter maken voor Orgalux. Kan zijn dat dat nu toevallig gelijk is aan LBX. ---------------------------------------- File: APPS/EC/Winkelkar/WinkelkarService/impl/PrijsBepaler/SFSPrijsFactorBepaler.cls.xml Revision Comment by Tom Vermeulen on 08 September 2020, 10:03 defect http://subversion02:8060/cru/CR-2135#c3473 zelfde opmerking: aparte parameter gebruiken, zelfs als ie momenteel gelijk is. --- ID: CR-2135 http://subversion02:8060/cru/CR-2135 Title: [ICT-1753] [rvTVE, rvJWI] EC: MP: ORGALUX: MultipersonalityPrijsWebservice bouwen en aan ITR aanbieden... Statement of Objectives: State: Review Author: Sam Van Hoey Moderator: Sam Van Hoey Reviewers: (0 active, 2 completed*) Jo Willems (*) Tom Vermeulen (*)