Początki z OOP, Krytyka, komentarze, pomoc w napisaniu lepszego kodu :) Opcje

[Wicepsik]

Witam,
Zaczynam pisać w OOP, pomaga mi w tym kolega doradzając conieco. Chciałbym byście pomogli mi w pisaniu lepszego kodu. Przestawię wam moje wypociny.

[PHP] pobierz, plaintext 
<?php
class pogoda{
 
    public $web;
    public $cities;
    public $temp;
    public $hpa;
    
    public function __construct($city){
 /*
Wpisane wszystkie miasta które są dostępne w polsce
*/
     $this->cities = array('Aleksandrów Kujawski' => 11642, 'Augustów' => 11643, 'Bartoszyce' => 11644, 'Bełchatów' => 11646, 'Będzin' => 11645, 'Biała Podlaska' => 11647, 'Białka Tatrzańska' => 11648, 'Białobrzegi' => 11649, 'Białogard' => 11650, 'Białystok' => 11651, 'Bielice' => 11652, 'Bielsk Podlaski' => 11653, 'Bielsko-Biała' => 11654); 
 
     
     
        if(array_key_exists($city, $this->cities)){
            $this->web = file_get_contents('http://pogoda.interia.pl/miasta?id='.$this->cities[$city]);
            preg_match_all('/<b>([0-9]+)</b>/<span class="tex2B"  style="font-size:14px;">([0-9]+)</span>/<span class="tex3B">([0-9]+)</span>/', $this->web, $this->temp);
            preg_match_all('/<b>([0-9]+)</b> hPa/', $this->web, $this->hpa);
        }else{
            echo 'Brak miasta!';
        }
        
    }
    
    public function show_cities(){
            $text = '<select>';
        foreach($this->cities as $key => $value){
            $text .= '<option value="'.$value.'">'.$key.'</option>';
        }
            $text .= '</select>';
            
        return $text;
    }
    
    public function today_temp($mi = 0){
              if($mi = 0){
                  $text = $this->temp[1][0];
              }else{
                $text = $this->temp[3][0];
              }
         return $text;
    }
    
    public function temp($mi = 0, $li = 0, $day = 0){
              
            $on = ($mi == 0) ? 1 : 3;
            
            switch($day){
                case 0: if($li == 0) $of = 2; elseif($li == 1) $of = 3; else($li == 2) $of = 4; // przed południem, po południu, wieczorem
                          break;
                case 1: if($li == 0) $of = 5; elseif($li == 1) $of = 6; elseif($li == 2) $of = 7; else $of = 8; // nad ranem, przed południem, po południu, wieczorem
                          break;
                case 2: if($li == 0) $of = 9; elseif($li == 1) $of = 10; elseif($li == 2) $of = 11;  else $of = 12; // nad ranem, przed południem, po południu, wieczorem
                          break;
                case 3: $of = ($li == 0) ? 13 : 14;  // dzien, noc
                          break;
                case 4: $of = ($li == 0) ? 15 : 16;   // dzien, noc
                          break;
                case 5: $of = ($li == 0) ? 17 : 18;   // dzien, noc
                          break;
            }
            
         return $this->temp[$on][$of];
    }
    
    public function hPa($mi = 0){
    
        return $this->hpa[1][$mi];
    
    }
    
    
    public function __destruct() {
        unset($this);
    }
    
    
    
    
}
 
 
    $obiekt = new pogoda('Warszawa');
    $obiekt->show_cities();
    echo $obiekt->temp(1,1,1);
    echo $obiekt->hPa(2);
?>
[PHP] pobierz, plaintext 

Ten post edytował Wicepsik 19.07.2009, 19:05:10

[erix]

[PHP] pobierz, plaintext 
<?php
public function __construct($city){
/*
Wpisane wszystkie miasta które są dostępne w polsce
*/
    $this->cities = array('Aleksandrów Kujawski' => 11642, 'Augustów' => 11643, 'Bartoszyce' => 11644, 'Bełchatów' => 11646, 'Będzin' => 11645, 'Biała Podlaska' => 11647, 'Białka Tatrzańska' => 11648, 'Białobrzegi' => 11649, 'Białogard' => 11650, 'Białystok' => 11651, 'Bielice' => 11652, 'Bielsk Podlaski' => 11653, 'Bielsko-Biała' => 11654);
?>
[PHP] pobierz, plaintext 

Przecież te zmienne możesz zadeklarować bezpośrednio przy definicji zmiennej, po co w konstruktorze?

[PHP] pobierz, plaintext 
<?php
public function show_cities(){
           $text = '<select>';
       foreach($this->cities as $key => $value){
           $text .= '<option value="'.$value.'">'.$key.'</option>';
       }
           $text .= '</select>';
           
       return $text;
   }
?>
[PHP] pobierz, plaintext 

A czemu mieszasz logikę z wyglądem? MVC!

[PHP] pobierz, plaintext 
<?php
public function temp($mi = 0, $li = 0, $day = 0){
             
           $on = ($mi == 0) ? 1 : 3;
?>
[PHP] pobierz, plaintext 

No ok, ale co mają powiedzieć te zmienne? MI/LI? Coś z listą wyliczeniową? Będziesz w stanie zrozumieć to za miesiąc, gdy "odpoczniesz" od kodu? PS. Zainteresuj się składnią phpDoc.

[PHP] pobierz, plaintext 
<?php
public function __destruct() {
       unset($this);
   }
?>
[PHP] pobierz, plaintext 

A to już totalny bezsens. (IMG:http://forum.php.pl/style_emoticons/default/tongue.gif)

[Fifi209]

[PHP] pobierz, plaintext 
<?php
public function __construct($city){
/*
Wpisane wszystkie miasta które są dostępne w polsce
*/
    $this->cities = array('Aleksandrów Kujawski' => 11642, 'Augustów' => 11643, 'Bartoszyce' => 11644, 'Bełchatów' => 11646, 'Będzin' => 11645, 'Biała Podlaska' => 11647, 'Białka Tatrzańska' => 11648, 'Białobrzegi' => 11649, 'Białogard' => 11650, 'Białystok' => 11651, 'Bielice' => 11652, 'Bielsk Podlaski' => 11653, 'Bielsko-Biała' => 11654);
?>
[PHP] pobierz, plaintext 

Przecież te zmienne możesz zadeklarować bezpośrednio przy definicji zmiennej, po co w konstruktorze?

Można tak, można tak - kwestia przyzwyczajenia, jednak wersja z konstruktorem wydaje się być lepsza. ;d

[PHP] pobierz, plaintext 
<?php
public function show_cities(){
           $text = '<select>';
       foreach($this->cities as $key => $value){
           $text .= '<option value="'.$value.'">'.$key.'</option>';
       }
           $text .= '</select>';
           
       return $text;
   }
?>
[PHP] pobierz, plaintext 

A czemu mieszasz logikę z wyglądem? MVC!

Wcześniej były echa, ja mu doradziłem aby zmienił na return.

[PHP] pobierz, plaintext 
<?php
public function temp($mi = 0, $li = 0, $day = 0){
             
           $on = ($mi == 0) ? 1 : 3;
?>
[PHP] pobierz, plaintext 

No ok, ale co mają powiedzieć te zmienne? MI/LI? Coś z listą wyliczeniową? Będziesz w stanie zrozumieć to za miesiąc, gdy "odpoczniesz" od kodu? PS. Zainteresuj się składnią phpDoc.

Zgadzam się. (IMG:http://forum.php.pl/style_emoticons/default/winksmiley.jpg)

[PHP] pobierz, plaintext 
<?php
public function __destruct() {
       unset($this);
   }
?>
[PHP] pobierz, plaintext 

A to już totalny bezsens. (IMG:http://forum.php.pl/style_emoticons/default/tongue.gif)

Zwolnienie pamięci uważasz za bezsens? Gdyby to był większy system, też byłby to bezsens? (IMG:http://forum.php.pl/style_emoticons/default/biggrin.gif)
To tak jak np. z zamykaniem połączenia mysql, jak nie użyjesz mysql_close() skrypt i tak będzie działał.

Ten post edytował fifi209 19.07.2009, 22:36:59