[php] Script duurt te lang.. [20 sec]

[w00d]

Met de onderstaande code lees ik een txt bestand uit en maak aan de hand daarvan een smoelenboek. Werkt opzicht prima, maar om een rare reden duurt het gewoon erg lang ( 20seconden ) voor dat de pagina geladen is.

Heeft iemand een ideetje waar ik de mist in ga dat het zo lang duurd? Want veel bijzonders is het niet: Ik lees een txt bestand uit, filter deze, maak wat kolommen en vul deze. Lijkt me toch niet erg veel werkt. Daarbij maakt het trouwen niet uit hoeveel werknemers er in het txt bestand staan, 5 of 100 is geen verschil...

PHP:

<!-- Display employees-->
            <?php
            $myFile = "data/employees.txt";
            $fh = fopen($myFile, 'r');
            $data1 = fread($fh, filesize($myFile));
            fclose($fh);
            $allewerknemers = count_chars($data1);  
            $werknemer=explode("#",$data1);//split hier de werknemers met scheidingsteken (#)

            $start=0;         
            $eind=$allewerknemers[ord("#")];       //tellen maximale werknemers
            
            $maxcols =2; // max 2 kolommen
            $tel = 0;
            
            //Welke bv wordt opgevraagd en de waarde meegegeven aan $bv
            if($_GET['bv'] == "Groep"){
            $bv = "Thunnissen Groep bv ";
            }elseif($_GET['bv'] == "Bouw"){
            $bv = "Thunnissen Bouw bv";
            }elseif($_GET['bv'] == "BouwB"){
            $bv = "Thunnissen Bouw Boskoop";
            }elseif($_GET['bv'] == "Ontw"){
            $bv = "Thunnissen Ontwikkeling";
            }elseif($_GET['bv'] == "OntwN"){
            $bv = "Thunnissen Ontwikkeling Noord";
            }elseif($_GET['bv'] == "Mat"){
            $bv = "Thunnissen Materieeldienst";
            }else{
            $bv = "Thunnissen Groep";}
            
            
            echo "<table class=\"employee\";>";
            
            for ($i = 0; $i < $num_rows; $i++);
            if ($tel ==0) echo "<tr>";
                    
            while($start<$eind)   //zolang start kleiner is dan eind werknemers toevoegen
                {                   
                    $gegevens=explode("|",$werknemer[$start]); //split hier de gegevens van de                                                                                     werknemers met scheidingstekens (|)\
                    
                    if ($gegevens[0] == $bv)    //laten alleen de medewerkers zien die bij de gekozen
                                                //bv horen
                    echo "<td class=\"employeecard\";><img src=\"images/employees/".$gegevens[1].".jpg\" /><br />Naam:&nbsp;".$gegevens[0]."<br />Functie:&nbsp;".$gegevens[2]."<br />Tel:&nbsp;".$gegevens[3]."</td>";
                    echo "<td width=\"100px\"></td>";
                    $start++; 
                    $tel++;  //teller voor kolommen verhogen
                     
                    if ($tel == $maxcols){ echo "</tr><tr></tr>"; $tel=0;} //als max aantal items per kolom bereikt is, nieuwe regel invoegen
                    
                }
            ?>
        <!-- End  -->

[RAJH]

Je kan met microtime een aantal delen van deze code gaan profilen om te zien wat er precies zo lang duurt.

[tha_crazy]

Wat het mij lijkt te zijn is dat het te lang duurt om de bestanden op te halen.
Want hij moet het bestand natuurlijk ook opnenen.
Hierover ben ik alleen niet 100% zeker.

[Pete]

PHP:

 for ($i = 0; $i < $num_rows; $i++);

Waar is $num_rows gedefinieerd? En wat doet dit statement? (niks volgens mij).

Zet ook even je errorniveau op E_ALL. Dan krijg je zulke dingen vanzelf te zien.


Als het dan nog niet opgelost is zou ik inderdaad op verschillende plekken in je script een microtime printen zodat je later kan zien wat het langst duurde.

[eek]

PHP:

 for ($i = 0; $i < $num_rows; $i++);

Waar dient dit voor?

bah... net te laat

[ Voor 10% gewijzigd door eek op 13-04-2007 09:11 ]

[P.O. Box]

je doet een while-lus in een for-lus... dat lijkt me hier redelijk overbodig...
en waarom doe je count_chars, als je ook gewoon count($werknemer) kunt gebruiken?

ook al te laat

[ Voor 6% gewijzigd door P.O. Box op 13-04-2007 09:17 ]

