Funktion bewerten/kritisieren

[Edgar Ehritt]

Hallo Michael,

<?PHP
function get_hero($target){
    $ch = curl_init();

im Folgenden böte sich curl_setopt_array() auch an.

curl_setopt($ch, CURLOPT_UNRESTRICTED_AUTH, true);

CURLOPT_UNRESTRICTED_AUTH ist hier völlig falsch. Du sendest keine Authentifizierung. Demzufolge ist CURLOPT_FOLLOWLOCATION Diskussionsgrundlage. Wenn der Server eine Weiterleitung vornimmt, wirst Du es nie erfahren. Dein Script folgt hier stumpf, ohne dass Du dies abfängst. Du solltest, wie weiter unten beschrieben, in der Script-Logik mindestens den HTTP-Status abfangen.

curl_setopt($ch, CURLOPT_URL, "http://www.battlefieldheroes.com/heroes/".urlencode($target));
    curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, '2');
    curl_setopt($ch, CURLOPT_LOW_SPEED_LIMIT, '5000');
    curl_setopt($ch, CURLOPT_LOW_SPEED_TIME, '4');

curl_setopt($ch, CURLOPT_USERAGENT, "Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5");

Dieses Script ist kein Mozilla, noch rendert es mit dem Gecko. Zum einen ist dies unnötiger Datentransfer, zum anderen könnte der Server aufgrund des alten Browsers Änderungen des Inhalts veranlassen. Was soll das also?

curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);

$content = curl_exec($ch);

Sagen wir, die Verbindung unterschreite für länger als vier Sekunden die gesetzte Geschwindigkeitsgrenze. Für diesen Fall trifft folgende Aussage zu: ($content===true)
Prüfe also, ob $content gefüllt ist. Allgemein ist HTTP nicht ganz ohne und Du solltest auf alles gefasst sein. Dein jetziger Scriptansatz ist nur auf eins gefasst - nähmlich alles geht gut. Davon ist nicht auszugehen. Server können überlastet sein, Admins können sich beim Umkonfigurieren vertippt haben, was zu temporären Ausfällen käme, deren Datenbank schmiert mal schnell ab - und, und, und.

Im simpelsten Fall reicht hier wirklich folgendes:

if(($content=curl_exec($ch))===false){  
        return(false);  
}

Dennoch gebe ich zu bedenken, lieber curl_setopt($ch,CURLOPT_HEADER,true); zu nutzen und die HTTP-Schicht ebenso zu analysieren. Ein kleines Beispiel, was den Status ermittels:

$ch=curl_init($host);  
  
curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);  
curl_setopt($ch, CURLOPT_HEADER,         true);  
curl_setopt($ch, CURLOPT_TIMEOUT,        5);  
  
if(($content=curl_exec($ch))===false){  
        curl_close($ch);  
        return(false);  
}  
list($header,     $content)     =explode("\r\n\r\n",$content,   2);  
list($statusline)               =explode("\r\n",    $header ,   2);  
list($httpversion,$status,$rest)=explode(' ',       $statusline,3);  
  
if($status!=200){  
        fehlerprotokoll($header,$target);  
        # besser aber Du nutzt switch() und machst Einzelfallbehandlungen  
}  
curl_close($ch);

curl_close($ch);
    ##############################################################
    $doc = new DOMDocument();
    @$doc->loadHTML($content);

Mit @ bekommst Du nicht mal mit, wenn es zu oben beschriebenen Verbindungsfehlern kommt und $content false ist.

$divs = $doc->getElementsByTagName('div');
    $heroes = array();
    for($i = 0; $i < $divs->length; ++$i){
        if($divs->item($i)->getAttribute('class') == 'cache-info'){
            $bold = $divs->item($i)->getElementsByTagName('b');
            for($a = 0; $a < $bold->length; ++$a){
                if($a == 2){
                    $next_update = $bold->item($a)->nodeValue;
                    $next_update = eregi_replace('h', '', $next_update);
                    $next_update = eregi_replace('m', '', $next_update);

Statt der beiden eregi_replace()-Ersetzungen reicht hier str_replace(array('h','m'),array(''),$next_update); völlig aus. Weiter unten wirst Du auf diesen Wert zurückgreifen, nach Deinem jetzigen Ansatz kann (if($a == 2)) die Variable _nicht_ initialisiert sein.

}
            }
        }
        if($divs->item($i)->getAttribute('id') == 'heroprofile'){

$heroes ist nicht initialisiert: $heroes=array();

$ul = $divs->item($i)->getElementsByTagName('ul');
            for($b = 0; $b < $ul->length; ++$b){
                if($ul->item($b)->getAttribute('class') == 'stats'){
                    $heroes['profile'] = array();

Werden mehrere Listen der Klasse 'stats' gefunden, werden bereits gefundene Werte schlichtweg überschrieben. In wieweit das aber Relevanz bekommt, weiß ich natürlich nicht.

$listitems = $ul->item($b)->getElementsByTagName('li');
                    for($a = 0; $a < $listitems->length; ++$a){
$arraykey = explode(' ', $listitems->item($a)->getAttribute('class'));
                        $keyvalue = explode('-', $listitems->item($a)->getAttribute('class'));
                        $heroes['profile'][$arraykey[0]] = $keyvalue[1];
                    }
                }
            }
            $h2 = $divs->item($i)->getElementsByTagName('h2');
            for($c = 0; $c == 0; ++$c){
                $heroes['profile']['name'] = $h2->item($c)->nodeValue;
            }
            $table = $divs->item($i)->getElementsByTagName('table');
            for($d = 0; $d < $table->length; ++$d){
                if($table->item($d)->getAttribute('class') == 'profileinfo'){
                    $heroes['stats'] = array();

Selber Vorbehalt.

$tr = $table->item($d)->getElementsByTagName('tr');
                    for($e = 0; $e < $tr->length; ++$e){
                        $th = $tr->item($e)->getElementsByTagName('th');
                        $td = $tr->item($e)->getElementsByTagName('td');
                        $heroes['stats'][$th->item(0)->nodeValue] = $td->item(0)->nodeValue;
                    }
                }
            }
        }
    }
    if(count($heroes) != 0){
        $heroes['lifetime'] = $next_update;

Was wenn $next_update nicht initialisiert wurde, weil kein <b> gefunden wurde?

return $heroes;
    }else{
        return false;
    }
}
?>

Gruß aus Berlin!
eddi