[PHP] Code optimieren

[Zweipunktnull]

Hallo!

Ich habe da ein Stück Code, was ich sehr suboptimal finde, weil es doch recht viele Wiederholungen enthält. Allerdings habe ich momentan einfach keine Idee, wie ich es besser machen könnte.

Es geht darum, dass unterschiede Seiten eingebunden werden sollen - abhängig von einer GET-Variable und der Tatsache, ob der Benutzer eingeloggt ist oder nicht. Für manche Seiten muss man eingeloggt sein, für andere nicht. Und wenn ein uneingeloggter Benutzer eine Seite aufruft, für die man eingeloggt sein muss, soll die Seite index angezeigt werden.

switch ($_GET['page'])
{
  case 'page1':
    if ($_SESSION['logged_in'])
    {
        include 'page1';
        break;
    }
  case 'page2':
    if ($_SESSION['logged_in'])
    {
        include 'page2';
        break;
    }
  case 'page3':
    if ($_SESSION['logged_in'])
    {
        include 'page3';
        break;
    }
  case 'page4':
    include 'page4';
    break;
  case 'page5':
    include 'page5';
    break;
  case 'page6':
    include 'page6';
    break;
  default:
    include 'index';
}

Das if ($_SESSION['logged_in']) wiederholt sich für meinen Geschmack zu oft. Wenn ich allerdings zuerst überprüfe, ob der Benutzer eingeloggt ist, dann muss ich die switch-Struktur teilweise doppelt schreiben. Ich sitze also irgendwie in einer Zwickmühle.

So etwas wie case 'page1' && $_SESSION['logged_in']: wäre optimal - leider geht das so in PHP nicht.

Hat also jemand eine andere Idee?

 

[Backslash]

Hm.. ungetestet aber sollte eig. gehen:

$page = $_GET['page'];
if ($page == "page1" || $page == "page2" || $page == "page3") {
 if ($_SESSION['logged_in']) {
 include($page); } }
else {
 include($page); }

mfg

 

[Zweipunktnull]

Ähm - ja - also das Beispiel war vereinfacht. In Wirklichkeit stimmt die GET-Variable nicht immer mit dem überein, was inkludiert wird. Also ich bräuchte schon so etwas wie eine switch-Anweisung, wo ich für jeden Fall einen bestimmten Code hinschreiben kann.

 

[Backslash]

Ja das konnt ich ja nicht wissen
Aber du kannst ja das switch an die stelle des includes schreiben, dürfte immernoch kürzer sein. Und wenn es dir zu viele Seiten sind, die login benötigen, um sie in das if() reinzuschreiben könntest du eine Datei machen in der diese gelistet sind (oder so eine Liste schreiben). Dann einfach mit strpos abgleichen.
Das wären so spontane Ideen von mir.
mfg

 

[Zweipunktnull]

Nichts für Ungut, aber die Idee fand ich auch nicht so toll... ich hab mich jetzt von der switch-anweisung verabschiedet und sie durch eine elseif-anweisung ersetzt. performancemäßig dürfte der unterschied wahrscheinlich schwindend gering sein.

 

[Kampfgnom]

Hast du keine Datenbank zur Verfügung?

Einfach eine Tabelle mit den Feldern id, include_file und login_required.
Dann übergibst du nur noch eine ID und holst dir den ganzen Rest aus der DB...

So wie das bisher gemacht ist, finde ich es mehr als suboptimal.

Oder natürlich du schreibst ein mehrdimensionales Array wenn dir eine Datenbank zu übertrieben erscheint. Der Code wird dadurch zwar eher länger als kürzer, aber du hast das Seitenmanagement (das Array) und den Code selber auseinandergehalten. Das macht es wesentlich einfacher zu warten.

 

[Zweipunktnull]

Das wird dann aber aufwändig, oder hab ich nur wieder suboptimalen Code produziert?

if (in_array($page, $pages))
{
    if ($pages[$page]['login_required'] == 1)
    {
        if ($_SESSION['logged_in'])
        {
            include $pages[$page]['include_file'];
        }
        else
        {
            include $pages['index']['include_file'];
        }
    }
    else
    {
        include $pages[$page]['include_file'];
    }
}
else
{
    include $pages['index']['include_file'];
}

 

[Backslash]

Ich habe es bei meiner Seite so gemacht, dass einfach beim zu schützenden Bereich jeder Seite ein kleines

