[PHP] Waar leg ik de verantwoordelijkheid?

[Morax]

Sorry voor de misschien wat algemene topictitel. Ik heb net een nieuw rechtensysteem geimplementeerd op mijn site, maar ik sta een beetje voor een dilemma.

Als voorbeeld een poll. In mijn rechtensysteem is een recht "vote" waarmee ik kan bepalen of je op polls mag stemmen. Nu dacht ik in mijn poll class gewoon te zetten:

PHP:

class poll{
  public function hasRight($action){
    //Hier word binnen het rechtensysteem gecheckt of je mag stemmen
  }
}

$poll = new poll(1);

if(!$poll->hasRight('vote')){
  error('U mag niet stemmen.');
}

Het probleem is alleen, dat ik natuurlijk ook moet checken of iemand all gestemd heeft, maar waar leg ik nu de verantwoordelijkheid om dat ook te controleren?

Optie 1:
Afvangen binnen de hasRight() functie.
Heeft als nadeel dat je veel uitzonderingen in je hasRight functie krijgt, en dat die functie dus in sommige gevallen erg groot en/of onleesbaar kan worden.

PHP:

class poll{
  public function hasRight($action){
    if($action == 'vote'){
      if($this->hasVoted()){
        return false;
    }

    //Hier word binnen het rechtensysteem gecheckt of je mag stemmen
  }

  public function hasVoted(){
    //Hier word gecontroleerd of de persoon al gestemd heeft
  }
}

$poll = new poll(1);

if(!$poll->hasRight('vote')){
  error('U mag niet stemmen.');
}

Optie 2: Aparte functies voor elke action.
Heeft als nadeel dat het rechtensysteem wat minder flexibel word, omdat je voor elk mogelijk recht een aparte functie moet maken.

PHP:

class poll{
  public function canVote(){
    if($this->hasVoted()){
      return false;
    }

    //Hier word binnen het rechtensysteem gecheckt of je mag stemmen
  }

  public function canEdit(){
    //Hier word binnen het rechtensysteem gecontroleerd of je polls mag wijzigen
  }

  public function canDelete(){
    //Hier word binnen het rechtensysteem gecontroleerd of je polls mag verwijderen
  }

  public function hasVoted(){
    //Hier word gecontroleerd of de persoon al gestemd heeft 
  }
}

$poll = new poll(1);

if(!$poll->canVote()){
  error('U mag niet stemmen.');
}

Optie 3: Buiten de classes laten.
Heeft als nadeel dat je veel dezelfde checks krijgt en een wijziging op meerder plaatsen moet aanpassen

PHP:

class poll{
  public function hasRight($action){
    //Hier word binnen het rechtensysteem gecheckt of je mag stemmen
  }

  public function hasVoted(){
    //Hier word gecontroleerd of de persoon al gestemd heeft
  }
}

$poll = new poll(1);

if(!$poll->hasRight('vote') || $poll->hasVoted()){
  error('U mag niet stemmen.');
}

Alledrie de opties gaan natuurlijk werken, maar wat is de meest nette/onderhoudbare optie? Of zou je het misschien helemaal anders doen? Ik ben benieuwd naar jullie input

[Verwijderd]

Zoiets misschien?

PHP:

<?php

    class poll {
    
        function canVote() {
            
            return (!$this->hasVoted() && $user->hasRight('poll_vote'))
            
        }
    
    }
    
    if ($poll->canVote()) {}
    
    if ($user->hasRight('poll_edit')) {}
    if ($user->hasRight('poll_delete')) {}

?>

[ Voor 16% gewijzigd door Verwijderd op 30-04-2007 22:04 ]

[Morax]

Verwijderd schreef op maandag 30 april 2007 @ 22:01:
Zoiets misschien?

PHP:

<?php

    class poll {
    
        function canVote() {
            
            return (!$this->hasVoted() && $user->hasRight('poll_vote'))
            
        }
    
    }
    
