salebab / database

PHP/MySQL database wrapper, extends PDO and PDOStatement
MIT License
76 stars 25 forks source link

Refactoring #1

Closed komita1981 closed 11 years ago

komita1981 commented 11 years ago

Zdravo Sale, Evo ja sam dao svoj skromni doprinos. Malo sam se igrao sa kodom - nisam ni testirao krajnji rezultat tj. da li je sve u redu. Dobro je što si pokrenuo ovo - nadam se da će još neko postaviti svoj kod.

Mislim da imaš bug u metodi limit() kada prima dva parametra a drugi parametar nije null. Tada on treba da dodje na prvo mesto a prvi parametar na drugo iza zareza.

salebab commented 11 years ago

Cao, pre svega hvala za pull request! Slazem se sa nekim stvarima, ali sa nekima. Builder klasa je sklepana na brzinu da bi zavrsila neke stvari, ali postoje slucajevi u kojima nece raditi kako treba, tako da mislim da bi je mozda trebalo iz pocetka napisati kako valja.

Ono sto mi se ne svidja je sto u getQuery() metodi 20 vrlo jednostavnih linija koda si zamenio sa pozivima 7 metoda i prosirio kod na dodatnih 130 linija koje rade maltene istu stvar. Mislim da je nepotrebno to, bilo bi dobro da se jos neko ukljuci i kaze sta misli :)

komita1981 commented 11 years ago

I ja bih voleo da čujem još nečije mišljenje. Kao prvo mislim da nije 130 linija - docblock se ne računa :-) . Razlog zašto sam uveo privatne metode jeste čitljivost koda. Mislim da se ovako kod brže tumači jer i ljudi tumače kod kao i mašine tj ulazili bi u svaki kondicional a ovako programer odmah vidi iz kojih celina je sastavljen sql query bez potrebe da ulaze u pojedinosti. Ideja mi je da smanjim vreme tumačenja a ne broj linija koda.

salebab commented 11 years ago

Da, potpuno jasno :)

komita1981 commented 11 years ago

Ternary operator ima smisla kad je jednostavan i ja ga u tim situacijama koristim. Čim je malo komplikovanije to ide u kondicional iako bi moglo da se napiše preko ternary operatora. Da citiram - "By default, use an if/else. The ternary ?: should be used only for the simplest cases.".

Mislim da sam nastavio da koristim camelCase - bar sam se trudio da koristim :-) - možda je negde promaklo

Što se tiče return void u doc block-u - malo ću proučiti kako je ispravno

ldusan84 commented 11 years ago

Pa da, to je bas poenta, tako unapredjujemo znanje. Mislim nije napisano nesto mnogo puno koda, al eto i malo koda kad se napise moze da se analizira u normalnoj atmosferi bez ego-a itd. :)

salebab commented 11 years ago

Merged. Nakon toga sam sredio jos malo kod kako bi bi u skladu sa PSR1/PSR2 standardom. Kod koji sledi zaista ne mogu da prihvatim i mislim da je generalno mnogo losa praksa da se promenjive setuju u kondicionalu:

! is_array($params) and $params = array($params);

tako da sam zamenio takve izraze sa citljivijim i jednstavnijim:

if(!is_array($params)) {
    $params = array($params);
}

P.S. Ternary operator je ok sto se mene tice, ali ovo no no :)

Djuki commented 11 years ago

@salebab Slažem se ovo tvoje je čitljivije. A to sa kondicionalom iznad je jednom twitnuo Phil Sturgeon i pitao se zašto developeri ne koriste takvu sintaksu :)

ldusan84 commented 11 years ago

Samo sec. Za ovo:

! is_array($params) and $params = array($params);

je Phil Sturgeon rekao da je dobro???

Djuki commented 11 years ago

Jeste, sada bi bilo teško naći taj twit ..

Djuki commented 11 years ago

Pazi parafraziram: Zašto li kog đavola programeri ne koriste ovakvu sintaksu ? i onda taj kod...

ldusan84 commented 11 years ago

Pa za taj kod recimo kad bi napisao u NetBeansu izaslo bi ti upozorenje :D