if ($_SESSION['logged_in']){ (..code..) } else { echo('access denied'); }

steht. Ist vermutlich sogar die kürzeste Lösung.
mfg

 

[Zweipunktnull]

Ist vermutlich sogar die kürzeste Lösung.

Wenn ich dich Problem hätte.

Es geht hierbei allerdings nicht in erster Linie darum, geschütze Seiten zu realisieren, sondern eben darum, dass abhänig von einer GET-Variable unterschiedlicher Code ausgeführt wird.

Kampfgnoms Vorschlag gefällt mir schon gut, also der mit dem Array - eine Datenbank fände ich etwas übertrieben für das Problem (obwohl ich prinzipiell sowieso in der Applikation Datenbanktabellen benötige, aber die Array-Methode gefällt mir halt besser )
Nur kommt mir mein geposteter Code irgendwie schon wieder so suboptimal vor, vielleicht schaut
Kampfgnom ja nochmal vorbei.

EDIT:

Also die Abfrage, ob page überhaupt einen gültigen Wert hat, hab ich jetzt mal voran geschickt. so erspare ich mir eine verschaltelung. trotzdem bin ich irgendwie unzufrieden mit dem code

$page = (in_array($_GET['page'], $pages)) ? $_GET['page'] : 'index';



if ($pages[$page]['login_required'] == 1)
{
    if ($_SESSION['logged_in'])
    {
        include $pages[$page]['include_file'];
    {
    else
    }
        include $pages['index']['include_file'];
    }
}
else
{
    include $pages[$page]['include_file'];
}

 

[Kampfgnom]

if ($pages[$page]['login_required'] == 1 && !$_SESSION['logged_in'])
{
	include $pages['index']['include_file'];
}
else
{
	include $pages[$page]['include_file'];
}

Vielleicht so? Oh mann ich hab grad echt Angst dass ich was total verchecke und total peinlichen Code poste. Kommt mir ehrlich gesagt zu einfach vor...

Aber ehrlich gesagt kommt es mir auch ziemlich richtig vor ^^

 

[Zweipunktnull]

hm.. ich glaube.. du bist einfach genial - genial einfach

also ich bin in gedanken mal alle fälle durchgegangen und eigentlich müsste es so gehen.
also vielen dank, das ist echt besser als die switch- oder elseif-lösung.

 

[Backslash]

Fast meine erste Lösung, nur mit nem Array wos drinsteht
Aber sieht gut aus und die Seiten stehn nicht mehr direkt im Code.
mfg

 

[Kampfgnom]

Außerdem kannst du auch diesen Teil mit in die erste if-Abfrage mit reinnehmen:

(in_array($_GET['page'], $pages))

Also

if (   !in_array($_GET['page'], $pages) 
         ||   ( $pages[$page]['login_required'] == 1 && !$_SESSION['logged_in'] ) )
{
    include $pages['index']['include_file'];
}
else
{
    include $pages[$page]['include_file'];
}

Allerdings kannst du dann später nicht mehr andere Fehler-Seiten aufrufen für die verschiedenen Fehler. Deswegen würde ich es so machen:

if ( !in_array($_GET['page'], $pages) )
{
    //Fehler 404 (eigene Fehlerseite bauen)
    include $pages['index']['include_file'];
}
else if ( $pages[$page]['login_required'] == 1 && !$_SESSION['logged_in'] ) 
{
    //Fehler Not Logged in -> Zur Loginseite leiten oder Ähnliches
    include $pages['index']['include_file'];
}
else
{
    include $pages[$page]['include_file'];
}

EDIT: Was ich eigentlich sagen will: Es kommt nicht auf die Kürze oder die Geschwindigkeit an, sondern auf Erweiterbarkeit und Wartbarkeit. Lieber mehr Code als so ein asd?fgh:jkl-Mist

 

[1668mib]

Ich finde es ehrlich gesagt auch besser, wenn die geschützte Seite sich selbst schützt, durch eine Weiterleitung auf eine Zielseite in der dann der Code steht, welcher ausgeführt werden soll, wenn man nicht eingeloggt ist, halte ich für die langfristig elegantere Lösung.

 

[Zweipunktnull]