    if ($poll->canVote()) {
    }

?>

Jij gaat dus voor optie 2, heb je ook nog argumentatie?

[Sybr_E-N]

Waarom zou je de klasse Poll willen voorzien van rechtencheckfunctionaliteit? Ik zou dat er uit laten en overlaten aan een andere klasse, eventueel op een hoger nivo gaan regelen.

[Morax]

Sybr_E-N schreef op maandag 30 april 2007 @ 22:09:
Waarom zou je de klasse Poll willen voorzien van rechtencheckfunctionaliteit? Ik zou dat er uit laten en overlaten aan een andere klasse, eventueel op een hoger nivo gaan regelen.

Dat is hoe mijn systeem momenteel werkt. Als jij een andere suggestie hebt: Ik sta open Het topic heet niet voor niets "Waar leg ik de verantwoordelijkheid", in plaats van "Welke van de 3 opties moet ik nemen"

Ik weet niet of je ook een voorbeeld/argumentatie hebt, want van alleen "ik zou het door een andere klasse of op hoger niveau regelen" zie ik nu niet meteen de voordelen

[ibmos2warp]

Ik zou voor optie drie gaan, je hoeft voor die poll alleen te kijken of iemand mag en kan stemmen. Ik zou niet weten waarvoor je zou moeten kijken of iemand die poll mag aanpassen, of verwijderen, overigs zou ik dat ook gewoon in een stoppen. Naar mijn mening kan je de zaak op deze manier het beste overzien, je hoeft niet te gaan zoeken naar iets, want het staat op de plek waar je het zou verwachten (naar mijn id dan he )

[Verwijderd]

Het ligt er ook een beetje aan hoe de poll gebruikt gaat worden. Stel dat alle gebruikers alleen hun eigen poll mogen wijzigen (bv in een forum) dan zou je eigenlijk wel een poll::canEdit() functie moeten maken.

PHP:

class poll {
  function canEdit() {
   return (($this->owner_id == $user->id) && $user->hasRight('poll_edit'))
  }

}

En aangezien het aantal actions dat een poll heef (delete,edit,vote) toch beperkt is zou ik gewoon voor elke 'recht' een aparte functie maken. En daarin kan je weer wat geavanceerdere rechten verwerken met het rechtensysteem van elke user. Bv:

PHP:

class poll {
  function canEdit() {
      if ($user->hasRight('poll_edit_others')) return true;
      else return (($this->owner_id == $user->id) && $user->hasRight('poll_edit'))
  }

}

[Morax]

Verwijderd schreef op maandag 30 april 2007 @ 22:31:
Het ligt er ook een beetje aan hoe de poll gebruikt gaat worden. Stel dat alle gebruikers alleen hun eigen poll mogen wijzigen (bv in een forum) dan zou je eigenlijk wel een poll::canEdit() functie moeten maken.

PHP:

class poll {
  function canEdit() {
   return (($this->owner_id == $user->id) && $user->hasRight('poll_edit'))
  }

}

En aangezien het aantal actions dat een poll heef (delete,edit,vote) toch beperkt is zou ik gewoon voor elke 'recht' een aparte functie maken. En daarin kan je weer wat geavanceerdere rechten verwerken met het rechtensysteem van elke user. Bv:

PHP:

class poll {
  function canEdit() {
      if ($user->hasRight('poll_edit_others')) return true;
      else return (($this->owner_id == $user->id) && $user->hasRight('poll_edit'))
  }

}

De poll-class is even een voorbeeld class, mede omdat iedereen dat kent. Het punt is, de manier waarop ik het nu ga doen, moet ik natuurlijk wel door blijven trekken naar andere classes, anders word het een inconsistent zooitje. En momenteel heb ik al een class die 6 typen rechten kent

Maar bijvoorbeeld mijn Comment class, heeft inderdaad de situatie waarin een gebruiker zijn eigen berichten mag wijzigen, los van of het mag van het rechtensysteem, en dan krijg je inderdaad weer hetzelfde punt als hierboven.

[ Voor 8% gewijzigd door Morax op 30-04-2007 22:36 ]

[SchizoDuckie]

Ik zou dit dus iig een niveau hoger managen, want ga je al deze functies ook voor andere objecten implementeren? Dan ben je jezelf dus aant herhalen en kan je iets algemeners bedenken, een rightsmanager class bijvoorbeeld, die je dan in je poll class weer kan aanspreken (daar is dan weer niets mis mee)

[Verwijderd]

Sowieso hoort $user een argument te zijn die je meegeeft aan de hasRight, hssVoted, etc. methods. Een poll hoort niet bij een user, een user hoort niet bij een poll. Geen van beiden hoort dus een property te zijn van de ander, en dus moet je die dingen als argument meegeven.

[Verwijderd]

Ik zou voor optie 2 gaan.
en wat cheatah zegt is zeker juist, als je geen user meegeeft, hoe weet deze dan over wie het gaat?
of heb je een $_SESSION variable die je in de class gebruikt?

[Morax]

Verwijderd schreef op maandag 30 april 2007 @ 22:44:
Sowieso hoort $user een argument te zijn die je meegeeft aan de hasRight, hssVoted, etc. methods. Een poll hoort niet bij een user, een user hoort niet bij een poll. Geen van beiden hoort dus een property te zijn van de ander, en dus moet je die dingen als argument meegeven.

Uiteraard, maar ik heb alle code even gereduceerd tot de kern van het probleem: Waar leg ik de verantwoordelijkheid. Ik weet niet of je daar ook nog iets over te zeggen hebt?

Ik ben juist benieuwd hoe andere mensen dit op zouden lossen, inclusief argumenten, zodat ik er misschien ook nog wat van kan leren, of om te zien of iemand anders misschien nog een andere oplossing aandraagt waar ik nog niet aan had gedacht.

[Verwijderd]

Normaal gesproken vallen rechten ed onder security en is het aan te raden deze als geheel op zichzelf staand deel te zien (en vaak dus te integreren in een aparte class).

Argumenten:

- Je komt niet dit soort vragen tegen, alles te doen met security moet in het security systeem
- Herbruikbaarheid van code, bij security komt vaak veel extra code kijken ter beveiliging van het geheel. Deze code wil je in principe gebruiken bij alles wat te doen heeft met security.
- Beveiliging, je kan voor het security deel bijvoorbeeld een aparte database gebruiken welke encryptie toepast op verschillende lagen. Je hoeft dit dan maar op 1 plek te implementeren.

Daarnaast vind ik persoonlijk het iets overzichtelijker wanneer security zaken in 1 systeem staan, je kan dan vrij snel zien waar eventuele fouten zitten. Fouten in je systeem zijn 1 ding, maar fouten in het security deel wil je toch eigenlijk wel voor 100% uitsluiten.

Dus in principe optie 3, alleen doordat je alles integreert in een apart systeem/class heb je niet het probleem dat je veel dezelfde code hebt oid.

[ Voor 7% gewijzigd door Verwijderd op 30-04-2007 23:19 ]

[Verwijderd]

Ik zie eerlijk gezegt geen andere optie dan de rechten voor iedere class apart te behandelen. Het enigste wat je kan doen in het consistent houden door de naam (hasRight) van de functie die het afhandelt (van verschillende classes) hetzelfde te houden (zoals bij optie 3). En in die functie kan je natuurlijk gewoon gebruik maken van een hogere niveau ( -> $user->hasRight).

PHP:

<?php

class poll {
    function hasRight($action) {
        
        switch ($action) {
            case 'edit':
                if ($user->hasRight('poll_edit_others')) return true;
                        else return (($this->owner_id == $user->id) && $user->hasRight('poll_edit'))
                  case 'delete':
                  case 'vote':
        }
    }

}

$poll->hasRight('edit')
$comment->hasRight('edit'); 

?>

Of je kan het via een omweg doen:

PHP:

class user {
    