[w00d]

eek schreef op vrijdag 13 april 2007 @ 09:11:

PHP:

 for ($i = 0; $i < $num_rows; $i++);

Waar dient dit voor?

bah... net te laat

Zal ik is heel eerlijk zijn > Geen idee, maar zonder die regel is hij en gewoon snel en hij werkt ook nog. Dus thnxs dat was hem.

Ik heb dat stuk code gevonden om de tabellen te maken, dacht dat die regel daar voor nodig was, maar blijkbaar niet.

[.oisyn]

w00d schreef op vrijdag 13 april 2007 @ 09:41:
[...]


Zal ik is heel eerlijk zijn > Geen idee, maar zonder die regel is hij en gewoon snel en hij werkt ook nog. Dus thnxs dat was hem.

Fuck zeg, ga je eens snel inlezen in het programmeren. Als je al niet eens weet wat een for-lus doet, klakkeloos code overneemt, en van ons vervolgens gaat verwachten dat we je problemen verhelpen...

[w00d]

.oisyn schreef op vrijdag 13 april 2007 @ 13:31:
[...]


Fuck zeg, ga je eens snel inlezen in het programmeren. Als je al niet eens weet wat een for-lus doet, klakkeloos code overneemt, en van ons vervolgens gaat verwachten dat we je problemen verhelpen...

Daar heb je gelijk in, maar ik neem nou niet echt klakkeloos code over. 90% van wat er staat begrijp ik en weet ik van wat het doet, maar je kan helaas niet alles weten, ik ben het aan het leren, gebruik daarvoor veel w3school en het php manual om te kijken wat functies doen en hoe ik die moet gebruiken en soms kijk ik hoe andere bepaalde dingen gedaan hebben.

[Wim Leers]

@TS: je kan ook nog wat doen aan je indentering (want die ziet er niet uit), en ipv. "if { ... } elseif { ...} elseif .......", kun je ook gewoon een switch() doen (Nog korter en duidelijker kan ook, maar dat is toch al een hele verbetering.)

EDIT: net effe je site bezocht, en daar gaat wat mis wanneer je 'm bezoek met Safari... just FYI

[ Voor 17% gewijzigd door Wim Leers op 14-04-2007 13:03 ]

[HuHu]

w00d schreef op vrijdag 13 april 2007 @ 22:11:
[...]


Daar heb je gelijk in, maar ik neem nou niet echt klakkeloos code over. 90% van wat er staat begrijp ik en weet ik van wat het doet, maar je kan helaas niet alles weten, ik ben het aan het leren, gebruik daarvoor veel w3school en het php manual om te kijken wat functies doen en hoe ik die moet gebruiken en soms kijk ik hoe andere bepaalde dingen gedaan hebben.

Dat is een goede manier, maar zoals je het nu doet is het bagger. Bekijk de HTML output van je script eens goed, die zit waarschijnlijk erg brak in elkaar.

Maak eerst eens met de hand een tabel in HTML met daarin een aantal gegevens die je wilt hebben. Zet die vervolgens om naar een PHP for-lus en laat op de juiste plaats dingen invullen.

Want hoe je het nu doet gaat 't flink mis.

[robbert]

w00d schreef op vrijdag 13 april 2007 @ 22:11:
[...]


Daar heb je gelijk in, maar ik neem nou niet echt klakkeloos code over. 90% van wat er staat begrijp ik en weet ik van wat het doet, maar je kan helaas niet alles weten, ik ben het aan het leren, gebruik daarvoor veel w3school en het php manual om te kijken wat functies doen en hoe ik die moet gebruiken en soms kijk ik hoe andere bepaalde dingen gedaan hebben.

Niet om heel lullig te doen, maar constructies als if else, for, while, etc zijn toch wel de meest essentiële dingen in het programmeren. Ik zou toch eens beginnen met leren te begrijpen wat die dingen doen voordat je je ergens anders in gaat verdiepen.

[GlowMouse]

Door de punt-komma achter de for-loop is hij nog waardelozer dan hij al was: dan wordt namelijk nergens doorheen geloopt.

Daarnaast is het bij PHP zo dat je beter ingebouwde functies kunt gebruiken dan zelf constructies bedenken, omdat het iets uitmaakt qua snelheid en vaak duidelijker is.

PHP:

            $fh = fopen($myFile, 'r');
            $data1 = fread($fh, filesize($myFile));
            fclose($fh);

kun je daarom beter vervangen door:

PHP:

$data1 = file_get_contents($myFile);

