[PHP]Sprawdzenie klasy Opcje

[minolone]

Witam.
Prosze was o sprawdzenie klasy oraz wyrzucenie mi wszystkich błędów co jest nie tak jak być powinno.
Jest nie skończona ale wszytsko działa tak jak chciałem.

[PHP] pobierz, plaintext 
<?php
 
    print_r($_POST);
 
    class AirConditioning {
 
        const HOME = 20; 
        const HOME_ROOF = 0.2;
        const COMPANY = 10;
        const COMPANY_PERCENT = 0.1;
        const COMPANY_ROOF = 0.1;
        const PEOPLE = 0.1;
 
        private $people;
        //public $window;
        private $roof;
 
        public function __construct($room, $height, $area, $people, $window, $roof) {
            $this->room = $room;
            $this->height = $height;
            $this->area = $area;
            if(!empty($window)) $this->window = $window;
            if(!empty($people)) $this->people = $people;
            if(!empty($roof)) $this->roof = $roof;
        }
 
        public function check() {
            if($this->room == 0) {
                return $this->home($this->height, $this->area);
            }
            if($this->room == 1) {
                return $this->company($this->height, $this->area);
            }
        }
 
        public function home($height, $area) {
            $count = $height*$area/self::HOME;
 
            if($this->roof == 1) {
                $roof = self::HOME_ROOF;
            }
 
            if($this->people > 0) {
                $people = $this->people($this->people);   
            }
 
            if(isset($roof) AND $roof > 0) {
                $count = $count+$count*$roof;
            }
 
            if(isset($people) AND $people > 0) {
                $count = $count+$people;
            }
 
            return $count;
        }
 
        public function company($height, $area) {
            $count = $height*$area;
            $count = $count/self::COMPANY;
            $add = self::COMPANY_PERCENT;
 
            if($this->roof == 1) {
                $add += self::COMPANY_ROOF;
            }
 
            if($this->people > 0) {
                $people = $this->people($this->people);   
            }
 
            if(isset($roof) AND $roof > 0) {
                $count = $count+$count*$roof;
            }
 
            if(isset($people) AND $people > 0) {
                $count = $count+$people;
            }
 
            return $count;
        }
 
        public function people($people) {
            $people = $people*self::PEOPLE;
            return $people;
        }
 
    } 
 
?>
[PHP] pobierz, plaintext 

[rocktech.pl]

Witam.

Trochę szczegółów, komentarze, przykłady użycia poprosimy.

Na ten moment PHP_CodeSniffer (IMG:style_emoticons/default/smile.gif)

[BASH] pobierz, plaintext 
phpcs AirConditioning.class.php 
 
FILE: AirConditioning.class.php
--------------------------------------------------------------------------------
FOUND 25 ERROR(S) AND 3 WARNING(S) AFFECTING 22 LINE(S)
--------------------------------------------------------------------------------
  2 | ERROR   | Missing file doc comment
  5 | ERROR   | Missing class doc comment
  5 | ERROR   | Opening brace of a class must be on the line after the
    |         | definition
 14 | ERROR   | Private member variable "people" must be prefixed with an
    |         | underscore
 16 | ERROR   | Private member variable "roof" must be prefixed with an
    |         | underscore
 18 | ERROR   | You must use "/**" style comments for a function comment
 18 | ERROR   | Opening brace should be on a new line
 22 | WARNING | Inline control structures are discouraged
 23 | WARNING | Inline control structures are discouraged
 24 | WARNING | Inline control structures are discouraged
 27 | ERROR   | Missing function doc comment
 27 | ERROR   | Opening brace should be on a new line
 28 | ERROR   | Expected "if (...) {\n"; found "if(...) {\n"
 31 | ERROR   | Expected "if (...) {\n"; found "if(...) {\n"
 36 | ERROR   | Missing function doc comment
 36 | ERROR   | Opening brace should be on a new line
 39 | ERROR   | Expected "if (...) {\n"; found "if(...) {\n"
 43 | ERROR   | Expected "if (...) {\n"; found "if(...) {\n"
 47 | ERROR   | Expected "if (...) {\n"; found "if(...) {\n"
 51 | ERROR   | Expected "if (...) {\n"; found "if(...) {\n"
 58 | ERROR   | Missing function doc comment
 58 | ERROR   | Opening brace should be on a new line
 63 | ERROR   | Expected "if (...) {\n"; found "if(...) {\n"
 67 | ERROR   | Expected "if (...) {\n"; found "if(...) {\n"
 71 | ERROR   | Expected "if (...) {\n"; found "if(...) {\n"
 75 | ERROR   | Expected "if (...) {\n"; found "if(...) {\n"
 82 | ERROR   | Missing function doc comment
 82 | ERROR   | Opening brace should be on a new line
