Witaj Gościu! ( Zaloguj | Rejestruj )

Forum PHP.pl

> Początki z OOP, Krytyka, komentarze, pomoc w napisaniu lepszego kodu :)
Wicepsik
post
Post #1





Grupa: Zarejestrowani
Postów: 1 575
Pomógł: 299
Dołączył: 26.03.2009

Ostrzeżenie: (20%)
X----

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 
  1. <?php
  2. class pogoda{
  3.  
  4.     public $web;
  5.     public $cities;
  6.     public $temp;
  7.     public $hpa;
  8.     
  9.     public function __construct($city){
  10.  /*
  11. Wpisane wszystkie miasta które są dostępne w polsce
  12. */
  13.      $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); 
  14.  
  15.      
  16.      
  17.         if(array_key_exists($city, $this->cities)){
  18.             $this->web = file_get_contents('http://pogoda.interia.pl/miasta?id='.$this->cities[$city]);
  19.             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);
  20.             preg_match_all('/<b>([0-9]+)</b> hPa/', $this->web, $this->hpa);
  21.         }else{
  22.             echo 'Brak miasta!';
  23.         }
  24.         
  25.     }
  26.     
  27.     public function show_cities(){
  28.             $text = '<select>';
  29.         foreach($this->cities as $key => $value){
  30.             $text .= '<option value="'.$value.'">'.$key.'</option>';
  31.         }
  32.             $text .= '</select>';
  33.             
  34.         return $text;
  35.     }
  36.     
  37.     public function today_temp($mi = 0){
  38.               if($mi = 0){
  39.                   $text = $this->temp[1][0];
  40.               }else{
  41.                 $text = $this->temp[3][0];
  42.               }
  43.          return $text;
  44.     }
  45.     
  46.     public function temp($mi = 0, $li = 0, $day = 0){
  47.               
  48.             $on = ($mi == 0) ? 1 : 3;
  49.             
  50.             switch($day){
  51.                 case 0: if($li == 0) $of = 2; elseif($li == 1) $of = 3; else($li == 2) $of = 4; // przed południem, po południu, wieczorem
  52.                           break;
  53.                 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
  54.                           break;
  55.                 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
  56.                           break;
  57.                 case 3: $of = ($li == 0) ? 13 : 14;  // dzien, noc
  58.                           break;
  59.                 case 4: $of = ($li == 0) ? 15 : 16;   // dzien, noc
  60.                           break;
  61.                 case 5: $of = ($li == 0) ? 17 : 18;   // dzien, noc
  62.                           break;
  63.             }
  64.             
  65.          return $this->temp[$on][$of];
  66.     }
  67.     
  68.     public function hPa($mi = 0){
  69.     
  70.         return $this->hpa[1][$mi];
  71.     
  72.     }
  73.     
  74.     
  75.     public function __destruct() {
  76.         unset($this);
  77.     }
  78.     
  79.     
  80.     
  81.     
  82. }
  83.  
  84.  
  85.     $obiekt = new pogoda('Warszawa');
  86.     $obiekt->show_cities();
  87.     echo $obiekt->temp(1,1,1);
  88.     echo $obiekt->hPa(2);
  89. ?>
[PHP] pobierz, plaintext


Ten post edytował Wicepsik 19.07.2009, 19:05:10
Go to the top of the page
+Quote Post
 
 Start new topic
Odpowiedzi (1 - 9)
Fifi209
post
Post #2





Grupa: Zarejestrowani
Postów: 4 655
Pomógł: 556
Dołączył: 17.03.2009
Skąd: Katowice

Ostrzeżenie: (0%)
----- 

[PHP] pobierz, plaintext 
  1. <?php
  2. $obiekt->show_cities();
  3. ?>
[PHP] pobierz, plaintext

raczej 
[PHP] pobierz, plaintext 
  1. <?php
  2. echo  $obiekt->show_cities();
  3. ?>
[PHP] pobierz, plaintext


Ale i tak widzę poprawiłeś dużo, o czym mówiłem. ;d