Je while($start<$eind) ziet er logischer uit met een for-loop, omdat je vantevoren al precies weet hoevaak hij doorlopen wordt.

Je table wordt nergens afgesloten, en de tabelregels worden wel gesloten maar voor de volgende regel niet meer geopend. Naar de html-output moet je dus ook nog eens kijken.

_{Daarnaast kan dit stukje:
[php]
$allewerknemers = count_chars($data1);
$eind=$allewerknemers[ord("#")];
[/php]
beter vervangen worden door:
[php]
$eind = substr_count ($data1, '#')
[/php]
Je grote if-elseif-brei ziet er duidelijker uit met een switch-statement}

[ Voor 26% gewijzigd door GlowMouse op 14-04-2007 16:49 . Reden: deel was al gemeld ]

[FragFrog]

GlowMouse schreef op zaterdag 14 april 2007 @ 16:47:
Daarnaast is het bij PHP zo dat je beter ingebouwde functies kunt gebruiken dan zelf constructies bedenken, omdat het iets uitmaakt qua snelheid en vaak duidelijker is.

PHP:

            $fh = fopen($myFile, 'r');
            $data1 = fread($fh, filesize($myFile));
            fclose($fh);

kun je daarom beter vervangen door:

PHP:

$data1 = file_get_contents($myFile);

Waar lees jij dat'ie PHP 4.3 of hoger heeft? Zijn nog genoeg hosts die PHP 4.1 gebruiken en dan moet je wel degelijk fopen, fread en fclose toepassen.

Mocht'ie wel de beschikking hebben over PHP 4.3+ dan vermoed ik dat file() trouwens handiger is aangezien'ie zo te zien een CSV achtig iets probeert in te lezen, maar enfin, dat vereist ook weer dat'ie z'n halve script gaat herschrijven wat vast niet zo'n goed idee is..

[w00d]

Ik heb al jullie opmerking en tips doorgelezen en het script inmiddels aangepast.

@robbert: Ik weet wat ze doen, wist alleen niet precies wat die regel daar deed, vandaar.

@GlowMouse: Bedank voor de tips, de switch statement is inderdaad veel overzichtelijker. En ook het stukje van het tellen van het aantal werknemers is netter zo.
Ik gebruikte de if else statement ook in me menu's, tja wat je niet weet kan je ook niet toepassen, maar ik heb die nu ook meteen maar vervangen.

De table etc wordt buiten de php code afgesloten.De volgende regel wordt indien nodig wel geopend,

PHP:

if ($tel ==0) echo "<tr>";

Waar ik de regels afsluit en een lege tussen voeg zet ik ook de teller weer op 0 waarna er dus een nieuwe regel wordt geopend.

PHP:

<!-- Display employees-->
            <?php
            $myFile = "data/employees.txt";
            $fh = fopen($myFile, 'r');
            $data1 = fread($fh, filesize($myFile));
            fclose($fh);
            
            $werknemer=explode("#",$data1);//split hier de werknemers met scheidingsteken (#)
            $eind = substr_count ($data1, '#');    
            
            $maxcols =2; // max 2 kolommen
            $tel = 0;
            
            //Welke bv wordt opgevraagd en de waarde meegegeven aan $bv
            
            switch ($_GET['bv']) {
                case "Groep":
                $bv = "Thunnissen Groep bv ";
                break;
                case "Bouw":
                $bv = "Thunnissen Bouw bv ";
                break;
                case "BouwB":
                $bv = "Thunnissen Bouw Boskoop bv ";
                break;
                case "Ontw":
                $bv = "Thunnissen Bouw Boskoop bv ";
                break;
                case "OntwNoord":
                $bv = "Thunnissen Ontwikkeling Noord bv ";
                break;
                case "Mat":
                $bv = "Thunnissen Bouw bv Materieeldienst ";
                break;
                default:
                $bv = "Thunnissen Groep bv ";
            }   
            
        
            echo "<table class=\"employee\";>";
            
            
            if ($tel ==0) echo "<tr>";
            
            for( $start = 0; $start<$eind; $start++){
                    
                    $gegevens=explode("|",$werknemer[$start]); //splits hier de gegevens van de                                                                                        werknemers met scheidingstekens (|)
                                                
                        //Open images directory
                        $dir = opendir("images\employees");
                        //List files in images directory
                    
                        $foto = "geenfoto.jpg";
                    
                        while (($file = readdir($dir)) !== false)
                            {
                            if ($file == "".$gegevens[1].".jpg") $foto = $file;
                            }
                        closedir($dir);
                        
                        if ($gegevens[0] == $bv){
                        
                        echo "<td class=\"employeecard\";><img src=\"images/employees/".$foto."\" />
                        <br />Naam:&nbsp;".$gegevens[1]."<br />Functie:&nbsp;".$gegevens[2]."<br />     
                        Tel:&nbsp;".$gegevens[3]."</td>";
                        echo "<td width=\"100px\"></td>";
                        }
                    
                        $tel++;  //teller voor kolommen verhogen
                     
                     
                    if ($tel == $maxcols){ echo "</tr><tr></tr>"; $tel=0;} //als max aantal items per kolom bereikt is, nieuwe regel invoegen
                    
                }
            ?>
        <!-- End  -->

