[PHP] Eigen functies best practices.

[S.O.]

Ik heb een vraag m.b.t. best practices voor het schrijven van eigen functies in PHP.

Voor zover ik gelezen en begrepen heb, zou een eigen functie maar 1 ding / actie moeten doen, indien er meerdere dingen gebeuren, dan zou je deze eruit moeten halen en in een andere functie moeten stoppen.
Dit bevorderd het hergebruik van code.
Nu vraag ik me of bijv. controle / fout afhandeling binnen dezelfde functie zou kunnen blijven of dat dit ook uitgesplitst zou moeten worden.

Een voorbeeldje:
We willen een bestand verplaatsen van locatie.
We geven de functie de naam "moveFile".
Het is natuurlijk wenselijk om eerst te controleren of het bestand wel bestaat op de source locatie en ook de destination locatie te controleren of we daar geen bestand gaan overschrijven.
Zetten we de controles in de "moveFile" functie en maken daar 1 functie van, of maken we voor de controles ook weer functies die we dan aanroepen binnen de "moveFile" functie.

Hoe bepalen jullie wat wel en niet verder uitsplitst moet worden?
Ligt het aan de grote van het project of je eventueel met meerdere personen aan een project werkt ??


Grt. S.O.

[Jantje2000]

Het ligt er aan of je het in een andere functie zou willen hergebruiken.

Dus als jij bijvoorbeeld in een andere functie ook wil controleren of dat bestand al op de server zit, dan splits je in functies, anders niet.

Het gaat eigenlijk gewoon om de DRY-regel. Dat betekent Don't Repeat Yourself. Je mag dus geen code dubbel schrijven. Opdelen in functies helpt daarbij.

Het zoveel mogelijk opdelen in functies is echter geen doel op zich, hoewel je in principe ook weer het single responsibility principe wil aanhouden, waarbij iedere class/function/etc voor slechts 1 ding afhankelijk is.

Het ligt dus heel erg aan de interpretatie en voorkeur

[NMe]

Jantje2000 schreef op donderdag 15 oktober 2020 @ 12:42:
Het ligt er aan of je het in een andere functie zou willen hergebruiken.

Dus als jij bijvoorbeeld in een andere functie ook wil controleren of dat bestand al op de server zit, dan splits je in functies, anders niet.

Behalve dat de functies voor die checks al in elk zichzelf respecterend framework (inclusief PHP) zitten. Tenzij je dat wil gaan wrappen in een complete FileSystem-class hoef je dit specifieke voorbeeld sowieso niet in losse functies op te splitsen want het enige dat die nieuwe check-functie vervolgens doet is een file_exists-call wrappen... Of je zo'n wrapperfunctie aanroept of file_exists (of een eventuele wrapper die een framework als Symfony of Laravel biedt) maakt ook amper nog uit.

[acemoo]

Niet alleen DRY hoeft hier van toepassing te zijn.

Het kan ook gaan om leesbaarheid.
Als je check 20 regels code is, wil je dan specifiek weten hoe deze check gedaan word als je de moveFile functie leest of wil je gewoon lezen welke checks er gedaan worden?

Je kunt voor elke check een eigen functie maken zodat je in de moveFile functie ziet wat er gedaan word zonder dat je de daadwerkelijke hoe hoeft te snappen.

[RobIII]

NMe schreef op donderdag 15 oktober 2020 @ 12:46:
[...]

Behalve dat de functies voor die checks al in elk zichzelf respecterend framework (inclusief PHP) zitten. Tenzij je dat wil gaan wrappen in een complete FileSystem-class hoef je dit specifieke voorbeeld sowieso niet in losse functies op te splitsen want het enige dat die nieuwe check-functie vervolgens doet is een file_exists-call wrappen... Of je zo'n wrapperfunctie aanroept of file_exists (of een eventuele wrapper die een framework als Symfony of Laravel biedt) maakt ook amper nog uit.

Sowieso is zo'n check vrij nutteloos (race condition). Als de file nog bestaat op 't moment dat je de check doet en de split second voordat je de file opent wordt de file alsnog verwijderd krijg je alsnog een exception die je zonder die check nét zo goed zou krijgen. Open gewoon meteen die file (dus skip die check) en handel vervolgens de eventuele exception die je krijgt bij 't openen af die je anders net zo goed tóch al moest afhandelen.