Dziwna jest ta funkcja temp() (IMG:http://forum.php.pl/style_emoticons/default/haha.gif)
Go to the top of the page
+Quote Post
Pawel_W
post
Post #3





Grupa: Zarejestrowani
Postów: 1 675
Pomógł: 286
Dołączył: 15.06.2009
Skąd: Wieliczka

Ostrzeżenie: (0%)
----- 

[PHP] pobierz, plaintext 
  1. <?php
  2. case 0: if($li == 0) $of = 2; elseif($li == 1) $of = 3; else($li == 2) $of = 4; // przed południem, po południu, wieczorem
  3.                          break;
  4.                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
  5.                          break;
  6.                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
  7. ?>
[PHP] pobierz, plaintext


w każdym case $of jest proporcjonalna do $li, zamień to na
[PHP] pobierz, plaintext 
  1. <?php
  2. case 0: $of = $li+2;                         break;
  3.                case 1: $of = $li+5                         break;
  4.                case 2: $of = $li+9
  5. ?>
[PHP] pobierz, plaintext


i będzie krócej tak jak chciałeś ;]
Go to the top of the page
+Quote Post
Fifi209
post
Post #4





Grupa: Zarejestrowani
Postów: 4 655
Pomógł: 556
Dołączył: 17.03.2009
Skąd: Katowice

Ostrzeżenie: (0%)
-----

Krócej to byłoby zdefiniować stałe w klasie np.

rano, poludnie, wieczor