    function hasRight($action,$class) {
        
        return $class->hasRight($this,$action);
        
    }
    
}


$user->hasRight('edit',$poll);
$user->hasRight('edit',$comment);

... wat misschien mooier, maar ook een beetje omslachtig is.

[Verwijderd]

Wellicht heb je hier dan wat aan optie 4: Zorgen dat je security een aspect wordt van je class/functie. Hiervoor zijn proxies noodzakelijk, en tot mijn grote verbazing kan zelfs php dat: proxies, delegates en decorators

[Grijze Vos]

Verwijderd schreef op dinsdag 01 mei 2007 @ 09:56:
Wellicht heb je hier dan wat aan optie 4: Zorgen dat je security een aspect wordt van je class/functie. Hiervoor zijn proxies noodzakelijk, en tot mijn grote verbazing kan zelfs php dat: proxies, delegates en decorators

Ik vind dat gebruikte voorbeeld daar ("waarom niet gewoon inheritance") echt uber ranzig. Aspect Oriented Programming in bijv. Aspect J gaat dan ook net een tikkie anders. (Heb er ooit een paper over geschreven.) Ik zou niet een complex stuk software van die gasten willen zien hoor, volgens mij is dat compleet ononderhoudbaar. Dat terwijl juist een van de gedachten achter AOP is dat het eenvoudiger wordt, op 1 plek een Aspect neerzetten, en dat automagisch inhaken op je calls naar members of whatnot. En niet alsnog al je code uitbreiden telkens met van die ranzige code.

[Verwijderd]

Grijze Vos schreef op dinsdag 01 mei 2007 @ 10:38:
Ik vind dat gebruikte voorbeeld daar ("waarom niet gewoon inheritance") echt uber ranzig.

Geen overerving omdat je daarmee niet alle functies van een class in 1 functie kan overriden.

Grijze Vos schreef op dinsdag 01 mei 2007 @ 10:38:
Aspect Oriented Programming in bijv. Aspect J gaat dan ook net een tikkie anders. (Heb er ooit een paper over geschreven.)

De werking is anders maar het doel is hetzelfde (Ik heb er nog geen paper over geschreven.)

Grijze Vos schreef op dinsdag 01 mei 2007 @ 10:38:
Ik zou niet een complex stuk software van die gasten willen zien hoor, volgens mij is dat compleet ononderhoudbaar. Dat terwijl juist een van de gedachten achter AOP is dat het eenvoudiger wordt, op 1 plek een Aspect neerzetten, en dat automagisch inhaken op je calls naar members of whatnot.

De gedachte achter AOP is de verantwoordelijkheid op de juiste plaats hebben. Eenvoudig is een relatief begrip en heeft er echt helemaal niets mee te maken.

Grijze Vos schreef op dinsdag 01 mei 2007 @ 10:38:
En niet alsnog al je code uitbreiden telkens met van die ranzige code.

Wat is er mis met een (abstracte) base class die aop functionaliteit toevoegt?

[Grijze Vos]

Verwijderd schreef op dinsdag 01 mei 2007 @ 11:34:
De werking is anders maar het doel is hetzelfde (Ik heb er nog geen paper over geschreven.)

De werking van aan het einde van elke functie een logThis() aanroep is ook anders, maar heeft ook hetzelfde doel als een trace log in AOP. Toch zou ik dat dan maar niet AOP noemen...

De gedachte achter AOP is de verantwoordelijkheid op de juiste plaats hebben. Eenvoudig is een relatief begrip en heeft er echt helemaal niets mee te maken.

Eenvoudig, overzichtelijk, dat moet altijd je doel zijn als programmeur. Daar wordt het namelijk onderhoudbaar van.

Wat is er mis met een (abstracte) base class die aop functionaliteit toevoegt?

Het argument is 'PHP ondersteund geen multiple inheritance', dus bouwen we deze constructie. Ten eerste kun je met MI ook niet een functie uit twee baseclasses allebei tegelijkertijd overerven, er zal er altijd een gekozen worden als de namen overeenkomen, ten tweede krijg je van die fijne constructies als

PHP:

$obj = new BoldHelloWorld(
         new ItalicHelloWorld (
           new HelloWorld()
         )
);

Vind ik persoonlijk nou niet een stukje duidelijk te volgen code. Denk je dat een nieuw persoon in een oogopslag kan zien wat hier gedaan wordt?

Ten derde krijg je dan weer meteen problemen als je 'gewone' inheritance wilt toepassen, want je classes inheriten dan alweer van je abstracte baseclass.

[ Voor 5% gewijzigd door Grijze Vos op 01-05-2007 12:05 ]

[PrisonerOfPain]

Grijze Vos schreef op dinsdag 01 mei 2007 @ 12:04:
[...]

De werking van aan het einde van elke functie een logThis() aanroep is ook anders, maar heeft ook hetzelfde doel als een trace log in AOP. Toch zou ik dat dan maar niet AOP noemen...

Het uiteindelijke doel is het afhandelen van cross-cutting concerns. Of de specifieke implementatie daarvan AOP heet of niet is amper relevant te noemen in dit geval en de originele uitdrukking kan ook een gebrekkig vocabulaire van de originele poster geweest kunnen zijn. De daadwerkelijke implementatie is over het algemeen amper relevant te noemen, of dat zoals hier, door een Proxy class word gedaan – omdat dat nu eenmaal de meest eenvoudige en toegankelijke manier is om het in PHP te doen – of door een AOP taal, gaat denk ik voorbij aan de discussie.

Vind ik persoonlijk nou niet een stukje duidelijk te volgen code. Denk je dat een nieuw persoon in een oogopslag kan zien wat hier gedaan wordt?

Verklaar je hier een fundamenteel design pattern tot bad practice of zie ik je punt niet?

[Verwijderd]

Grijze Vos schreef op dinsdag 01 mei 2007 @ 12:04:
De werking van aan het einde van elke functie een logThis() aanroep is ook anders, maar heeft ook hetzelfde doel als een trace log in AOP. Toch zou ik dat dan maar niet AOP noemen...

Wat noem jij dan AOP?

Grijze Vos schreef op dinsdag 01 mei 2007 @ 12:04:
Eenvoudig, overzichtelijk, dat moet altijd je doel zijn als programmeur. Daar wordt het namelijk onderhoudbaar van.

Leuk generiek statement, maar dat zegt dus helemaal niets over AOP en het is al zeker niet het doel. Tevens is je statement fout: Eenvoudig en overzichtelijk zorgen helemaal niet voor onderhoudbare code.

Een php pagina die een lijstje uit de db oprakelt en toont is zeer eenvoudig en overzichtelijk. Immers, alle betrokken logica zit in 1 bestand. Maar onderhoudbaar is het niet, zeker niet wanneer de logica op meerdere pagina's wordt gekopieerd. (let op: Eenvoudig is relatief)

Grijze Vos schreef op dinsdag 01 mei 2007 @ 12:04:
Het argument is 'PHP ondersteund geen multiple inheritance', dus bouwen we deze constructie. Ten eerste kun je met MI ook niet een functie uit twee baseclasses allebei tegelijkertijd overerven, er zal er altijd een gekozen worden als de namen overeenkomen, ten tweede krijg je van die fijne constructies als

PHP:

$obj = new BoldHelloWorld(
         new ItalicHelloWorld (
           new HelloWorld()
         )
);

Ik denk niet dat je het volledig hebt begrepen. Je laat nu een fragment van een decorator pattern zien. Hetgeen met een soortgelijke contructie kan worden bereikt. Voor AOP zijn we voornamelijk geinteresseerd in proxies, die dus prima zijn te realiseren met de 'magic' functies.

Grijze Vos schreef op dinsdag 01 mei 2007 @ 12:04:
Ten derde krijg je dan weer meteen problemen als je 'gewone' inheritance wilt toepassen, want je classes inheriten dan alweer van je abstracte baseclass.

Dan kunnen we in de meeste gevallen nog compositie toepassen.

[Grijze Vos]

We gaan ondertussen al behoorlijk offtopic.

PrisonerOfPain schreef op dinsdag 01 mei 2007 @ 12:27:
[...]
Verklaar je hier een fundamenteel design pattern tot bad practice of zie ik je punt niet?

Gezien hetgeen de TS wil doen vind ik het hier zeker niet gepast nee. En aangezien het nogal hard je mogelijkheden mbt inheritance onderuit haalt vind ik dit zeker geen geweldige aanpak nee. Het singleton pattern wordt ook regelmatig misbruikt. Dat iets een goed pattern is wil niet zeggen dat elke toepassing ervan automatisch goed is.

Verwijderd schreef op dinsdag 01 mei 2007 @ 12:46:
Ik denk niet dat je het volledig hebt begrepen. Je laat nu een fragment van een decorator pattern zien. Hetgeen met een soortgelijke contructie kan worden bereikt. Voor AOP zijn we voornamelijk geinteresseerd in proxies, die dus prima zijn te realiseren met de 'magic' functies.

Laat ik dan zeggen dat ik je linkje niet echt vind aansluiten bij je verhaal. Laten we nu echter weer ontopic gaan, en de TS helpen. Als we willen discussieren over AOP kunnen we ook een nieuwe draad openen.

[ Voor 50% gewijzigd door Grijze Vos op 01-05-2007 12:52 ]

[Verwijderd]

Grijze Vos schreef op dinsdag 01 mei 2007 @ 12:46:
Laat ik dan zeggen dat ik je linkje niet echt vind aansluiten bij je verhaal. Laten we nu echter weer ontopic gaan, en de TS helpen. Als we willen discussieren over AOP kunnen we ook een nieuwe draad openen.

Heb jij, de aop guru, dan nog iets contructiefs te melden voor TS?

[Grijze Vos]

Verwijderd schreef op dinsdag 01 mei 2007 @ 13:25:
[...]
Heb jij, de aop guru, dan nog iets contructiefs te melden voor TS?

Ja, heb ik. (Ben geen AOP guru hoor. In dat paper heb ik samen met een medestudent alleen geconcludeerd dat AOP overgehyped werd door de AOP aanhangers, en dat 90% van de geweldige oplossingen op eenvoudigere manieren in gewoon OOP afgehandeld kunnen worden. Blijven wel een paar handige voorbeelden over hoor, zoals debug tracing. M'goed, weer offtopic dit.) Zat net nog op mijn werk, dus kan nu ik thuis ben er even wat langer dan 2 minuten naar kijken.

----

Je hebt rechten die gecontroleerd moeten worden, mag de user voten, editen, deleten, etc. Die breng je imo onder in een aparte class. Verdere dingen als heeft de user al gevote, die breng je onder in functies in de class zelf, als ze niet in een regel te vatten zijn, of als ze vaker dan een keer voorkomen.

Versimpeld:

PHP:

class poll
{

  public function vote($user)
  {
    if($security->hasRights($user, "pollVote") && !$this->hasVoted($user))
    {
      // handle vote
    }
  }
}

Ervanuitgaande dat $security een instance is van de class die de rechten afhandeld.

Lijkt me redelijk duidelijk, standaard access control zit netjes in een aparte class, en specifieke issues m.b.t. de poll worden in de eigen class afgehandeld.

[ Voor 37% gewijzigd door Grijze Vos op 01-05-2007 13:36 ]