[mcDavid]

Er zijn al een hoop nuttige tips gegeven. Wat verder belangrijk is is een beetje je gezond verstand/ervaring gebruiken. Er is namelijk nooit één absolute waarheid of beste manier om iets te doen.

Wat richtlijnen die je daarbij kunt gebruiken zijn bijvoorbeeld:

- probeer niet te diep te "nesten" (dus bijv. niet tig if-statements in elkaar)
- als je code wat lang wordt, bedenk dan of je het op kunt splitsen
- probeer code te schrijven die leest "als een verhaal".

Met dat laatste bedoel ik: Stel je doet verschillende checks om te kijken of een gebruiker wel de juiste rechten heeft om een bestand te mogen verplaatsen. Dan moet iemand die de code doorleest, die verschillende checks "ontcijferen" en beredeneren wat het nut daarvan zal zijn. Als je ze verplaatst naar een method "checkUserPermissions()", en die aanroept, dan weet de lezer al precies wat er gebeurt (of iig zou moeten gebeuren) zonder uberhaupt maar in de method gekeken te hebben.

Verder zijn er nadrukkelijk géén richtlijnen wat precies "te lang" is hoe vaak je mag nesten. Dat komt gewoon op je inzicht aan. Zorg dat je veel code reviewt van anderen en anderen al jouw code reviewen en leer van elkaar.

[Janoz]

mcDavid schreef op donderdag 15 oktober 2020 @ 17:59:
Verder zijn er nadrukkelijk géén richtlijnen wat precies "te lang" is hoe vaak je mag nesten. Dat komt gewoon op je inzicht aan. Zorg dat je veel code reviewt van anderen en anderen al jouw code reviewen en leer van elkaar.

tbh, die zijn er wel. Wat jij 'nesten' noemt heet eigenlijk cyclomatic complexity van een fucntie/methode. Nesten kan wat verwarrend zijn omdat je het dan ook zou kunnen hebben over hoe diep de calls gaan. Voor cyclomatic complexity zijn wel degelijk richtlijnen en er is ook software om dat automatisch te kunnen controleren. Zodra die bv boven de 7 begint te komen is het raadzaam om de code te refactoren.

[DJMaze]

RobIII schreef op donderdag 15 oktober 2020 @ 14:57:
Open gewoon meteen die file (dus skip die check) en handel vervolgens de eventuele exception die je krijgt bij 't openen af die je anders net zo goed tóch al moest afhandelen.

Ook al ben ik het met je eens, PHP is hier onhandig.
De php error log loopt vol

edit:
met warnings

en er zijn geen exceptions in https://php.net/rename
Je moet dus zelf een exception schrijven en de error_log setting uit zetten.

[NMe]

DJMaze schreef op donderdag 15 oktober 2020 @ 20:45:
[...]

Ook al ben ik het met je eens, PHP is hier onhandig.
De php error log loopt vol en er zijn geen exceptions in https://php.net/rename
Je moet dus zelf een exception schrijven en de error_log setting uit zetten.

Waar heb je het in hemelsnaam over? Native errors implementeren al tijden gewoon de Throwable-interface die je gewoon kan catchen. Misschien moet je eens niet makkelijke dingen overcompliceren.

[DJMaze]

NMe schreef op donderdag 15 oktober 2020 @ 22:15:
Waar heb je het in hemelsnaam over? Native errors implementeren al tijden gewoon de Throwable-interface die je gewoon kan catchen. Misschien moet je eens niet makkelijke dingen overcompliceren.

Mijn PHP 7.4.10

PHP:

<?php
ini_set('display_errors', 0);
ini_set('log_errors', 1);
try {
    echo rename('oldfakename','newfakename') ? "rename success\n" : "rename failed\n";
} catch (\Throwable $e) {
    print_r($e);
}

Draai dat maar eens...

[NMe]

DJMaze schreef op donderdag 15 oktober 2020 @ 23:29:
[...]


Mijn PHP 7.4.10

PHP:

<?php
ini_set('display_errors', 0);
ini_set('log_errors', 0);
try {
    echo rename('oldfakename','newfakename') ? "rename success\n" : "rename failed\n";
} catch (\Throwable $e) {
    print_r($e);
}

Draai dat maar eens...

Daar returnt rename volgens spec gewoon false.

[DJMaze]