nadać im wartości i używać w switchu po prostu stałych i tak samo przy wywołaniu metody. (IMG:http://forum.php.pl/style_emoticons/default/haha.gif)
Go to the top of the page
+Quote Post
erix
post
Post #5





Grupa: Moderatorzy
Postów: 15 467
Pomógł: 1451
Dołączył: 25.04.2005
Skąd: Szczebrzeszyn/Rzeszów




 
[PHP] pobierz, plaintext 
  1. <?php
  2. public function __construct($city){
  3. /*
  4. Wpisane wszystkie miasta które są dostępne w polsce
  5. */
  6.     $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);
  7. ?>
[PHP] pobierz, plaintext

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

[PHP] pobierz, plaintext 
  1. <?php
  2. public function show_cities(){
  3.            $text = '<select>';
  4.        foreach($this->cities as $key => $value){
  5.            $text .= '<option value="'.$value.'">'.$key.'</option>';
  6.        }
  7.            $text .= '</select>';
  8.            
  9.        return $text;
  10.    }
  11. ?>
[PHP] pobierz, plaintext

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

[PHP] pobierz, plaintext 
  1. <?php
  2. public function temp($mi = 0, $li = 0, $day = 0){
  3.              
  4.            $on = ($mi == 0) ? 1 : 3;
  5. ?>
[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 
  1. <?php
  2. public function __destruct() {
  3.        unset($this);
  4.    }
  5. ?>
[PHP] pobierz, plaintext

A to już totalny bezsens. (IMG:http://forum.php.pl/style_emoticons/default/tongue.gif)
Go to the top of the page
+Quote Post
Fifi209
post
Post #6





Grupa: Zarejestrowani
Postów: 4 655
Pomógł: 556
Dołączył: 17.03.2009
Skąd: Katowice

Ostrzeżenie: (0%)
-----

Cytat(erix @ 19.07.2009, 22:29:34 ) *
[PHP] pobierz, plaintext 
  1. <?php
  2. public function __construct($city){
  3. /*
  4. Wpisane wszystkie miasta które są dostępne w polsce
  5. */
  6.     $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);
  7. ?>
[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

Cytat(erix @ 19.07.2009, 22:29:34 ) *
[PHP] pobierz, plaintext 
  1. <?php
  2. public function show_cities(){
  3.            $text = '<select>';
  4.        foreach($this->cities as $key => $value){
  5.            $text .= '<option value="'.$value.'">'.$key.'</option>';
  6.        }
  7.            $text .= '</select>';
  8.            
  9.        return $text;
  10.    }
  11. ?>
[PHP] pobierz, plaintext

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

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

Cytat(erix @ 19.07.2009, 22:29:34 ) *
[PHP] pobierz, plaintext 
  1. <?php
  2. public function temp($mi = 0, $li = 0, $day = 0){
  3.              
  4.            $on = ($mi == 0) ? 1 : 3;
  5. ?>
[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)

Cytat(erix @ 19.07.2009, 22:29:34 ) *
[PHP] pobierz, plaintext 
  1. <?php
  2. public function __destruct() {
  3.        unset($this);
  4.    }
  5. ?>
[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
Go to the top of the page
+Quote Post
erix
post
Post #7





Grupa: Moderatorzy
Postów: 15 467
Pomógł: 1451
Dołączył: 25.04.2005
Skąd: Szczebrzeszyn/Rzeszów
Cytat
jednak wersja z konstruktorem wydaje się być lepsza. ;d

Śmiem się sprzeczać. Niby pod jakim względem lepsza? Potrafisz uzasadnić? Masz jakieś argumenty?

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

To nie jest na to miejsce. Widok/helper - owszem. Ale nie logika.

Cytat
Zwolnienie pamięci uważasz za bezsens? Gdyby to był większy system, też byłby to bezsens?

Eee, to jest inna sprawa. Zwalnianie zasobów SQL, to co innego. Poza tym, skrypt przy końcu robi to sam.

Poczytaj, kiedy jest wyzwalany destruktor, to wtedy porozmawiamy, czy jest sens korzystania z unset" title="Zobacz w manualu PHP" target="_manual na $this. [;
Go to the top of the page
+Quote Post
Fifi209
post
Post #8





Grupa: Zarejestrowani
Postów: 4 655
Pomógł: 556
Dołączył: 17.03.2009
Skąd: Katowice

Ostrzeżenie: (0%)
-----

Cytat(erix @ 19.07.2009, 22:42:46 ) *
Śmiem się sprzeczać. Niby pod jakim względem lepsza? Potrafisz uzasadnić? Masz jakieś argumenty?

Jeżeli najdzie mnie myśl pobierania z konstruktora jakiegoś parametru to tylko w konstruktorze podmienię, nie muszę grzebać w reszcie kodu. ;p
Z resztą zazwyczaj ludzie tak piszą, że w klasie tylko definiują zmienne a w konstruktorze nadają im domyślne wartości.

Cytat(erix @ 19.07.2009, 22:42:46 ) *
Poczytaj, kiedy jest wyzwalany destruktor, to wtedy porozmawiamy, czy jest sens korzystania z unset" title="Zobacz w manualu PHP" target="_manual na $this. [;


Podczas niszczenia obiektu. A czy to taki duży błąd...? (IMG:http://forum.php.pl/style_emoticons/default/smile.gif) Nie, ale fakt mogłoby go nie być.
Go to the top of the page
+Quote Post
erix
post
Post #9





Grupa: Moderatorzy
Postów: 15 467
Pomógł: 1451
Dołączył: 25.04.2005
Skąd: Szczebrzeszyn/Rzeszów
Cytat
Z resztą zazwyczaj ludzie tak piszą, że w klasie tylko definiują zmienne a w konstruktorze nadają im domyślne wartości.

Zależy. Naprawdę zależy. Ale skoro to jest statyczna tablica, to dlaczego przy definiowaniu typów zmiennych nie wrzucasz wszystkich do jednego worka (np. dajesz null, null, null) i im typ przypisujesz dopiero w konstruktorze? A co by było, gdybyś chciał przerobić klasę na statyczną? Wtedy masz więcej roboty. (IMG:http://forum.php.pl/style_emoticons/default/tongue.gif)

Cytat
Podczas niszczenia obiektu. A czy to taki duży błąd...? Nie, ale fakt mogłoby go nie być.

BUZI -> http://pl.wikipedia.org/BUZI
Go to the top of the page
+Quote Post
Fifi209
post
Post #10





Grupa: Zarejestrowani
Postów: 4 655
Pomógł: 556
Dołączył: 17.03.2009
Skąd: Katowice

Ostrzeżenie: (0%)
-----

Cytat(erix @ 19.07.2009, 23:21:26 ) *
Zależy. Naprawdę zależy. Ale skoro to jest statyczna tablica, to dlaczego przy definiowaniu typów zmiennych nie wrzucasz wszystkich do jednego worka (np. dajesz null, null, null) i im typ przypisujesz dopiero w konstruktorze? A co by było, gdybyś chciał przerobić klasę na statyczną? Wtedy masz więcej roboty. (IMG:http://forum.php.pl/style_emoticons/default/tongue.gif)

Po prostu wszystko zależy od przeznaczenia klasy. (IMG:http://forum.php.pl/style_emoticons/default/winksmiley.jpg)

Cytat(erix @ 19.07.2009, 23:21:26 ) *
BUZI -> http://pl.wikipedia.org/BUZI

Hahaha, spodobało mi się. Poprawiłeś mi humor.
Go to the top of the page
+Quote Post
« Następny starszy · Object-oriented programming · Następny nowszy »
 

Reply to this topicStart new topic
2 Użytkowników czyta ten temat (2 Gości i 0 Anonimowych użytkowników)
0 Zarejestrowanych:

 

Tryb wyświetlania: Przełącz na: Standardowy · Linearny+ · Przełącz na: Drzewo

Śledź ten temat · Wyślij temat na e-mail · Wydrukuj ten temat · Subskrybuj to forum

RSS Aktualny czas: 23.08.2025 - 00:07
Powered By IP.Board © 2025  IPS, Inc.
All changes by PHP.pl Administrators
Hosting zapewnia NQ.pl hosting, trac, svn