--------------------------------------------------------------------------------
[BASH] pobierz, plaintext 

Ten post edytował rocktech.pl 11.01.2012, 15:10:23

[minolone]

~rocktech.pl, myśle że "doc comment" sobie odpuścimy
proszę o sprawdzenie kodu czy jest to napisane w dość dobry sposób
pozdrawiam

[PHP] pobierz, plaintext 
<?php
 
class AirConditioning
{
 
    const HOME = 20;
    const HOME_ROOF = 0.2;
    const COMPANY = 10;
    const COMPANY_PERCENT = 0.1;
    const COMPANY_ROOF = 0.1;
    const PEOPLE = 0.1;
 
    public $people;
    public $roof;
 
    public function __construct ($room, $height, $area, $people, $window, $roof)
    {
        $this->room = $room;
        $this->height = $height;
        $this->area = $area;
 
        if (! empty($window)) {
            $this->window = $window;
        }
 
        if (! empty($people)) {
            $this->people = $people;
        }
 
        if (! empty($roof)) {
            $this->roof = $roof;
        }
    }
 
    public function check ()
    {
        if ($this->room == 0) {
            return $this->home($this->height, $this->area);
        }
 
        if ($this->room == 1) {
            return $this->company($this->height, $this->area);
        }
    }
 
    public function home ($height, $area)
    {
        $count = $height * $area / self::HOME;
 
        if ($this->roof == 1) {
            $roof = self::HOME_ROOF;
        }
 
        if ($this->people > 0) {
            $people = $this->people($this->people);
        }
 
        if (isset($roof) and $roof > 0) {
            $count = $count + $count * $roof;
        }
 
        if (isset($people) and $people > 0) {
            $count = $count + $people;
        }
 
        return $count;
    }
 
    public function company ($height, $area)
    {
        $count = $height * $area;
        $count = $count / self::COMPANY;
        $add = self::COMPANY_PERCENT;
 
        if ($this->roof == 1) {
            $add += self::COMPANY_ROOF;
        }
 
        if ($this->people > 0) {
            $people = $this->people($this->people);
        }
 
        if (isset($roof) and $roof > 0) {
            $count = $count + $count * $roof;
        }
 
        if (isset($people) and $people > 0) {
            $count = $count + $people;
        }
 
        return $count;
    }
 
    public function people ($people)
    {
        $people = $people * self::PEOPLE;
        return $people;
    }
 
}
 
?>
[PHP] pobierz, plaintext 

[rocktech.pl]

Hej właśnie komentarz chciałbym zobaczyć. Na pierwszy rzut oka domyślam się, że klasa ma klimatyzować ...
Znacznie łatwiej byłoby się połapać potencjalnemu oceniającemu gdybyś zamieścił opis, przykłady użycia oraz komentarze, które odzwierciedlają tok myślenia autora i same w sobie mogą stanowić przedmiot oceny.

Pozdrawiam, jak ty się przyłożysz to inni też (IMG:style_emoticons/default/smile.gif)

[minolone]

Proszę o sprawdzenie poprawności kodu oraz ew. poprawki,
Napewno jakieś błędy się znajdą ale proszę o wyrozumiałość.
Ogólnie wszystko działa tak jak powinno.