@NMe inderdaad, dus waar is die "native" Throwable/Exception?

Als ik log_errors/display_errors aan zet zie je exact wat ik bedoel wat WEL gelogged wordt.
Leuk als je heel veel rename() doet, loopt de log lekker vol...

[MueR]

DJMaze schreef op donderdag 15 oktober 2020 @ 23:29:
[...]


Mijn PHP 7.4.10

PHP:

<?php
ini_set('display_errors', 0);
ini_set('log_errors', 1);
try {
    echo rename('oldfakename','newfakename') ? "rename success\n" : "rename failed\n";
} catch (\Throwable $e) {
    print_r($e);
}

Draai dat maar eens...

Volgens de docs returned rename een bool, dus je kunt daar gewoon een if (!rename(...)) doen om een exception te gooien...

[MueR]

DJMaze schreef op donderdag 15 oktober 2020 @ 23:37:
@NMe inderdaad, dus waar is die "native" Throwable/Exception?

Als ik log_errors/display_errors aan zet zie je exact met wat ik bedoel wat WEL gelogged wordt.
Leuk als je heel veel rename() doet, loopt het lekker vol...

Zucht. De docs zeggen wat rename doet. Een ERROR geeft een error. een bool return niet.

[DJMaze]

Zucht.

PHP:

<?php
error_reporting(E_WARNING);
ini_set('display_errors', 1);
ini_set('log_errors', 1);
for ($i = 0; $i < 1000; ++$i) {
    rename("oldfakename{$i}", "newfakename{$i}");
}

En check je error logs nog eens

Dan krijg je dingen als

code:

PHP Warning:  rename(oldfakename999,newfakename999): No such file or directory in /rename.php on line 6
PHP Stack trace:
PHP   1. {main}() /rename.php:0
PHP   2. rename() /rename.php:6

Warning: rename(oldfakename999,newfakename999): No such file or directory in /rename.php on line 6

Call Stack:
    0.0002     361144   1. {main}() /rename.php:0
    0.0515     361744   2. rename() /rename.php:6

[ Voor 49% gewijzigd door DJMaze op 15-10-2020 23:43 ]

[Freeaqingme]

@DJMaze wat is je punt dan?

In dingen die exceptions gooien heb ik ook wel eens iets als:

code:

try {
    doStuff();
} catch(\Exception $e) {
    // ignore
}

Dan is een functie die geen exceptie gooit weer makkelijker als het niet uitmaakt of het slaagt of niet.

Wat dat betreft vind ik dat Go een mooie tussenweg biedt. Daarbij worden de errors altijd geretourneerd, en moet je er expliciet voor kiezen om 'm te discarden.

Aan PHP is het enige wat ik echt jammer vindt is dat het soms op de ene manier gebeurt, en soms op de andere manier. Dat wordt natuurlijk wel steeds meer rechtgetrokken, maar dat zijn dan toch helaas was historische artifacts.

[MueR]

DJMaze schreef op donderdag 15 oktober 2020 @ 23:41:
Zucht.
bullshit code
En check je error logs nog eens

Dan krijg je dingen als
[code]PHP Warning:

Warning. Warning !== Error.

Warnings zijn die dingen die tegen een developer zeggen: FIX YOUR SHIT, YOU LITTLE SHIT.

Errors zijn runtime errors.

[ Voor 18% gewijzigd door MueR op 15-10-2020 23:51 ]

[DJMaze]

Freeaqingme schreef op donderdag 15 oktober 2020 @ 23:45:
@DJMaze wat is je punt dan?

rename() gooit geen exception, dus dat er wordt geopperd om de checks achterwege te laten kan potentieel veel in je error log gooien.
Dat die ene race condition wel in de log belandt is minder erg.
En als je de error log echt schoon wil houden gebruik je je eigen error handler of zet je E_WARNING uit en gooi je een exception met de last error.

[Gomez12]

MueR schreef op donderdag 15 oktober 2020 @ 23:50:
[...]


Warning. Warning !== Error.

Warnings zijn die dingen die tegen een developer zeggen: FIX YOUR SHIT, YOU LITTLE SHIT.

Errors zijn runtime errors.

Dat is toch ook exact DJMaze zijn punt?
De discussie begon met :

DJMaze schreef op donderdag 15 oktober 2020 @ 20:45:
[...]