@ Kampfgnom
Auch nicht schlecht. thx
Um das ganze noch ein wenig zu erweitern: Es wäre nett, wenn ein paar seiten nur dann aufgerufen werden können, wenn man nicht eingeloggt ist. bspw. macht es wenig sinn, wenn man eingeloggt ist eine seite aufzurufen, auf dem man ein loginformular präsentiert bekommt. ich habe gerade überlegt wie das gehen soll. man könnte im array noch einen schlüssel logout_required einführen, aber das kommt mir - na, was wohl? - schon wieder so subsoptimal vor.

@ 1668mib
Hm... ich bin mir nicht sicher ob ich dich korrekt verstehe. Meinst du soetwas wie Backslash in Post #8 erwähnt hat?

 

[Backslash]

Was passiert außerdem, wenn man direkt auf eine der geschützten Seiten geht, wenns nicht bei denen im Code steht? Genau, sie werden einfach angezeigt...

/edit:
ok in dem fall nicht

 

[Zweipunktnull]

Das ist leider unmöglich. Man kann die einzelnen Module gar nicht einzeln aufrufen, weil sie an einem für den Internetbenutzer unzugänglichen Ort liegen. Entweder .htaccess-geschützt oder gleich außerhalb des Root-Verzeichnisses.

 

[Enigma]

Schreibs doch in ne Datenbank oder in ein Array und geh dann diesen array durch:

$pages = array(
  'page1' => array('requiresLogin' => true),
  'page2' => array('requiresLogin' => false)
  );

if(!isset($pages[$_GET['page']]) {
  // fehler: seite gibts net
}

if($pages[$_GET['page']]['requiresLogin'] == true && !$_SESSION['logged_in']) {
  // fehler: login benötigt
}

 

[Zweipunktnull]

Also das Problem, dass manche Seiten erfordern, dass der Benutzer ausgeloggt ist, habe ich nun folgendermaßen gelöst.
Danke nochmal an Kampfgnom für den Tipp mit dem Array (und allen anderen, die mitgedacht haben ).

// Array, welches eine Liste aller Seiten enthält
$pages = array(
    // Seiten, die nichts erfordern; reqcode = requirement code = 0
    'page1' => array('incpath' => 'page1', 'reqcode' => 0),
    'page2' => array('incpath' => 'page2', 'reqcode' => 0),

    // Seiten, die erfordern, dass der Benutzer nicht eingeloggt ist; reqcode = requirement code = 1
    'page3' => array('incpath' => 'page3', 'reqcode' => 1),
    'page4' => array('incpath' => 'page4', 'reqcode' => 1),

    // Seiten, die erfordern, dass der Benutzer eingeloggt ist; reqcode = requirement code = 2
    'page5' => array('incpath' => 'page5', 'reqcode' => 2),
    'page6' => array('incpath' => 'page6', 'reqcode' => 2)
);


// Seite existiert nicht
if (!array_key_exists($page, $pages))
{
    include $pages['page1']['incpath'];
}
// Seite erfordert, dass der Benutzer nicht eingeloggt ist; Benutzer ist aber eingeloggt
elseif ($pages[$page]['reqcode'] == 1 && $_SESSION['loggedin'])
{
    include $pages['page1']['incpath'];
}
// Seite erfordert, dass der Benutzer eingeloggt ist; Benutzer ist aber nicht eingeloggt
elseif ($pages[$page]['reqcode'] == 2 && !$_SESSION['loggedin'])
{
    include $pages['page1']['incpath'];
}
// Seite kann und darf inkludiert werden
else
{
    include $pages[$page]['incpath'];
}

 

[1668mib]

Ich würde es weder in eine Datenbank noch in einem Array festhalten, welche Seite welchen Status benötigt.

Diese Logik würde ich direkt in die Datei schreiben...
Grob wie es Backslash geschrieben hatte, allerdings noch etwas" komplexer". Also nicht nur ein "Access denied" sondern halt direkt ne HTTP-Weiterleitung auf eine "sinnvolle" Seite. Z.B. wenn man in auf eine Seite die einen Login benötigt zugreifen will, und nicht eingeloggt ist, eben auf eine Login-Seite weiterleiten. Wenn man eingeloggt ist, aber z.B. die "Passwort vergessen"-Funktion nutzen will, macht natürlich eine Weiterleitung keinen Sinn, in diesem Fall wäre eine Fehlerseite schon logisch.

Ach ja und noch etwas nebensächliches: Was soll denn "requirement code" heißen? Wenn dann vielleicht "Requires Code".... ;-)