[PHP] pobierz, plaintext 
<?php
/**
 * AirConditioning
 * Klasa odpowiedzialna za wyliczanie mocy klimatyzatora
 */
class AirConditioning {
    const HOME = 20;                //Dzielenie wyniku m3 dla pomieszczeń mieszkalnych
    const HOME_ROOF = 0.2;          //Dodatek 20% dla okien dachowych, mieszkalnych
    const COMPANY = 10;             //Dzielenie wyniku m3 dla pomieszczeń handlowych
    const COMPANY_PERCENT = 0.1;    //Dodatek 10% dla pomieszczeń handlowych
    const COMPANY_ROOF = 0.1;       //Dodatek 10% dla okien dachowych, handlowych
    const PEOPLE = 0.1;             //Dodatek 0.1 kW za każdego człowieka
    const COMPANY_SOUTH = 0.05;     //Dodatek 5% za okna skierowane na południe, pomieszczenia handlowe
    const COMPANY_WEST = 0.05;      //Dodatek 5% za okna skierowane na zachód, pomieszczenia handlowe
    const HOME_SOUTH = 0.1;         //Dodatek 10% za okna skierowane na południe, pomieszczenia mieszkalne
    const HOME_WEST = 0.1;          //Dodatek 10% za okna skierowane na zachód, pomieszczenia mieszkalne
 
    public $people;
    public $roof;
    public $south;
    public $west;
 
    /**
     * AirConditioning::__construct()
     * Pobranie danych z formularza
     * @param mixed $room informacje o pomieszczeniu
     * @param mixed $height wysokość pomieszzenia
     * @param mixed $area powierzchnia pomieszczenia
     * @param mixed $people informacje o osobach w pomieszczeniu 
     * @param mixed $roof okna dachowe
     * @param mixed $south kierunek okien, południe
     * @param mixed $west kierunek okien, zachód
     */
    public function __construct($room, $height, $area, $people, $roof, $south, $west)
    {
        $this->room = $room;
        $this->height = $height;
        $this->area = $area;
 
        if (!empty($north)) {
            $this->north = $north;
        }
 
        if (!empty($south)) {
            $this->south = $south;
        }
 
        if (!empty($east)) {
            $this->east = $east;
        }
 
        if (!empty($west)) {
            $this->west = $west;
        }
 
        if (!empty($people)) {
            $this->people = $people;
        }
 
        if (!empty($roof)) {
            $this->roof = $roof;
        }
    }
 
    /**
     * AirConditioning::check()
     * Pobranie danych o pomieszczeniu
     */
    public function check() 
    {
        if ($this->room == 1) {
            return $this->home($this->height, $this->area);
        }
 
        if ($this->room == 2) {
            return $this->company($this->height, $this->area);
        }
    }
 
    /**
     * AirConditioning::home()
     * Obliczanie mocy klimatyzatora dla pomieszczeń mieszkalnych
     */
    public function home($height, $area) 
    {
        $count = $height * $area / self::HOME;
        $add = '';
 
        if ($this->roof == 1) {
            $add += self::HOME_ROOF;
        }
 
        if ($this->people > 0) {
            $people = $this->people($this->people);
        }
 
        if ($this->west == 1) {
            $add += self::HOME_WEST;
        }
 
        if ($this->south == 1) {
            $add += self::HOME_SOUTH;
        }
 
        $count = $count + $count * $add;
 
        if (isset($people) and $people > 0) {
            $count = $count + $people;
        }
 
        return $count;
    }
 
    /**
     * AirConditioning::company()
     * Obliczanie mocy klimatyzatora dla pomieszczeń handlowych
     */
    public function company($height, $area) 
    {
        $count = $height * $area;
        $count = $count / self::COMPANY;
        $add = self::COMPANY_PERCENT;
 
        if ($this->west == 1) {
            $add += self::COMPANY_WEST;
        }
 
        if ($this->south == 1) {
            $add += self::COMPANY_SOUTH;
        }
 
        if ($this->roof == 1) {
            $add += self::COMPANY_ROOF;
        }
 
        if ($this->people > 0) {
            $people = $this->people($this->people);
        }
 
        $count = $count + $count * $add;
 
        if (isset($people) and $people > 0) {
            $count = $count + $people;
        }
 
        return $count;
    }
 
    /**
     * AirConditioning::people()
     * Wylicza moc dla każdej osoby w pomieszczeniu
     */
    public function people($people) 
    {
        $people = $this->people * self::PEOPLE;
 
        return $people;
    }
 
}
 
?>
[PHP] pobierz, plaintext 

Przykład użycia

[PHP] pobierz, plaintext 
<?php
$ob = new AirConditioning($room, $height, $area, $people, $roof, $north, $south, $east, $west);
echo 'Wyliczone zapotrzebowanie na moc klimatyzatora wynosi '. $ob->check(). ' kW.';
?>
[PHP] pobierz, plaintext 

[wookieb]

Nie pokazuj raportu ze Sniffera bo każdy ma inne standardy kodowania. Mnie np nie odpowiada ani Zend-owy ani Symfony2 więc jak?

[minolone]

~wookieb, ogólnie chodzi mi o sprawdzenie czy jest to jakoś tak w miare napisane, oraz ew. co powinienem zmienić, poprawić przy pisaniu.
Ogólnie to co chciałem uzyskać jest zrobione, czyli wylicza mi moc klimatyzatora tak jak powinno.

Ten post edytował minolone 17.02.2012, 14:26:10

[rocktech.pl]

Proponuję PHPUnit.

[minolone]

~rocktech.pl, jakoś nie możemy się dogadać, ja nie potrzebuje robić jakiś testów na tej klasie, tylko dowiedzieć się o tym jak jest napisana i co ew zmienić, poprawić. Jest to do użytku wewnętrznego, testami bede się zajmował w innym czasie.

[rocktech.pl]

Hej moja wina przeczytałem

czy wylicza mi moć klimatyzatora tak jak powinno.

Co do klasy ta metoda i własność i stała o tej samej nazwie są trochę mylące.

[PHP] pobierz, plaintext 
 
public function people($people) {
 
            $people = $people*self::PEOPLE;
 
            return $people;
 
        }
[PHP] pobierz, plaintext 

[PHP] pobierz, plaintext 
       if ($this->people > 0) {
 
            $people = $this->people($this->people);
 
        }
[PHP] pobierz, plaintext 

Dalej co do parametrów raczej boolean zamiast mixed?

[PHP] pobierz, plaintext 
     * @param mixed $south kierunek okien, południe
 
     * @param mixed $west kierunek okien, zachód
.....
 
        //i to sparwdzanie  w konstruktorze bez sensu od razu przypisz wartość 
        if (!empty($south)) {
 
            $this->south = (bool) $south;
 
        }
[PHP] pobierz, plaintext 

Potem w metodzie home

[PHP] pobierz, plaintext 
($this->south) AND $add += self::HOME_SOUTH;
//a nie
if($this->south == 1) {
     $add += self::HOME_SOUTH; 
}
[PHP] pobierz, plaintext 

[Crozin]

1. Powinieneś starać się by nazwa klasy była rzeczownikiem, a nazwy metod czasownikami - kod staje się wtedy znacznie bardziej "naturalny". W chwili obecnej Twój kod (przykładowo: $ac->people(), "klimatyzacja -> ludzie") nie ma żadnego sensu (z puntu widzenia normalnego czytania).
2. Jesteś pewien, że te wszystkie dodatki powinny być stałymi? Jestem niemal pewien, że cześć z tych wartości mogłaby się w przyszłości zmienić.
3. Nie jestem pewien co do sposobu użycia tego kodu, ale wydaje się, że jedynie konstruktor i metoda check() powinny być publiczne.
4. Używasz niezadeklarowanych właściwości obiektu (patrz: north, east).
5. Powinieneś dodać jakieś sprawdzanie poprawności przekazywanych argumentów, np. w konstruktorze (szczególnie gdy w dokumentacji podajesz mixed jako typ argumentu).
6. Nigdy nie używaj jakiś "magicznych" numerków w kodzie (patrz: if ($this->room == 1)). Zamiast nich użyj stałych, znacznie poprawia to cztelność kodu i daje możliwość jego wygodnej dokumentacji.
7. Właściwość room może przyjąć dowolną wartość, a tymczasem Ty zakładasz jedynie pojawienie się wartości 1 lub 2. Powinieneś wyrzucić jakiś wyjątek, gdy próbuje się podać inną.
8. Mimo iż PHP pozwala na zadeklarowanie i zdefiniowanie zmiennej w bloku przykładowo IF-a i późniejsze użycie tej zmiennej poza tym blokiem:

[PHP] pobierz, plaintext 
public function home(...) {
   ...
 
   if (...) {
      $people = ...;
   }
 
   echo isset($people) ? $people : 'n/a';
}
[PHP] pobierz, plaintext 

Nigdy nie powinieneś tego robić. Zdefiniuj wcześniej tą zmienną:

[PHP] pobierz, plaintext 
public function home(...) {
   $people = null;
   ...
 
   if (...) {
      $people = ...;
   }
 
   echo !is_null($people) ? $people : 'n/a';
}
[PHP] pobierz, plaintext 

9. Właściwości north, south, west i east powinny być chyba typu boolean?
10. Metody home() i company() (nieodpowiednie nazwy swoją drogą) wyglądają mi na dokładnie takie same z tą różnicą, że jedna operuje na stałych HOEM_*, a druga na COMPANY_*, racja? Bezproblemu zrobiłbyś z tego jedną metodę z trzecim argumentem przyjmującym obiekt/tablicę (typu wyliczeniowego w PHP niestety nie ma) z tymi stałymi.

Generalnie - do całościowego przepisania.

[minolone]

~Crozin, super że zainteresowałeś się moją klasą i dostałem tak spore informacje.
Ad.4 - zapomniałem usunąć z kodu,
Ad.2 - Raczej zmieniać się nie będą, na obecną chwilę oczywiście, może w przyszłości gdy to rozbuduje, narazie chciałem coś napisać i dostać wskazówki co robić a czego nie.
Ad.5 - Dokumentacja została wygenerowana i po prostu nie pozmieniałem, mój błąd.
Ad.7 - Wartości mogą być tylko 1 lub 2, pobierane z checkbox-a


Nie zostaje mi nic innego jak przepisać z twoimi wskazówkami.
Pozdrawiam, Dzięki

[Crozin]

Ad.2 - Raczej zmieniać się nie będą, na obecną chwilę oczywiście, może w przyszłości gdy to rozbuduje, narazie chciałem coś napisać i dostać wskazówki co robić a czego nie.

Nawet nie chodzi o to czy się zmienią teraz czy później. Takie rzeczy z natury nie są stałymi, w dodatku nie odnosisz tutaj żadnych korzyści z ich użycia. Pewnego dnia te wartości zmienią się i będzie problem. Problem wynikający z błędnego zaplanowania. Coś jak w przypadku VAT-u, gdzie w wielu aplikacjach ludzie myśleli, że 22% to stała. (IMG:style_emoticons/default/wink.gif)
Zamień to więc na normalną właściowść, a najlepiej na osobny obiekt (zbiór tych wartości to jedna właściwość, ale sama w sobie złożna z kilku innych; przy okazji punkt #10 będzie wtedy śmiesznie prosty do rozwiązania, a interfejs konstruktra znacznie się prości).

Ad.7 - Wartości mogą być tylko 1 lub 2, pobierane z checkbox-a

[PHP] pobierz, plaintext 
$ac = new AirConditioning(6, ...);
$result = $ac->check();
[PHP] pobierz, plaintext 

Skoro mogą być tylko dwie to taki kod powinien wyrzucić wyjątek, a tego nie zrobi.