Ook al ben ik het met je eens, PHP is hier onhandig.
De php error log loopt vol en er zijn geen exceptions in https://php.net/rename
Je moet dus zelf een exception schrijven en de error_log setting uit zetten.

En dat klopt gewoon, want in wezen zegt php de hele tijd tegen RobIII's werkwijze : FIX YOUR SHIT, YOU LITTLE SHIT.

Alleen een aantal mensen overreageert hier op het woordje error in error log etc.

[Voutloos]

Het is zelfs een valide en veel voorkomende opzet om een error handler te registreren die dus alle errors vervangt door Exceptions. Bij sommige frameworks ben je er misschien niet eens bewust van.

Hier zit ook wat legacy, dus het hele error vs Exception onderscheid is ook gewoon niet perfect. Het idee om zoveel mogelijk op rename() te vertrouwen is wel een juiste overigens. Meer atomair en minder system calls (inc netwerk en/of trage disks!).

[ Voor 39% gewijzigd door Voutloos op 16-10-2020 08:59 ]

[Hopscotch]

Afgezien van dit specifieke voorbeeld en hoe je error handling doet, denk ik dat Robert Martin het het best uitlegt. Het gaat om 1 niveau van abstractie. Als we even aannemen dat je die checks daadwerkelijk zelf moet doen en niet in de taal of in een api zit ingebouwd. Wat moet je dan hoog over doen om een bestand te verplaatsen?

- check of het bestand bestaat
- check of de destination valid is
- daadwerkelijk verplaatsen

In dat geval zou je van elke stap een eigen functie maken met een duidelijke naam.

[Gomez12]

Hopscotch schreef op vrijdag 16 oktober 2020 @ 17:26:
Afgezien van dit specifieke voorbeeld en hoe je error handling doet, denk ik dat Robert Martin het het best uitlegt. Het gaat om 1 niveau van abstractie. Als we even aannemen dat je die checks daadwerkelijk zelf moet doen en niet in de taal of in een api zit ingebouwd. Wat moet je dan hoog over doen om een bestand te verplaatsen?

- check of het bestand bestaat
- check of de destination valid is
- daadwerkelijk verplaatsen

In dat geval zou je van elke stap een eigen functie maken met een duidelijke naam.

Tja, je moet het helemaal zelf weten hoor, maar ik verklaar je voor compleet gek als je op deze onnadenkende wijze bezig gaat.

Check of het bestand bestaat is in elke taal gewoon 1 functie-call, oftewel je creeert een nieuwe functie met daarin 1 functiecall. Gefeliciteerd, je hebt nu een alias gemaakt en je gaat in de knoei komen qua Naming etc want de meest logische naam voor deze functie is al in gebruik door de functie die je wil aliassen...

En hetzelfde geld voor of de destination valid is.

Of je moet echt wat toe te voegen hebben aan die functies en daarom zou je het eruit kunnen trekken, of je kan gewoon 1 checkfunctie maken waarin je alletwee de functie-aanroepen knalt.

Maar Uncle Bob bedoelt het absoluut niet zoals jij het nu zegt..

[Hopscotch]

Gomez12 schreef op vrijdag 16 oktober 2020 @ 17:39:
[...]

Tja, je moet het helemaal zelf weten hoor, maar ik verklaar je voor compleet gek als je op deze onnadenkende wijze bezig gaat.

Check of het bestand bestaat is in elke taal gewoon 1 functie-call, oftewel je creeert een nieuwe functie met daarin 1 functiecall. Gefeliciteerd, je hebt nu een alias gemaakt en je gaat in de knoei komen qua Naming etc want de meest logische naam voor deze functie is al in gebruik door de functie die je wil aliassen...

En hetzelfde geld voor of de destination valid is.

Of je moet echt wat toe te voegen hebben aan die functies en daarom zou je het eruit kunnen trekken, of je kan gewoon 1 checkfunctie maken waarin je alletwee de functie-aanroepen knalt.

Maar Uncle Bob bedoelt het absoluut niet zoals jij het nu zegt..

Als de functie al bestaat ga je hem natuurlijk niet zelf (nog een keer) maken, dan kan je een bestaande functie hergebruiken, of je hem nu zelf ooit gemaakt hebt of dat ie in de taal zit. Dit voorbeeld is wat dat betreft ook een beetje onhandig omdat het zoals je zegt altijd al in de taal zal zitten.