[P.O. Box]

kijk ook nog even naar: http://www.php.net/manual/nl/function.file-exists.php voordat je een hele directory gaat door-whilen...

[w00d]

hmm idd, dat kan ik beter even anders doen. Zal et vanaaf is ff aanpassen. Thnxs voor de tips

[xces]

ennuh, er is natuurlijk ook de magische "glob" functie

[GlowMouse]

FragFrog schreef op zondag 15 april 2007 @ 04:13:
[...]
Mocht'ie wel de beschikking hebben over PHP 4.3+ dan vermoed ik dat file() trouwens handiger is aangezien'ie zo te zien een CSV achtig iets probeert in te lezen, maar enfin, dat vereist ook weer dat'ie z'n halve script gaat herschrijven wat vast niet zo'n goed idee is..

CSV vereist niet dat iedere regel met een nieuw record begint. Een field mag best een newline bevatten.

w00d schreef op zondag 15 april 2007 @ 21:03:
De table etc wordt buiten de php code afgesloten.De volgende regel wordt indien nodig wel geopend,

PHP:

if ($tel ==0) echo "<tr>";

Waar ik de regels afsluit en een lege tussen voeg zet ik ook de teller weer op 0 waarna er dus een nieuwe regel wordt geopend.

Die regel staat boven je for-loop. Aangezien $tel daar 0 is, kun je die regel gewoon vervangen door echo '<tr>';. Waar het om gaat is dat binnenin je loop de regel na sluiten de tr ook weer geopend wordt. Maar wat je daar doet is sluiten-openen-sluiten ipv sluiten-openen. Daarnaast moet na afloop van je loop de table weer netjes afgesloten worden (zowel de tabel-rij als de tabel zelf).

Volgend punt: slechts als $gegevens[0] == $bv voeg je een nieuw element toe aan je tabel. Toch verhoog je $tel onafhankelijk van deze conditie. Ik zou op regel 48 daarom invoegen:

PHP:

if ($gegevens[0] != $bv) continue;

Door continue hoef je geen heel stuk te indenten, en door deze controle direct te plaatsen zodra dat mogelijk is, verlies je zo weinig mogelijk performance.

[ Voor 13% gewijzigd door GlowMouse op 16-04-2007 13:55 ]

[Bergen]

Ik ben niet zo'n voorstander van meerdere opdrachten op 1 regel, maar in geval van zo'n case maak ik altijd een uitzondering, het wordt er veel leesbaarder door:

PHP:

switch($_GET['bv']) {
    case "Groep":     $bv = "ThunnissenGroepbv"; break;
    case "Bouw":      $bv = "ThunnissenBouwbv"; break;
    case "BouwB":     $bv = "ThunnissenBouwBoskoopbv"; break;
    case "Ontw":      $bv = "ThunnissenBouwBoskoopbv"; break;
    case "OntwNoord": $bv = "ThunnissenOntwikkelingNoordbv"; break;
    case "Mat":       $bv = "ThunnissenBouwbvMaterieeldienst"; break;
    default:          $bv = "ThunnissenGroepbv";
}

[FragFrog]

GlowMouse schreef op maandag 16 april 2007 @ 13:35:
CSV vereist niet dat iedere regel met een nieuw record begint. Een field mag best een newline bevatten.

Klopt, en daar kun je ook simpel genoeg iets aan doen Ik zit te slapen, als file() die newlines in een record pakt ben je inderdaad nog steeds de sjaak Mijn excuses, je hebt inderdaad gelijk dat in zo'n situatie file() niet handig is.

Meeste CSV files die ik gebruikt heb in de tijd hadden trouwens mooi geen newlines, als je echt zoveel tekst moet verwerken dat items meerder regels beslaan zijn er IMHO mooiere oplossingen als databases.

[ Voor 13% gewijzigd door FragFrog op 16-04-2007 23:21 ]