Review #7
Comments
Geleerd
Daarnaast vond ik het ook interressant om te verkennen welke benamingen er nu daadwerkelijk gebruikt worden bij de organisaties waar mee gewerkt wordt bij Sibi. Mijn doel was om de Nederlandse data die deze provider genereert daar zoveel mogelijk op aan te laten sluiten. StructuurFakerPHP gebruikt een generieke class en locale classes voor hun providers. In het geval dat er geen generieke class is voor een Provider wordt als default de Wat betreft de duplicatie, deze loopt ook enigszins hand in hand met mijn keuze om de data in aparte classes te zetten. De logica voor het samenstellen van de strings loopt niet uiteen per locale, slechts de data waar ze op gebaseert zijn. Mijn keuze om de data in aparte classes te zetten was om het overzichtelijk te houden, zeker omdat de data al een flink aantal regels (300-ish) was. Ik heb deze gereformat en alsnog toegevoegd aan de locale classes. Interne werkingGoeie tip! Ik heb het voor nu toegepast, al ben ik nog niet helemaal blij met de werking. Als ik bijvoorbeeld een locatie wil samenstellen uit een locatie naam en een locatie suffix, moeten er publieke functies voor naam en suffix. Deze zijn dan dus ook bruikbaar via bijvoorbeeld TestsIk ben gestart met het maken van tests en heb het overgrote deel TDD ontwikkeld. Waar ik voornamelijk tegenaan liep was autoloading en namespacing in combinatie met de testsettings van PHPStorm. Ik ben begonnen met een test om te kijken of de Verder had ik het idee om voor alle functies te testen of deze ook daadwerkelijk een samenstelling van de toegestane data bevatten, maar dit bleek al gauw ingewikkeld en tijdsintensief voor weinig meerwaarde. (Inmiddels is er een test toegevoegd die voor $faker->contractType() test of aanmaken van een naam bestaande uit meerdere delen werkt zoals verwacht.)
|
|
Echt tof wat je allemaal hebt geleerd en wat je hebt staan!
Waarom is dit niet mogelijk met Faker's eigen classes?
Heb je in deze overweging overwogen om de functies in een superclass (parent) te zetten en de data classes daarvan te laten extenden?
Is dat nu duidelijk?
Wat heb je hier al geprobeerd? In je code heb je een deel data en een deel functies. In dit geval zijn de variabelen public en zou je deze in je test kunnen aanpassen om af te dwingen welk resultaat er uit de functies komt. Daardoor kan je de functies beter testen. (als variabelen niet public zijn, zijn dummy models of mocks nog een optie. Soms structureer je code dan ietjes anders). Je krijgt dan bijv.: public function test_contract_type_returns_a_formatted_contract_type(): void
{
foreach ($this->folder as $folder) {
$faker = $this->setFaker($folder);
$provider = /* get the provider created in the setFaker function from somewhere */;
$provider::$contractTypeFormats = ['{{contractTypeName}} {{contractTypeSuffix}}'];
$provider::$contractTypeName = 'Awesome';
$provider::$contractTypeSuffix = 'Employee';
$contractType = $faker->contractType();
$this->assertEquals('Awesome Employee', $contractType);
}
}Ik heb dit uit de losse pols geschreven dus niet getest. Maar even voor het idee. :) Je gebruikt in deze test de variabele De regel er onder ! str_contains($name, $part) ?: $i++;vind ik moeilijk te lezen. Een |
Hey, zoals belooft bij deze een review van de code.
Als eerste wil ik zeggen dat ik het heel tof vind dat je deze library gemaakt hebt! 🎉 Ik ben wel heel benieuwd naar de dingen die je hierdoor geleerd hebt. Zou je die kunnen verwoorden?
Review
Structuur
Wat mij als eerste opvalt in de structuur is aardig wat duplicatie in de en_US en nl_NL namespaces. De classes
\HealthcareTeamsFaker\Provider\en_US\HealthCareTeamsen\HealthcareTeamsFaker\Provider\nl_NL\HealthCareTeamszijn vrijwel identiek. Wanneer ik een diff doe van de twee bestanden, zijn er maar een paar regels die afwijken. Het enige wat afwijkt is de aanroep naar de data classes.Vraag:
Wat zou je kunnen doen om de duplicatie hier te kunnen verminderen? Als tip zou ik eens kijken naar hoe Faker het intern zelf doet met hun Providers. Let vooral op dat er dus generieke classes zijn, en specifieke classes per taal implementatie. Wellicht kan je daar iets mee.
Ook zie ik dat Faker zelf de data voor verschillende talen in de class zelf zet en geen
*Dataclasses gebruikt. Kan je toelichten waarom je voor deze structuur gekozen hebt?Interne werking
Wanneer ik wat dieper inzoom ik een constructie om verschillende delen van bijvoorbeeld een locatie naam op te bouwen (hier). Faker heeft daar zelf ook al een ingebouwde oplossing voor. Ook daarvoor verwijs ik je graag naar de source code van Faker zelf
Tests
Super tof dat je tests hebt gemaakt! Hoe was dit? Hielp het bij het ontwikkelen? Waar liep je tegenaan bij het maken van tests? Ik ben echt heel benieuwd!
Wat mij opvalt in je tests is dat er een paar tests zijn die niet veelzeggend zijn. Bijvoorbeeld:
test_it_returns_valid_team. Deze test assert dat de uitkomst van de team() method een string is (wat al gedefineerd is in de return type), vervolgens wordt de uitkomst vergeleken met de getrimde versie van de uitkomst. We weten nu dat de team methode een string terug geeft en dat hij geen spaties aan het begin en einde bevat. Waarom mag een team geen spatie bevatten? Ik denk wanneer je meer richting de opzet van Faker zelf gaat, de testen een stuk minder omvangrijk worden, aangezien je dan meer voortborduurt op de interne werking van faker zelf.De tests die je hebt, zouden de
Faker\Tests\TestCaseclass kunnen extenden, zodat je wat helper methodes kan gebruiken die die class beschikbaar stelt. Bijvoorbeeld degetProvidersmethode.Hopelijk heb je hier wat aan!
The text was updated successfully, but these errors were encountered: