Review

[megawubs]

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\HealthCareTeams en \HealthcareTeamsFaker\Provider\nl_NL\HealthCareTeams zijn 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 *Data classes 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\TestCase class kunnen extenden, zodat je wat helper methodes kan gebruiken die die class beschikbaar stelt. Bijvoorbeeld de getProviders methode.

Hopelijk heb je hier wat aan!

[tvdstel]

Geleerd

• Opfrisser autoloading en namespacing
• Hoe Github tags en releases werken
• Hoe je een composer package maakt (en deze toevoegd aan packagist)
• Verkenning Github workflows en het toepassen van phpunit testen
• Verdieping van kennis hoe FakerPHP werkt. Super leuk ook om te kijken naar de providers die andere mensen hebben gemaakt en hoe zij die structureren.
• Dat FakerPHP een fork is van de originele Faker welke abandoned is. Wat betekent dat we voorlopig nog geen afscheid hoeven nemen van FakerPHP! :partying_face:

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.

Structuur

FakerPHP 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 en_US locale gebruikt. De reden dat ik heb gekozen om de base class niet te gebruiken, is zodat ik een test kon maken die alle locale varianten in 1 keer kan testen op minimaal correct type output. Een ander voordeel hiervan is dat het de mogelijkheid biedt om taalspecifieke testen te gebruiken om locale specifieke logica te testen, bijvoorbeeld als iemand in de toekomst een locale toevoegd die op een ander manier werkt.

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 werking

Goeie 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 $faker->locationName. Dit vind ik minder wenselijk. Ik twijfel nog of ik hier een eigen oplossing voor wil/ga maken geinspireerd op de functionliteit van FakerPHP zelf. (Done)

Tests

Ik 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 en_US class iets kon teruggeven en heb daar best wel even mee gestoeid. Toen dat eenmaal stond ben ik die class gaan uitbreiden. Vooral bij het maken van $faker->team en $faker->location heb ik de testen ook veel gebruikt om te kijken of er terug werdt gegeven wat ik verwachtte. Later heb ik de testclass toegevoegd die alle locales tegelijk aftest.

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.)

Faker\Tests\TestCase zit helaas niet ingebakken bij de release build van FakerPHP.

[MarkKremer]

Echt tof wat je allemaal hebt geleerd en wat je hebt staan!

FakerPHP 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 en_US locale gebruikt. De reden dat ik heb gekozen om de base class niet te gebruiken, is zodat ik een test kon maken die alle locale varianten in 1 keer kan testen op minimaal correct type output.

Waarom is dit niet mogelijk met Faker's eigen classes?

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.

Heb je in deze overweging overwogen om de functies in een superclass (parent) te zetten en de data classes daarvan te laten extenden?

Waar ik voornamelijk tegenaan liep was autoloading en namespacing in combinatie met de testsettings van PHPStorm.

Is dat nu duidelijk?

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.

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 $i. i wordt vaak gebruikt in een loop om de huidige index bij te houden. Een snelle google search zei dat i voor index, iterator of int staat. Wat het meest waar is weet ik niet en er zit dus wat vrije interpretatie. Ik zie het zelf echt als de index. De code in de link gebruikt het om het aantal verschijningen bij te houden dat elementen voorkomen. Dan zou ik de variabele bijv. $count noemen.

De regel er onder

! str_contains($name, $part) ?: $i++;

vind ik moeilijk te lezen. Een if is iets langer maar makkelijker te lezen.