<div>Gata, m-am pus serios pe treabă. :-) Am tone de feedback, dar sunt mai mult chestii de principiu și de bune practici, nimic serios de întors pe dos în cod.</div><div><br></div><div><b>Generalități</b></div><div><b><br>
</b></div><div>M-aș simți mai bine dacă în wwwbase/ stau doar chestiile care trebuie să fie accesibile în browser (monitorul). Restul poate sta direct în rădăcină (DEX/crawler) sau eventual în DEX/tools/crawler.</div><b><div>
<b><br></b></div>AbstractCrawler.php</b><div><br><div>- include-urile le-aș pune relativ la __DIR__. Vezi exemple prin tools/, să zicem tools/migration.php. Îmi închipui că vom rula crawlerul din linia de comandă, iar asta se poate întâmpla din orice director (rădăcină sau wwwbase/ sau wwwbase/Crawler/). Nenorocirea cu include și require în PHP este că se uită relativ la CWD, nu relativ la directorul scriptului.</div>
<div><br></div><div>- teoretic util.php le include pe toate celelalte și execută singur db_init(), deci n-ar fi nevoie de ele.</div><div><br></div><div>- liniile 51 și 55 sunt identice, există vreun motiv?</div><div><br>
</div>
<div>- linia 59: curl nu raportează el codul de eroare? Poate e mai bine decât să punem noi din burtă 404. Sau măcar un comentariu în care să explici de ce punem noi 404.</div><div><br></div><div>- linia 81: cred că înțeleg apelul la strstr, dar ar merita un comentariu. Vrei să salvezi doar linkurile către un subdirector, nu? Oricum, ar fi mai clar:</div>
<div><br></div><div>if (StringUtil::startsWith($url, $this->currentLocation)) {</div><div> $urlHash...</div><div> $domain...</div><div> saveLink(...)</div><div>}</div><div><br></div><div>și scăpăm și de returnul urât în mijlocul funcției. :-)</div>
<div><br></div><div>- liniile 83 și 166: ai un typo, urlResouce. :-)</div><div><br></div><div>- saveLink2DB, savePage2DB: aș face metodele astea constructori (sau metode statice) în clasele respective, Link și CrawledPage, pe care le-aș crea în phplib/models. Parcă e încapsularea mai bună în felul ăsta. Aș redenumi Link în CrawledLink, că mai avem câteva obiecte cu Link în titlu. Structura minimă a acestor clase ar fi:</div>
<div><div><br></div><div>class CrawledLink extends BaseObject implements DatedObject {</div><div> public static $_table = 'CrawledLink';</div><div><br></div><div> ...</div><div>}</div><div><br></div><div>Și cu ocazia asta aș trece de la idiorm la paris, adică în loc de ORM::for_table("Link")->create() aș folosi Model::factory('CrawledLink')->create();</div>
<div><br></div></div><div>- linia 132: sunt 99% sigur că id-ul unei pagini proaspăt salvate este populat în câmpul id (sau care-o fi primary key), deci poți zice mai simplu $this->currentPageId = $tableObj->id;</div>
<div><br></div><div>- linia 144: strstr nu e safe aici, căci URL-ul poate fi relativ și poate conține http undeva în interior. Aș folosi StringUtil::startsWith($url, 'http');</div><div><br></div><div>Întrebare random: cum te descurci cu link-urile de tip mailto: sau alte tipuri? Poate ar fi util să verifici explicit că protocolul este unul din http sau https.</div>
<div><br></div><div>- linia 157: idem, bucata asta nu e safe, căci poți avea URL-uri dinamice: index.php?foo=bar. Aș pune explicit condiția String::endsWith($url, 'index.php') sau, mai compact, aș face un regexp de genul</div>
<div><br></div><div>preg_match_all("/^(.*)/index.(php|asp|htm|html)$/", $liteURL, $matches);</div><div><br></div><div>după care dă-i un var_dump($matches) și vezi că una din componente îți dă exact prima paranteză (partea până la ultimul slash).</div>
<div><br></div><div>- linia 179: nu îmi este clar ce rol are $justStarted, poți să-mi explici?</div><div><br></div><div>- liniile 185-197: este nevoie de blocul try/catch? Eu aș face așa (deși știu că uneori tind să fiu neglijent):</div>
<div><br></div><div> $nextLink = (string)ORM::for_table('Link')->raw_query("Select canonicalUrl from Link where canonicalUrl not in (Select url from CrawledPage);")->find_one()</div><div> return $nextLink ? $nextLink->canonicalUrl : null;</div>
<div><br></div><div><b>Crawler.php</b></div><div><b><br></b></div><div>- linia 45: Deocamdată rămâne cum e, dar ar trebui modificată într-o iterație ulterioară. Trebuie să controlăm mai fin, de la site la site, ce are voie crawlerul să facă. Văd două configurări:</div>
<div><br></div><div>- zona în care crawlerul are voie să intre (whitelist și/sau blacklist)</div><div>- patternul paginilor pe care crawlerul are voie să le salveze (regexp whitelist).</div><div><br></div><div>De exemplu, poate crawlerul are voie să intre prin tot site-ul, mai puțin în zona de media. Din zonele permise, salvăm doar unele pagini. Din celelalte extragem linkurile, dar nu le salvăm. Bunăoară, nu ne interesează paginile cu indexuri de articole (pagina principală a unui site de știri), ci doar articolele în sine (regexp /articol/.*.htm).</div>
<div><br></div><div>- în while(1) va fi nevoie de o temporizare la nivel de site (tot într-o versiune ulterioară). Probabil e de ajuns un hash map de la domeniu la timestamp-ul ultimei crawlări pe acel domeniu. Apoi getNextLink() va ști să caute numai în domeniile pe care este ok să le crawleze. Asta va înlocui apelul de sleep: dacă crawlerul are destule site-uri de crawlat încât să nu-l agaseze pe niciunul din ele, poate nu are nevoie să doarmă.</div>
<div><br></div><div>- linia 108: hm, acum îmi dau seama: tu vrei ca o instanță de Crawler să fie dedicată pentru un site, iar apoi să rulezi mai multe crawlere în mai multe threaduri? E ok și așa, sau putem face ca mai sus (un crawler care este și manager).</div>
<div><br></div><div><b>ParsedText și RawPage</b></div><div><b><br></b></div><div>Astea n-au ce căuta în repository. :-) Dacă vrei să le dedici niște directoare în cadrul repository-ului, este ok, dar atunci</div><div><br>
</div><div>(1) nu le-aș pune în wwwbase/. Poate în docs/crawler? sau, cum ziceam, mută tot directorul crawler în DEX/.</div><div>(2) în acele directoare, rulezi "svn propedit svn:ignore ." și pui "*". Atunci poți avea oricâte fișiere acolo și svn nu o să se agite.</div>
<div><br></div><div><b>clean_all.php</b></div><div><b><br></b></div><div>- pentru concizie, definește te rog o constantă $nl = pref_getSectionPreference('crawler', 'new_line'). Sau poți folosi direct PHP_EOL. Nici nu știam de ea. :-)</div>
<div><b><br></b></div><div>- aceleași observații legate de include-uri (util.php este suficient)</div><div><br></div><div>- liniile 14 și 20: ar trebui să folosești valorile definite în dex.conf, nu constante hard-coded</div>
<div><br></div><div>- pentru accesul la baza de date, cred că poți folosi direct db_execute(), definit de noi în phplib/db.php.</div><div><br></div><div>- îmi place că scriptul e idempotent (dacă îl rulezi de două ori la rând nu strică).</div>
<div><br></div><div><b>cookie_jar și crawler_log</b></div><div><b><br></b></div><div>pe astea le-aș pune în /tmp, sau eventual cookie_jar în /tmp și crawler_log în directorul nostru log/. În general, aș evita să pun în repository tot ce nu este identic pentru toată lumea.</div>
<div><br></div><div><b>simple_html_dom.php</b></div><div><b><br></b></div><div>l-aș muta în phplib/, chiar dacă momentan nu este folosit decât de către crawler. Bănuiesc că nu l-ai modificat, nu? Că încercăm să ne educăm să păstrăm bibliotecile externe intacte și să punem în fișiere separate ce customizăm noi.</div>
<div><br></div><div><br></div><div><br><div class="gmail_quote">2013/7/27 <span dir="ltr"><<a href="mailto:automailer@dexonline.ro" target="_blank">automailer@dexonline.ro</a>></span><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Author: alinu<br>
Date: Sat Jul 27 22:36:45 2013<br>
New Revision: 914<br>
<br>
Log:<br>
Prima versiune de Crawler<br>
<br>
Added:<br>
wwwbase/Crawler/<br>
wwwbase/Crawler/.htaccess<br>
wwwbase/Crawler/AbstractCrawler.php<br>
wwwbase/Crawler/AppLog.php<br>
wwwbase/Crawler/Crawler.php<br>
wwwbase/Crawler/ParsedText/<br>
wwwbase/Crawler/ParsedText/2013-07-27 22_20_52<br>
wwwbase/Crawler/ParsedText/2013-07-27 22_21_23<br>
wwwbase/Crawler/ParsedText/2013-07-27 22_21_53<br>
wwwbase/Crawler/ParsedText/2013-07-27 22_22_23<br>
wwwbase/Crawler/ParsedText/2013-07-27 22_22_54<br>
wwwbase/Crawler/ParsedText/2013-07-27 22_23_24<br>
wwwbase/Crawler/ParsedText/2013-07-27 22_23_55<br>
wwwbase/Crawler/ParsedText/2013-07-27 22_24_25<br>
wwwbase/Crawler/ParsedText/2013-07-27 22_24_55<br>
wwwbase/Crawler/ParsedText/2013-07-27 22_25_26<br>
wwwbase/Crawler/ParsedText/2013-07-27 22_25_56<br>
wwwbase/Crawler/ParsedText/2013-07-27 22_26_14<br>
wwwbase/Crawler/ParsedText/2013-07-27 22_26_44<br>
wwwbase/Crawler/ParsedText/2013-07-27 22_27_15<br>
wwwbase/Crawler/ParsedText/2013-07-27 22_27_46<br>
wwwbase/Crawler/ParsedText/2013-07-27 22_28_16<br>
wwwbase/Crawler/ParsedText/2013-07-27 22_28_47<br>
wwwbase/Crawler/ParsedText/2013-07-27 22_29_17<br>
wwwbase/Crawler/RawPage/<br>
wwwbase/Crawler/RawPage/2013-07-27 22_20_52<br>
wwwbase/Crawler/RawPage/2013-07-27 22_21_23<br>
wwwbase/Crawler/RawPage/2013-07-27 22_21_53<br>
wwwbase/Crawler/RawPage/2013-07-27 22_22_23<br>
wwwbase/Crawler/RawPage/2013-07-27 22_22_54<br>
wwwbase/Crawler/RawPage/2013-07-27 22_23_24<br>
wwwbase/Crawler/RawPage/2013-07-27 22_23_55<br>
wwwbase/Crawler/RawPage/2013-07-27 22_24_25<br>
wwwbase/Crawler/RawPage/2013-07-27 22_24_55<br>
wwwbase/Crawler/RawPage/2013-07-27 22_25_26<br>
wwwbase/Crawler/RawPage/2013-07-27 22_25_56<br>
wwwbase/Crawler/RawPage/2013-07-27 22_26_14<br>
wwwbase/Crawler/RawPage/2013-07-27 22_26_44<br>
wwwbase/Crawler/RawPage/2013-07-27 22_27_15<br>
wwwbase/Crawler/RawPage/2013-07-27 22_27_46<br>
wwwbase/Crawler/RawPage/2013-07-27 22_28_16<br>
wwwbase/Crawler/RawPage/2013-07-27 22_28_47<br>
wwwbase/Crawler/RawPage/2013-07-27 22_29_17<br>
wwwbase/Crawler/clean_all.php<br>
wwwbase/Crawler/cookie_jar<br>
wwwbase/Crawler/crawler_log<br>
wwwbase/Crawler/simple_html_dom.php<br><br></blockquote></div></div></div>