jako, że nie jestem "PRO" w PHP chciałbym swoje ewentualne błędy korygować na bieżąco, czy mógłby ktoś rzucić okiem na mój kod i go skrytykować conieco?

[PHP] pobierz, plaintext 
<?php
class Domains {
    private $_total             = 0;    // store total number of domains
    private $_good              = 0;    // store number of domains in good standing
    private $_expiring          = 0;    // store number of expiring domains
    private $_expired           = 0;    // store number of expired domains
    private $_domains           = array();
 
    public function __construct() {
        $this->_domains=$this->fetchAll();
    }
    // returns a number of domains
    public function numTotal() {
        return $this->_total;
    }
    // returns number of good domains
    public function numGood() {
        return $this->_good;
    }
    // returns number of expiring domains
    public function numExpiring() {
        return $this->_expiring;
    }
    // returns number of expired domains
    public function numExpired() {
        return $this->_expired;
    }
    // validates domain data
    public function validate($domain) {
        $_result    =   true;
        // perform check here
        return $_result;
    }
    // get a domain details for domain with given id
    public function get($id) {
        // basically we are going through the whole list and if we find a match we return the content
        foreach($this->_domains as $domain) {
            if ($domain['dom_id']==$id)
            {
                $_result=$domain;
                return $_result;
            }
        }
        // in case we won't find the match let's indicate it
        $_result=-1;
        return $_result;
    }
    // delete domain
    public function delete($id) {
        $_result    = false;
        try {
            $conn=DB::getConnection();
            $query=$conn->prepare("DELETE FROM `domains`
                                    WHERE dom_id=:dom_id");
            $query->execute(array('dom_id'          => $id));
            $_result = true;
        }
        catch (PDOException $e)
        {
            trigger_error('Domains: error deleting domain');
            $_result = false;
        }
        // let's reload just in case to reflect changes
        $this->_domains = $this->fetchAll();
        return $_result;
    }
    // update database with $domain, all required info should be there STUB
    public function update($domain) {
        $_result    = false;
        try {
            $conn=DB::getConnection();
            $query=$conn->prepare("UPDATE `domains`
                                    SET dom_name=:dom_name, dom_reg_id=:dom_reg_id, dom_reg_date=:dom_reg_date,
                                        dom_exp_date=:dom_exp_date, dom_comment=:dom_comment, dom_status=0
                                    WHERE dom_id=:dom_id");
            $query->execute(array('dom_id'          => $domain['dom_id'],
                                    'dom_name'      => $domain['dom_name'],
                                    'dom_reg_id'    => $domain['dom_reg_id'],
                                    'dom_reg_date'  => $domain['dom_reg_date'],
                                    'dom_exp_date'  => $domain['dom_exp_date'],
                                    'dom_comment'   => $domain['dom_comment']
            ));
            $_result = true;
        }
        catch (PDOException $e)
        {
            trigger_error('Domains: error updating domain');
            $_result = false;
        }
        // let's reload just in case to reflect changes
        $this->_domains = $this->fetchAll();
        return $_result;
    }
    // insert a domain into database STUB
    public function insert($domain) {
        $_result    = false;
        // check if this is a valid domain
        if ($this->validate($domain)) {
            // perform insert here
            try {
                $conn=DB::getConnection();
                $query=$conn->prepare("INSERT INTO `domains` (dom_name, dom_reg_id, dom_reg_date, dom_exp_date, dom_comment)
                                        VALUES (:dom_name, :dom_reg_id, :dom_reg_date,
                                            :dom_exp_date, :dom_comment)");
                $query->execute(array(  'dom_name'      => $domain['dom_name'],
                                        'dom_reg_id'    => (int)$domain['dom_reg_id'],
                                        'dom_reg_date'  => $domain['dom_reg_date'],
                                        'dom_exp_date'  => $domain['dom_exp_date'],
                                        'dom_comment'   => $domain['dom_comment'],
                ));
                $_result = true;
            }
            catch (PDOException $e)
            {
                trigger_error('Domains: error inserting domain'+$e);
                $_result = false;
            }            
        } else
        {
            trigger_error('Domains: trying to insert invalid domain data.');
        }
        // let's reload just in case to reflect changes
        $this->_domains = $this->fetchAll();
        return $_result;
    }
    // fetch all domains from database
    public function fetch() {
        try {
            $this->_total=0;
            $this->_expired=0;
            $this->_expiring=0;
            $this->_good=0;
            // open connection and create query
            $conn=DB::getConnection();
            $today = date("Y-m-d");
            // get all domains and calculate how many days left based on current date and exp_date
            $query=$conn->prepare("SELECT dom_id, dom_name, dom_reg_id, dom_reg_date, dom_exp_date,
                                            dom_comment, dom_status, TIMESTAMPDIFF(DAY, :today,
                                            domains.dom_exp_date) as dom_days_left
                                            FROM `domains`, `registrars`
                                            WHERE dom_reg_id=reg_id
                                            ORDER BY dom_days_left");
            $query->execute(array('today' => $today));
            $result = $query->fetchAll();
            $data = '';
            foreach ($result as $row) {
                $this->_total++;
                if ($row['dom_days_left']>30) {
                    $row['Status']=DOMAIN_OK;
                    $this->_good++;
                }
                elseif ($row['dom_days_left']>1) {
                    $row['Status']=DOMAIN_EXPIRING;
                    $this->_expiring++;
                }
                else {
                    $row['Status']=DOMAIN_EXPIRED;
                    $this->_expired++;
                }
                $data[]=$row;
            }
        }
        catch (PDOException $e)
        {
            // show error message upon PDO error
            trigger_error('Domains: error fetching domains table: '.$e);
            return false;
        }
        $this->_domains=$data;
        return $data;
    }
    // fetch an array of good domains
    public function fetchAll() {
        return $this->fetch();
    }
    // fetch an array of good domains
    public function fetchGood() {
        // browse all domains
        foreach ($this->_domains as $row) {
            if ($row['dom_days_left']>30)
                // add domains with more than 30 days left to result
                $data[]=$row;
        }
        return $data;
    }
    // fetch an array of good domains
    public function fetchExpiring() {
        // browse all domains
        foreach ($this->_domains as $row) {
            // add domains between 1 and 30 days to result
            if ($row['dom_days_left']>1 && $row['dom_days_left']<=30)
                $data[]=$row;
        }
        return $data;
    }
    // fetch an array of good domains
    public function fetchExpired() {
        // browse all domains
        foreach ($this->_domains as $row) {
            // add ones below one day to the result
            if ($row['dom_days_left']<=1)
                $data[]=$row;
        }
        return $data;
    }
}
[PHP] pobierz, plaintext 

ewentualnie na gist: https://gist.github.com/mprz/6cce0e6dd51e4796209c

DOMAIN_OK, DOMAIN_EXPIRING, gdzie ty to deklarujesz?
Trigger_error <- co to za cudo z średniowiecza?
Czemu każda metoda wywołuje DB::getConnection(); ? Daj to w konstruktorze.
Private $_expired i już wiem że nici z wygodnego dziedziczenia.

deklaracje są w config.inc.php, ale już załapałem że lepiej będzie trzymać w klasie
trigger_error jest tam w tylko w fazie 'development', później zmienię na logowanie błędów do pliku
co do prywatnych zmiennych - przecież do wszystkich są gettery, jeśli nie planuję rozszerzalności klasy nie mógłbym ich zostawić jako private? znam różnicę między private i protected

edit: nowa wersja, zmienione private->protected, dodane stałe, jedna zmienna dla połączenia PDO
https://gist.github.com/mprz/6cce0e6dd51e4796209c

Czemu nazwy składowych zaczynają się od znaku _ ? Czy to nie jest naleciałość po php4 gdzie nie było jeszcze private ? Czy nadal jest to potrzebne ?Nie wiem czy to błąd, czy nie. Po prostu pytam.

Masz sporo komentarzy. Według Wujka Boba, komentarz to "zło konieczne" gdy kod nie opisuje sam siebie. Czy więc zamiast metody o nazwie delete, nie lepiej nazwać ja deleteDomian ?
Podobnie metoda numExpiring() z komentarzem "returns number of expiring domains", mogła by się obejść bez komentarza, jeśli nazwalibyśmy ją getNumExpiringDomains()

dzięki za rady!

- podkreślenie służy mi temu, że od razu wiem, która zmienna jest public a która private/protected. podejrzałem to gdzieś i zdaje się mi służyć
- naczytałem się troche o konwencji nazw dla PHP (i OOD wogóle) i pamiętam, że duży nacisk położony był na nie dołączanie do nazw metod nazwy klasy. czyli:

[PHP] pobierz, plaintext 
$domain = new Domain();
$zmienna=$domain->delete();
[PHP] pobierz, plaintext 

a nie

[PHP] pobierz, plaintext 
$domain = new Domain();
$zmienna=$domain->deleteDomain();
[PHP] pobierz, plaintext 

gdzie druga wersja niepotrzebnie powiela Domain
wydaje mi się to rozsądnie pomyślane, dlatego robię to w ten sposób

Fakt, z tym się zgodzę. Nawet nie spojrzałem na nazwę klasy. W takim razie, komentarz "// delete domain" dla metody delete w klasie domain moim zdaniem jest zbyteczny, bo nie mówi niczego więcej ponad to, co wnioskujemy po nazwie klasy i metody. Być może czepiam się pierdół. Po prostu kod jest (moim zdaniem) na tyle dobry, że czepiam się tego co się da

Według Wujka Boba współczesne IDE są na tyle rozbudowane, że takie podkreślenia lub inne prefixy są zbyteczne.

wow, dzięki to moja pierwsza aplikacja


bardziej chodzi mi o to, że to JA widzę, która zmienna jest/nie jest publiczna

[PHP] pobierz, plaintext 
Czemu nazwy składowych zaczynają się od znaku _ ? Czy to nie jest naleciałość po php4 gdzie nie było jeszcze private ? Czy nadal jest to potrzebne ?Nie wiem czy to błąd, czy nie. Po prostu pytam.
[PHP] pobierz, plaintext 

Tyle razy były już o tym dyskusje. Zaczynanie metod private/protected od _ ma sens i jest nadal bardzo przydatne.

uaktualnienie: kilka funkcji nie sprawdzało czy zwracana wartość została zainicjowana. dodałem parę warunków w kodzie, i zwracane jest -1 jeśli $_result jest unset()

czemu -1? o.O Zwracaj false.

Na szybko klepnąłeś, nie chciało ci się ochłonąć i pomyśleć.
Zamiast tego:

[PHP] pobierz, plaintext 
	// get a domain details for domain with given id
	public function get($id) {
		// basically we are going through the whole list and if we find a match we return the content
		foreach($this->_domains as $domain) {
			if ($domain['dom_id']==$id)
			{
				$_result=$domain;
				return $_result;
			}
		}
		// in case we won't find the match let's indicate it
		$_result=-1;
		return $_result;
	}
[PHP] pobierz, plaintext 

Daj np. to:

[PHP] pobierz, plaintext 
	// get a domain details for domain with given id
	public function get($id) {
		// basically we are going through the whole list and if we find a match we return the content
		foreach($this->_domains as $domain) {
			if ($domain['dom_id']==$id)
			{
				return $domain;
			}
		}
		// in case we won't find the match let's indicate it
		return false;
	}
[PHP] pobierz, plaintext 

Ewentualnie jeśli zależy ci na zachowaniu $return:

[PHP] pobierz, plaintext 
	// get a domain details for domain with given id
	public function get($id) {
		// basically we are going through the whole list and if we find a match we return the content
	   $result = false;
		foreach($this->_domains as $domain) {
			if ($domain['dom_id']==$id)
			{
				$result=$domain;
			}
		}
		return $result;
	}
[PHP] pobierz, plaintext 

Jeśli robisz lokalne zmienne takie jak $result, nie ma sensu dawać _ czyli $_result. Znak _ dajesz dla private/protected.

To źle ci służy. Wszystkie dane( pola ) dobrze zaprojektowanej klasy powinny być prywatne ( enkapsulacja danych ), więć nie ma żadnego powodu by dodawać do nich podkreślink skoro wszystkie są prywatne. Dane chornione czy publiczne to są bardzo specjalne przypadki. Dostęp do danych klasy powinien odbywać się tylko przez metody dostępowe i to one są najwyżej chronione. Interfejs jest publiczny.Podkreślnik przed zmiennymi prywatnymi to zboczenie programistów javy ponieważ tam można odwołać się do pola klasy beż this w jej obrębie i nie widać czy to zmienna lokalna czy klasy na pierwszy rzut oka. Pozatym zmiennych lokalnych nie powinno się używać tylko zapytania. Zastępujemy zmienną tymczasową zapytaniem.

Twoja klasa jest odpowiedzialna za 2 rzeczy czyli o jedną za dużo. Widze tu... póki jeszcze nie zarzuciłem bielunia to widze... obiekt dziedziny domain i utrwalanie go. 2 klasy bym tu minimum widział oczywiscie domain i np domain_table. Warstwa dziedziny i warstwa utrwalania. Domain bedzie delegować do domain_table. domain_table będzie polem klasy domain.

Na pewno musisz zastosować te przekształcenia tak na oko
* Rozdzielenie Zapytania i Modyfikacji
* Zastąpienie odwołania do zmiennej zapytaniem
* Ekstrakcje klasy ( z domain wyciagnij np domain_table )
* Ekstrakcje metody ( podzialkuj dlugie metody )
* Przeniesienie metody ( przenies do domain_table z domain )
Zastosować jakiś wzorzec do mapowania realcyjno-obiektowego
* Brama danych tabeli
* Brama danych wiersza
* Rekord aktywny
* Data mapper to by była w tym wypadku lekka przesada.

Dobrze że używasz instrukcji preparowanych i nazwanych symboli zastępczych ci którzy tak nie robią robią po prostu siare.

Każdy kto nie czytał tych ksiązek jak i welu innych klasyków, a programuje powinien się wstydzić i kajać.
Refaktoryzacja ulepszanie struktury istniejacego kodu
Architektura systemow zarzadzania przedsiebiorstwem wzorce projektowe
Wracam do szukania bielunia obok firmy może coś znajde. Pissss joł.

Osobiście w javie nie spotkałem się z takimi zmiennymi. Metoda teoretycznie powinna być bardzo mała, a współczesne IDE tak kolorują składnie, by wiadomo było, czy zmienna jest składową klasy, czy lokalną w metodzie - dlatego nadal nie widzę powodu, by programiścy javy (i czemu akurat javy? w cpp też nie trzeba używać thisa) używali tego podkreślenia. Pewnie za mało kodu w javie widziałem albo oglądam tylko ten lepszy kod.
Natomiast podkreślenia w zmiennych prywatnych to bardzo częste zachowanie programistów PHP4 gdzie każda zmienna była publiczna (słowo kluczowe var), a dodanie podkreślenia na początku miało sygnalizować - tą zmienną traktuj jak prywatną, bo ona byłaby prywatna gdyby tylko php4 na to pozwoliło.


A czy tutaj nie lepiej skorzystać z wyjątków ? Wiem, że wyjątki w php to takie troche sztuczne są (choćby dlatego, że wyjątkami nie zostały objęte wbudowane funkcje), ale jak już mówimy o czystym kodzie, to wyjątki są po to, żeby ułatwić pisanie czystego kodu.


Wybacz, nie widziałem ani jednego wątku. W książce, którą czytałem używanie podkreślenia było krytykowane. Chętnie poznam inne opinie na ten temat, więc jakimś linkiem nie pogardzę

[PHP] pobierz, plaintext 
    // fetch an array of good domains
    public function fetchExpiring();
    // fetch an array of good domains
    public function fetchExpired();
[PHP] pobierz, plaintext 

takie same komentarze, dla dwóch metod ? Chyba cośtu jest nie tak.

Według Wujka Boba, takie warunki:

powinny być wyexportowane do innej metody. Np

[PHP] pobierz, plaintext 
if($this->domDaysLeftBetween1And30($row))
[PHP] pobierz, plaintext 

Albo

[PHP] pobierz, plaintext 
if($this->isBetween1And30($row[dom_days_left]))
[PHP] pobierz, plaintext 

Nie wiem, który wariant lepszy, ale ten drugi zdaje się nie pasować do klasy o nazwie Domain

wtedy komentarz:

będzie niepotrzebny.

Wadą tego rozwiązania jest oczywiście mniejsza wydajność (pewnie bardzo nieznaczna, ale w javie stosowany jest silny inlining, tutaj raczej nie bardzo.), większe rozbicie kodu (więcej metod) za to kod jest czytelniejszy i potrzebuje mniej komentarzy, które to lubią tracić na aktualności.

Lol, jaka nazwa

[PHP] pobierz, plaintext 
if($this->isBetween($row[dom_days_left], 1, 31))
[PHP] pobierz, plaintext 

@down - nie dziś

Fakt, Twoja lepsza Ale ideę chyba miałem w miarę dobrą.

Czy dasz mi jakieś "materiały" udowadniające słuszność zastosowania prefixu "_" w prywatnych składowych ?

progi 1 i 30 teraz są arbitralne ale będą definiowalne przez użytkownika nieco później (roadmap), więc zawczasu się przygotowałem do tego
ale można pomyśleć nad

[PHP] pobierz, plaintext 
isBetween($row[dom_days_left], $próg1, $próg2)
[PHP] pobierz, plaintext 

Czyli pewnie ani jutro, ani po jutrze

http://forum.php.pl/index.php?s=&showt...t&p=1041806

!*! - Było kiedyś coś ciekawszego.

OK. przeczytałem. Nie mam zamiaru się kłócić, bo widzę, że zdania są podzielone. Do mnie jednak trafiają te argumenty :
1. Dobry kod nie posiada zmiennych publicznych.
2. IDE koloruje zmienne tak, że bez problemu można odróżnić, cy jest to zmienna lokalna czy instancji obiektu.