Issue 4432047: informacny list parser plus test (Closed)
|
Created:
13 years ago by jakub.marek Modified:
13 years ago CC:
fajr-devel+codereview_googlegroups.com, jakub.markos_gmail.com Base URL:
http://fajr.googlecode.com/svn/trunk/ Visibility:
Public. |
Descriptioninformacny list parser plus test
Patch Set 1 #
Total comments: 46
Patch Set 2 : '' #
Total comments: 16
Patch Set 3 : '' #
Total comments: 27
Patch Set 4 : '' #
Total comments: 19
Patch Set 5 : '' #
Total comments: 1
MessagesTotal messages: 18
http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... File src/libfajr/data_manipulation/InformacnyListParser.php (right): http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListParser.php:47: public function fixProblematicTags($html) { Tiez presunut do ParserUtils, mozno trebars s parametrom $replaceBR (neviem, ci sa to robi aj v table parseri) http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListParser.php:67: public function fixIdAttributes(DOMDocument $dom) { Presunut do ParserUtils http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListParser.php:80: * Fix non-breakable spaces which were converted to special character furing parsing. preklep furing -> during Tiez by sa mohlo presunut do novej classy ParserUtils, kedze sa to pouziva aj v table parseri http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListParser.php:104: public function createDomFromHtml($html) { Presunut do ParserUtils http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListParser.php:117: public function spracujB(DOMElement $final, $pole) { Chyba dokumentacia. Asi nemusi byt public. http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListParser.php:153: public function createTableFromHtml($aisResponseHtml) { Chyba dokumentacia. Asi by sa hodilo mat iny nazov metody ;)
Sign in to reply to this message.
http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... File src/libfajr/data_manipulation/InformacnyListDataImpl.php (right): http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListDataImpl.php:28: public $list; $list by som dal ako private, kedze k jednotlivym polozkam chceme pristupovat cez metody. http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListDataImpl.php:31: $parser = new InformacnyListParser(); Pre testovatelnost nie je dobre, pokial trieda potrebuje na svoje fungovanie dalsie, ktore ma napevno v sebe definovane. Tento $parser by som preto odporucal presunut do parametrov konstruktora. http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListDataImpl.php:34: $this->setAttribute($table[0], 'SKOLA_FAKULTA'); Tu by bolo lepsie mieto stringu 'SKOLA_FAKULTA' pouzit hotnodu InformacnyListAttributeEnum::SKOLA_FAKULTA, dovod som ti napisal do testu. (pre vsetky atributy) http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListDataImpl.php:62: */ Pre automaticke generovanie dokumentacie je vhodne, aby komentare boli vo formate DocBlocks. Inspiruj sa ostatnymi komentarmi a pripadne aj http://en.wikipedia.org/wiki/PHPDoc http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListDataImpl.php:68: $this->list["$attribute"][] = trim($value[$i]); Je trochu vyhodnejsie pouzit priamo $attribute bez toho, aby to bolo v stringu, pretoze potom interpreter PHP nemusi ten string spracovavat. Dalej sa ti to tiez este opakuje. http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListDataImpl.php:86: } catch (NotImplementedException $e) { Toto odchytavanie vynimky na tomto mieste nema zmysel, pretoze kod v try bloku taku nevyhadzuje. http://codereview.appspot.com/4432047/diff/1/tests/src/libfajr/data_manipulat... File tests/src/libfajr/data_manipulation/InformacnyListDataImplTest.php (right): http://codereview.appspot.com/4432047/diff/1/tests/src/libfajr/data_manipulat... tests/src/libfajr/data_manipulation/InformacnyListDataImplTest.php:40: $this->assertTrue($this->informacnyList->hasAttribute('NAZOV')); Tuto metodu naozaj chceme volat s parametrami type InformacnyListAttributeEnum::NAZOV. Ma to totiz nasledovnu vyhodu: Ak budem chciet ziskat konkretny atribut v aplikacii, tak si nemusim spominat ake vsetky atributy existuju, ale moje IDE mi to bude vediet pekne cez autocomplete nahintit. http://codereview.appspot.com/4432047/diff/1/tests/src/libfajr/data_manipulat... File tests/src/libfajr/data_manipulation/InformacnyListParserTest.php (right): http://codereview.appspot.com/4432047/diff/1/tests/src/libfajr/data_manipulat... tests/src/libfajr/data_manipulation/InformacnyListParserTest.php:43: $this->assertEquals($table[6], '4'); Tu by som dal do testu uplne vsetky hodnoty. Nech sa nestane taka vec, ze len napriklad jedna sa zle sparsuje a bude to prave ta, ktoru netestujeme.
Sign in to reply to this message.
http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... File src/libfajr/data_manipulation/InformacnyListDataImpl.php (right): http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListDataImpl.php:31: $parser = new InformacnyListParser(); Ja by som to cele spravil opacne. InformacnyListDataImpl je len trieda starajuca sa o data a nie o parsovanie, tym padom nema co parsovat. Nech pekne krastne InformacnyListParser ma metodu parse() ktora vrati vyrobeny InformacnyListDataImpl, teraz napriklad informacny list naozaj nema co vediet o tom ako vyzera tabulka parsera!
Sign in to reply to this message.
http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... File src/libfajr/data_manipulation/InformacnyListDataImpl.php (right): http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListDataImpl.php:34: $this->setAttribute($table[0], 'SKOLA_FAKULTA'); este som zabudol -- toto ma byt jedno pole id->hodnota a potom jeden foreach! Plus otazka - ked sa zmeni format informacnych listov, dojdeme na to nejako alebo vsade dropujeme vsetky nadpisy a vobec sa to nedozvieme? Ak to druhe, tak je to VELMI ZLE lebo AIS ma tendenciu robit zmeny v roznych vystupoch! http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListDataImpl.php:64: public function setAttribute($value, $attribute) { 1. poradie $atribute $value 2. za akych okolnosti moze byt $value pole a preco sa to v tom pripade merguju polia? Od nastavovania pomocou "set" by som predpokladal, ze co dam do argumentu, to mi ostane. To iste pre "trim", trim ma robit parser -- ved preco by som nemohol mat netrimovane atributy ak potrebujem? http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... File src/libfajr/data_manipulation/InformacnyListParser.php (right): http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListParser.php:3: // Copyright (c) 2010-2011 The Fajr authors (see AUTHORS). uz mame aj novsi hlavickovy s @copyright, len este nebol porobeny vsade, skus pozriet nejake subory priamo v src (Fajr.php by mohol byt novy) http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListParser.php:131: if ($text_sused->tagName == 'p') { WTF je je 8x vnoreny scope? Neda sa to prepisat na omnoho zrozumitelnejsi kod? A preco si posuvame pole? nemozeme vratit pole a v povodnej funkcii mergovat? http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListParser.php:165: foreach ($b as $b_key) { aky ma toto vyznam? Preco nemozem zobrat jednoducho prvy $b element a spracovat ale musim tam robit divny while ktory to aj tak spravi iba raz? http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListParser.php:177: if ($i != 0) { zase ten nehorazny scope - preco napriklad nepouzit if ($i == 0) { $i = 1; continue; } http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListParser.php:184: //$final = $this->fixNbsp($final->textContent); zabudnute vykomentene veci http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListParser.php:185: if ($final->nodeType == \XML_ELEMENT_NODE) { opat, preco nerobit negaciu podmienky a continue?
Sign in to reply to this message.
http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... File src/libfajr/data_manipulation/InformacnyListDataImpl.php (right): http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListDataImpl.php:28: public $list; On 2011/04/17 16:59:22, majak wrote: > $list by som dal ako private, kedze k jednotlivym polozkam chceme pristupovat > cez metody. Done. http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListDataImpl.php:31: $parser = new InformacnyListParser(); tak to spravim tym druhym sposobom a sice, ze toto hodim do parsera a tu bude len jednoduchy konstruktor http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListDataImpl.php:31: $parser = new InformacnyListParser(); no snad som dobre pochopil, o co ti ide, momentalne mi to ale nechce ist.. musi sa na to dakto asi pozret http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListDataImpl.php:34: $this->setAttribute($table[0], 'SKOLA_FAKULTA'); to prepisem http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListDataImpl.php:34: $this->setAttribute($table[0], 'SKOLA_FAKULTA'); jedno pole id->hodnota, sak to skoro sedi nie? neviem na aku velku zmenu formatu by mal byt schopny parser reagovat, ak sa zamenia nazov s fakultou, tak na to nepridem.. http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListDataImpl.php:62: */ On 2011/04/17 16:59:22, majak wrote: > Pre automaticke generovanie dokumentacie je vhodne, > aby komentare boli vo formate DocBlocks. > Inspiruj sa ostatnymi komentarmi a pripadne aj > http://en.wikipedia.org/wiki/PHPDoc Done. http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListDataImpl.php:64: public function setAttribute($value, $attribute) { ad 2; $value moze byt pole, ked ide o literaturu, ktora moze mat viac poloziek (aspon to je jedina vec z examplu, ktora ich ma), k tomu trimu to moze robit aj parser, a asi to aj bude robit parser, ked presuniem cely konstruktor odtialto do parsera. http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListDataImpl.php:68: $this->list["$attribute"][] = trim($value[$i]); nechapem, co presne je zle? atribut nema byt v stringu, akom? http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListDataImpl.php:86: } catch (NotImplementedException $e) { On 2011/04/17 16:59:22, majak wrote: > Toto odchytavanie vynimky na tomto mieste nema zmysel, pretoze kod v try bloku > taku nevyhadzuje. Done. http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... File src/libfajr/data_manipulation/InformacnyListParser.php (right): http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListParser.php:3: // Copyright (c) 2010-2011 The Fajr authors (see AUTHORS). On 2011/04/17 19:58:59, ppershing wrote: > uz mame aj novsi hlavickovy s @copyright, len este nebol porobeny vsade, skus > pozriet nejake subory priamo v src (Fajr.php by mohol byt novy) Done. http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListParser.php:47: public function fixProblematicTags($html) { On 2011/04/17 15:11:01, anty wrote: > Tiez presunut do ParserUtils, mozno trebars s parametrom $replaceBR (neviem, ci > sa to robi aj v table parseri) Done. http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListParser.php:67: public function fixIdAttributes(DOMDocument $dom) { On 2011/04/17 15:11:01, anty wrote: > Presunut do ParserUtils Done. http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListParser.php:80: * Fix non-breakable spaces which were converted to special character furing parsing. On 2011/04/17 15:11:01, anty wrote: > preklep furing -> during > > Tiez by sa mohlo presunut do novej classy ParserUtils, kedze sa to pouziva aj v > table parseri Done. http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListParser.php:104: public function createDomFromHtml($html) { On 2011/04/17 15:11:01, anty wrote: > Presunut do ParserUtils Done. http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListParser.php:117: public function spracujB(DOMElement $final, $pole) { On 2011/04/17 15:11:01, anty wrote: > Chyba dokumentacia. > Asi nemusi byt public. Done. http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListParser.php:131: if ($text_sused->tagName == 'p') { Neviem, ci to dokazem prepisat.. Posuvam si pole, lebo inak ma to nenapadlo. Ked si vratim pole, neni to potom jedno? http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListParser.php:153: public function createTableFromHtml($aisResponseHtml) { On 2011/04/17 15:11:01, anty wrote: > Chyba dokumentacia. > Asi by sa hodilo mat iny nazov metody ;) Done. http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListParser.php:165: foreach ($b as $b_key) { On 2011/04/17 19:58:59, ppershing wrote: > aky ma toto vyznam? Preco nemozem zobrat jednoducho prvy $b element a spracovat > ale musim tam robit divny while ktory to aj tak spravi iba raz? Done. http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListParser.php:177: if ($i != 0) { On 2011/04/17 19:58:59, ppershing wrote: > zase ten nehorazny scope - preco napriklad nepouzit > if ($i == 0) { > $i = 1; > continue; > } Done. http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListParser.php:184: //$final = $this->fixNbsp($final->textContent); On 2011/04/17 19:58:59, ppershing wrote: > zabudnute vykomentene veci Done. http://codereview.appspot.com/4432047/diff/1/src/libfajr/data_manipulation/In... src/libfajr/data_manipulation/InformacnyListParser.php:185: if ($final->nodeType == \XML_ELEMENT_NODE) { On 2011/04/17 19:58:59, ppershing wrote: > opat, preco nerobit negaciu podmienky a continue? aku podmienku by som chcel negovat? http://codereview.appspot.com/4432047/diff/1/tests/src/libfajr/data_manipulat... File tests/src/libfajr/data_manipulation/InformacnyListParserTest.php (right): http://codereview.appspot.com/4432047/diff/1/tests/src/libfajr/data_manipulat... tests/src/libfajr/data_manipulation/InformacnyListParserTest.php:43: $this->assertEquals($table[6], '4'); ten informacny list co sluzi ako priklad neobsahuje asi polovicu hodnot, ale ok
Sign in to reply to this message.
aj to opat uploadni!
Sign in to reply to this message.
http://codereview.appspot.com/4432047/diff/1/tests/src/libfajr/data_manipulat... File tests/src/libfajr/data_manipulation/InformacnyListDataImplTest.php (right): http://codereview.appspot.com/4432047/diff/1/tests/src/libfajr/data_manipulat... tests/src/libfajr/data_manipulation/InformacnyListDataImplTest.php:40: $this->assertTrue($this->informacnyList->hasAttribute('NAZOV')); On 2011/04/17 16:59:22, majak wrote: > Tuto metodu naozaj chceme volat s parametrami type > InformacnyListAttributeEnum::NAZOV. > Ma to totiz nasledovnu vyhodu: > Ak budem chciet ziskat konkretny atribut v aplikacii, > tak si nemusim spominat ake vsetky atributy existuju, > ale moje IDE mi to bude vediet pekne cez autocomplete nahintit. Done.
Sign in to reply to this message.
http://codereview.appspot.com/4432047/diff/8001/src/libfajr/data_manipulation... File src/libfajr/data_manipulation/InformacnyListDataImpl.php (right): http://codereview.appspot.com/4432047/diff/8001/src/libfajr/data_manipulation... src/libfajr/data_manipulation/InformacnyListDataImpl.php:11: * @author Martin Králik <majak47@gmail.com>, Jakub Marek <jakub.marek@gmail.com> daj druhy krat @author, myslim ze to tak funguje ;-) http://codereview.appspot.com/4432047/diff/8001/src/libfajr/data_manipulation... File src/libfajr/data_manipulation/InformacnyListParser.php (right): http://codereview.appspot.com/4432047/diff/8001/src/libfajr/data_manipulation... src/libfajr/data_manipulation/InformacnyListParser.php:53: $this->setAttribute(InformacnyListAttributeEnum::SKOLA_FAKULTA, $table[0]); stale plati povodny comment o jednom poli a foreach http://codereview.appspot.com/4432047/diff/8001/src/libfajr/data_manipulation... src/libfajr/data_manipulation/InformacnyListParser.php:123: private function spracujB(DOMElement $final, $pole) { stale plati, ze bude lepsie nebrat $pole ako parameter, iba vracat pole a nasledne vo funkcii ktora vola spracujB() robit array_merge. Btw -- vedela by funkcia spracujB vratit aj obsah toho "<b>" tagu? Nech vieme checkovat ze nam nic nezmenili v aise. http://codereview.appspot.com/4432047/diff/8001/src/libfajr/data_manipulation... File src/libfajr/data_manipulation/ParserUtils.php (right): http://codereview.appspot.com/4432047/diff/8001/src/libfajr/data_manipulation... src/libfajr/data_manipulation/ParserUtils.php:35: class ParserUtils { v pripade ze tieto veci su rovnake ako v tom parseri co som pisal ja, tak ich treba odtial umazat a mat ich iba na tomto jedinom mieste http://codereview.appspot.com/4432047/diff/8001/tests/src/libfajr/data_manipu... File tests/src/libfajr/data_manipulation/InformacnyListParserTest.php (right): http://codereview.appspot.com/4432047/diff/8001/tests/src/libfajr/data_manipu... tests/src/libfajr/data_manipulation/InformacnyListParserTest.php:43: 'Univerzita Komenskďż˝ho v Bratislave - Fakulta matematiky, fyziky a informatiky'); encoding, treba to mat v utf-8, ak su povodne data v niecom inom, treba konvertovat v parseri http://codereview.appspot.com/4432047/diff/8001/tests/src/libfajr/data_manipu... tests/src/libfajr/data_manipulation/InformacnyListParserTest.php:44: $this->assertEquals($table->list[InformacnyListAttributeEnum::KOD], 'FMFI.KI/2-INF-235/00'); nemoze sa spravit assertEqauls na cele pole?
Sign in to reply to this message.
http://codereview.appspot.com/4432047/diff/8001/src/libfajr/data_manipulation... File src/libfajr/data_manipulation/InformacnyListDataImpl.php (right): http://codereview.appspot.com/4432047/diff/8001/src/libfajr/data_manipulation... src/libfajr/data_manipulation/InformacnyListDataImpl.php:29: public $list; Toto je ako done, ale ja tu stale vidim public... http://codereview.appspot.com/4432047/diff/8001/src/libfajr/data_manipulation... src/libfajr/data_manipulation/InformacnyListDataImpl.php:31: public function __construct($list) { tu v konstruktore asi nechceme mat ziaden parameter a inicializovat $this->list = array(); Obsah sa bude nastavovat cez setAttribute... http://codereview.appspot.com/4432047/diff/8001/src/libfajr/data_manipulation... File src/libfajr/data_manipulation/InformacnyListParser.php (right): http://codereview.appspot.com/4432047/diff/8001/src/libfajr/data_manipulation... src/libfajr/data_manipulation/InformacnyListParser.php:38: private $list; Toto tu nema byt, o to sa stara predsa InformacnyListDataImpl http://codereview.appspot.com/4432047/diff/8001/src/libfajr/data_manipulation... src/libfajr/data_manipulation/InformacnyListParser.php:51: $this->list = array(); namiesto $this->list = array(); daj $list = new InformacnyListDataImpl(); a potom v setAttributoch nizsie pouzi namiesto $this radsej $list http://codereview.appspot.com/4432047/diff/8001/src/libfajr/data_manipulation... src/libfajr/data_manipulation/InformacnyListParser.php:77: $new_list = new InformacnyListDataImpl($this->list); Toto ako som popisal vyssie http://codereview.appspot.com/4432047/diff/8001/src/libfajr/data_manipulation... src/libfajr/data_manipulation/InformacnyListParser.php:78: return $new_list; Tiez (t.j. tu potom bude return $list) http://codereview.appspot.com/4432047/diff/8001/src/libfajr/data_manipulation... src/libfajr/data_manipulation/InformacnyListParser.php:88: public function setAttribute($attribute, $value) { Preco si presunul setAttribute sem? To ma byt v InformacnyListDataImpl, nie?
Sign in to reply to this message.
http://codereview.appspot.com/4432047/diff/8001/src/libfajr/data_manipulation... File src/libfajr/data_manipulation/InformacnyListDataImpl.php (right): http://codereview.appspot.com/4432047/diff/8001/src/libfajr/data_manipulation... src/libfajr/data_manipulation/InformacnyListDataImpl.php:11: * @author Martin Králik <majak47@gmail.com>, Jakub Marek <jakub.marek@gmail.com> On 2011/04/18 20:59:59, ppershing wrote: > daj druhy krat @author, myslim ze to tak funguje ;-) Done. http://codereview.appspot.com/4432047/diff/8001/src/libfajr/data_manipulation... File src/libfajr/data_manipulation/InformacnyListParser.php (right): http://codereview.appspot.com/4432047/diff/8001/src/libfajr/data_manipulation... src/libfajr/data_manipulation/InformacnyListParser.php:123: private function spracujB(DOMElement $final, $pole) { done, ale s tym vracanim parametru nechapem.. to co je v tagu <b> je vlastne parameter $final, takze da sa to vratit, neviem ale ako sa vracaju dve hodnoty alebo ako si to myslel http://codereview.appspot.com/4432047/diff/8001/src/libfajr/data_manipulation... File src/libfajr/data_manipulation/ParserUtils.php (right): http://codereview.appspot.com/4432047/diff/8001/src/libfajr/data_manipulation... src/libfajr/data_manipulation/ParserUtils.php:35: class ParserUtils { no ja som si z nich odmazal ten trace, lebo som sa v tom nevedel vyznat.. anty povedal, ze by som to mohol dat tak jak to bolo, akurat u seba pouzivat nulltrace, aj ked presne neviem co sa tym mysli.
Sign in to reply to this message.
http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... File src/libfajr/data_manipulation/InformacnyListDataImpl.php (right): http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListDataImpl.php:41: if (!array_key_exists($attribute, $this->list)) { ako je to v parent dokumentacii -- ma sa vracat false alebo exception? http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... File src/libfajr/data_manipulation/InformacnyListParser.php (right): http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:50: $table = $this->parseHtmlIntoTable($html); nikde nevidim, ze checkujeme nejaky sposobom tu $table ci bola vyparsovana spravne. http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:52: $new_list = new InformacnyListDataImpl($this->list); najlepsie ak by ListDataImpl mal nepovinny parameter v konstruktore! http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:79: $this->setAttribute($attribute, $value); {} http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:132: return $pole; preco nie proste return ''; ? http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:137: return $pole; tunak tiez mozno vratit rovno return array(Parser...) http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:141: return; ako to ze nevraciame pole ked v @returns je array? http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:145: if ($text_sused->nodeType != \XML_ELEMENT_NODE) { wtf za porovnanie je toto? netusim som ze typeof funguje takymto stylom a nebol by som si tym isty -- je to niekde v dokumentacii? To iste je este niekde nizsie http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:150: $pole[0][] = ParserUtils::fixNbsp($text_sused->nodeValue); [0] je urcite dobre? http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:169: // $aisResponseHtml = iconv("WINDOWS-1250", "UTF-8", $aisResponseHtml); vykomentene riadky nemaju co robit v kode http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:174: $string = $domWholeHtml->saveHTML(); naco je tento riadok? http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:203: if ($final->tagName == 'div') { mozno by som celu tuto vec strcil do novej funkcie "parseDiv" http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... File src/libfajr/data_manipulation/ParserUtils.php (right): http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/ParserUtils.php:1: <?php odmazany trace je zly, pretoze tym padom je to nekompatibilne s mojim parserom. Uz som raz pisal, ze tie funkcie treba nasledne prehodit aj tam, nie? Anty trace myslel stylom $trace = new NullTrace() a mas instanciu Trace objektu. Anyway, interne by si mozno mohol pouzivat Trace aj v tvojom parsovaci, nech vieme ako sa postupne parsovali data! http://codereview.appspot.com/4432047/diff/16001/tests/src/libfajr/data_manip... File tests/src/libfajr/data_manipulation/InformacnyListParserTest.php (right): http://codereview.appspot.com/4432047/diff/16001/tests/src/libfajr/data_manip... tests/src/libfajr/data_manipulation/InformacnyListParserTest.php:44: $this->assertEquals( assertEquals ma poradie argumentov (expected, actual)
Sign in to reply to this message.
http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... File src/libfajr/data_manipulation/InformacnyListParser.php (right): http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:145: if ($text_sused->nodeType != \XML_ELEMENT_NODE) { On 2011/04/21 16:51:31, ppershing wrote: > wtf za porovnanie je toto? netusim som ze typeof funguje takymto stylom a nebol > by som si tym isty -- je to niekde v dokumentacii? To iste je este niekde nizsie Toto nie je typeof. Jedna sa o atribut triedy DOMNode, od ktorej to ma DOMElement podedene. http://www.php.net/manual/en/class.domnode.php#domnode.props.nodetype
Sign in to reply to this message.
http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... File src/libfajr/data_manipulation/InformacnyListDataImpl.php (right): http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListDataImpl.php:41: if (!array_key_exists($attribute, $this->list)) { On 2011/04/21 16:51:31, ppershing wrote: > ako je to v parent dokumentacii -- ma sa vracat false alebo exception? @returns string|false Value of requested attribute or false on failure. http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... File src/libfajr/data_manipulation/InformacnyListParser.php (right): http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:50: $table = $this->parseHtmlIntoTable($html); On 2011/04/21 16:51:31, ppershing wrote: > nikde nevidim, ze checkujeme nejaky sposobom tu $table ci bola vyparsovana > spravne. Done. http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:52: $new_list = new InformacnyListDataImpl($this->list); On 2011/04/21 16:51:31, ppershing wrote: > najlepsie ak by ListDataImpl mal nepovinny parameter v konstruktore! Done. http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:79: $this->setAttribute($attribute, $value); On 2011/04/21 16:51:31, ppershing wrote: > {} Done. http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:132: return $pole; On 2011/04/21 16:51:31, ppershing wrote: > preco nie proste return ''; ? Done. http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:137: return $pole; On 2011/04/21 16:51:31, ppershing wrote: > tunak tiez mozno vratit rovno return array(Parser...) Done. http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:141: return; On 2011/04/21 16:51:31, ppershing wrote: > ako to ze nevraciame pole ked v @returns je array? Done. http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:150: $pole[0][] = ParserUtils::fixNbsp($text_sused->nodeValue); On 2011/04/21 16:51:31, ppershing wrote: > [0] je urcite dobre? hej, pretoze chcem spravit pole v ramci jedneho miesta v poli, ktore vraciam, lebo potom to mergujem http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:169: // $aisResponseHtml = iconv("WINDOWS-1250", "UTF-8", $aisResponseHtml); On 2011/04/21 16:51:31, ppershing wrote: > vykomentene riadky nemaju co robit v kode Done. http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:174: $string = $domWholeHtml->saveHTML(); On 2011/04/21 16:51:31, ppershing wrote: > naco je tento riadok? Done. http://codereview.appspot.com/4432047/diff/16001/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:203: if ($final->tagName == 'div') { On 2011/04/21 16:51:31, ppershing wrote: > mozno by som celu tuto vec strcil do novej funkcie "parseDiv" Done. http://codereview.appspot.com/4432047/diff/16001/tests/src/libfajr/data_manip... File tests/src/libfajr/data_manipulation/InformacnyListParserTest.php (right): http://codereview.appspot.com/4432047/diff/16001/tests/src/libfajr/data_manip... tests/src/libfajr/data_manipulation/InformacnyListParserTest.php:44: $this->assertEquals( On 2011/04/21 16:51:31, ppershing wrote: > assertEquals ma poradie argumentov (expected, actual) Done.
Sign in to reply to this message.
majak, mohol by si sa na to pozriet aj ty? odo mna lgtm http://codereview.appspot.com/4432047/diff/17004/src/libfajr/data_manipulatio... File src/libfajr/data_manipulation/InformacnyListParser.php (right): http://codereview.appspot.com/4432047/diff/17004/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:243: $this->attribute_names = array(); tato privatna premenna sa mi moc nepaci, ale co uz, moc elegantnejsie to neviem riesit
Sign in to reply to this message.
Mam este par pripomienok. http://codereview.appspot.com/4432047/diff/17004/src/libfajr/data_manipulatio... File src/libfajr/data_manipulation/InformacnyListDataImpl.php (right): http://codereview.appspot.com/4432047/diff/17004/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListDataImpl.php:11: * @author Martin Králik <majak47@gmail.com>, Jakub Marek <jakub.marek@gmail.com> Druhy autor na novy riadok. http://codereview.appspot.com/4432047/diff/17004/src/libfajr/data_manipulatio... File src/libfajr/data_manipulation/InformacnyListParser.php (right): http://codereview.appspot.com/4432047/diff/17004/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:56: $new_list = new InformacnyListDataImpl($this->list); Nevidim dovod tohto vytvorenia objektu, kedze o 20 riadkov nizsie ho vytvaras znovu. http://codereview.appspot.com/4432047/diff/17004/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:95: * @param string $value Tu by sa zislo spomenut, ze to nemusi byt len string. http://codereview.appspot.com/4432047/diff/17004/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:106: $this->list[$attribute] = self::trim_delete_newline($value[0]); Nie som si uplne isty, ci ked je v poli len jeden prvok, ze musi byt dostupny pod indexom 0. Nebolo by lepsie pouzit napriklad end($value)? http://codereview.appspot.com/4432047/diff/17004/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:121: static function trim_delete_newline($string) { Mena vsetkych metod pouzivaju camelCase. Bolo by dobre, keby aj tato bola konzistentna. http://codereview.appspot.com/4432047/diff/17004/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:122: return trim((string) str_replace(array("\r", "\r\n", "\n"), '', $string)); Preco je tam pretypovanie na string? Pride mi lepsie overovat, ci $string je naozaj string cez Precondition. http://codereview.appspot.com/4432047/diff/17004/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:242: $pole = array(); Ja by som to premenoval na $parsedData alebo nieco take, meno $pole je dost nepopisujuce. http://codereview.appspot.com/4432047/diff/17004/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:247: $domWholeHtml = ParserUtils::createDomFromHtml($trace, $html); Pozor, toto moze vyhodit vynimku a ty to nijak neosetrujes. (Resp. nie je to spomenute v komentari.) http://codereview.appspot.com/4432047/diff/17004/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:262: $i = 1; Nenazves to trochu inak? Napriklad boolean $firstTr. Lebo chvilu mi trvalo, kym som pochopil, ze je to len na preskocenie prveho cyklu a inak sa to nepouziva.
Sign in to reply to this message.
http://codereview.appspot.com/4432047/diff/17004/src/libfajr/data_manipulatio... File src/libfajr/data_manipulation/InformacnyListDataImpl.php (right): http://codereview.appspot.com/4432047/diff/17004/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListDataImpl.php:11: * @author Martin Králik <majak47@gmail.com>, Jakub Marek <jakub.marek@gmail.com> On 2011/04/25 15:32:52, majak wrote: > Druhy autor na novy riadok. Done. http://codereview.appspot.com/4432047/diff/17004/src/libfajr/data_manipulatio... File src/libfajr/data_manipulation/InformacnyListParser.php (right): http://codereview.appspot.com/4432047/diff/17004/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:56: $new_list = new InformacnyListDataImpl($this->list); On 2011/04/25 15:32:52, majak wrote: > Nevidim dovod tohto vytvorenia objektu, > kedze o 20 riadkov nizsie ho vytvaras znovu. Done. http://codereview.appspot.com/4432047/diff/17004/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:95: * @param string $value On 2011/04/25 15:32:52, majak wrote: > Tu by sa zislo spomenut, ze to nemusi byt len string. Done. http://codereview.appspot.com/4432047/diff/17004/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:106: $this->list[$attribute] = self::trim_delete_newline($value[0]); On 2011/04/25 15:32:52, majak wrote: > Nie som si uplne isty, ci ked je v poli len jeden prvok, ze musi byt dostupny > pod indexom 0. > Nebolo by lepsie pouzit napriklad end($value)? Done. http://codereview.appspot.com/4432047/diff/17004/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:121: static function trim_delete_newline($string) { On 2011/04/25 15:32:52, majak wrote: > Mena vsetkych metod pouzivaju camelCase. > Bolo by dobre, keby aj tato bola konzistentna. Done. http://codereview.appspot.com/4432047/diff/17004/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:122: return trim((string) str_replace(array("\r", "\r\n", "\n"), '', $string)); On 2011/04/25 15:32:52, majak wrote: > Preco je tam pretypovanie na string? > Pride mi lepsie overovat, ci $string je naozaj string cez Precondition. Done. http://codereview.appspot.com/4432047/diff/17004/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:242: $pole = array(); On 2011/04/25 15:32:52, majak wrote: > Ja by som to premenoval na $parsedData alebo nieco take, > meno $pole je dost nepopisujuce. Done. http://codereview.appspot.com/4432047/diff/17004/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:247: $domWholeHtml = ParserUtils::createDomFromHtml($trace, $html); On 2011/04/25 15:32:52, majak wrote: > Pozor, toto moze vyhodit vynimku a ty to nijak neosetrujes. > (Resp. nie je to spomenute v komentari.) Done. http://codereview.appspot.com/4432047/diff/17004/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:262: $i = 1; On 2011/04/25 15:32:52, majak wrote: > Nenazves to trochu inak? > Napriklad boolean $firstTr. > Lebo chvilu mi trvalo, kym som pochopil, > ze je to len na preskocenie prveho cyklu a inak sa to nepouziva. Done.
Sign in to reply to this message.
lgtm http://codereview.appspot.com/4432047/diff/15003/src/libfajr/data_manipulatio... File src/libfajr/data_manipulation/InformacnyListParser.php (right): http://codereview.appspot.com/4432047/diff/15003/src/libfajr/data_manipulatio... src/libfajr/data_manipulation/InformacnyListParser.php:94: * @param string/array $value Mam pocit ze sa nepouziva "/" ale "|".
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
