Witaj Gościu! ( Zaloguj | Rejestruj )

Forum PHP.pl

 
Reply to this topicStart new topic
> Czysty kod- czy aby nie przesada?
daniel1302
post 9.10.2016, 10:54:30
Post #1





Grupa: Zarejestrowani
Postów: 602
Pomógł: 30
Dołączył: 1.08.2007
Skąd: Nowy Sącz

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


Witam, ostatnio jestem w trakcie lektury książki R.C Martin'a Czysty kod, i jest tam pełno kodu, który rozbijany jest na jedno dwu liniowe przykłady o dość specyficznych opisowych nazwach. Postanowiłem napisać kawałek kodu wg Jego schematu, akurat miałem refaktoryzować model słownika do pewnej aplikacji finansowej, gdzie kod jest fatalny, ale małymi krokami staram się to zmieniać i tak oto napisałem fragmęt:
  1. <?php
  2. class CriteriumDictionary
  3. {
  4. const TYPE_ACTIVITY = 0,
  5. TYPE_SIZE = 1,
  6. TYPE_TRADE = 2,
  7. TYPE_VIOVODE = 3,
  8. TYPE_CAPITAL = 4,
  9. TYPE_REGION = 5,
  10. TYPE_INCOME = 6,
  11. NO_SUBCRITERIUM = 0
  12. ;
  13.  
  14. public static $type = [
  15. self::TYPE_ACTIVITY => 'Działalność',
  16. self::TYPE_SIZE => 'Rozmiar',
  17. self::TYPE_TRADE => 'Branża',
  18. self::TYPE_VIOVODE => 'Województwo',
  19. self::TYPE_CAPITAL => 'Kapitał',
  20. self::TYPE_REGION => 'Region',
  21. self::TYPE_INCOME => 'Przychód'
  22. ];
  23.  
  24. public static $activity = [
  25. 1 => 'Usługowa',
  26. 2 => 'Produkcyjna',
  27. 3 => 'Handlowa'
  28. ];
  29.  
  30. public static $size = [
  31. 1 => [
  32. 'name' => 'Mała',
  33. 'desc' => 'poniżej 50 pracowników'
  34. ],
  35. 2 => [
  36. 'name' => 'Średnia',
  37. 'desc' => 'od 50 do 249 pracowników'
  38. ],
  39. 3 => [
  40. 'name' => 'Duża',
  41. 'desc' => 'od 250 do 500 pracowników'
  42. ],
  43. 4 => [
  44. 'name' => 'Bardzo duża',
  45. 'desc' => 'powyżej 500 pracowników'
  46. ]
  47. ];
  48.  
  49. public static $trade = [
  50. 1 => 'Bankowość',
  51. 2 => 'Budownictwo',
  52. 3 => 'Energetyka i ciepłownictwo',
  53. 4 => 'Handel',
  54. 5 => 'Logistyka, transport',
  55. 6 => 'Przemysł',
  56. 7 => 'IT i telekomunikacja',
  57. 8 => 'Ubezpieczenia',
  58. 9 => 'Usługi dla ludności',
  59. 10 => 'Usługi dla biznesu',
  60. 11 => 'Outsourcing',
  61. 12 => 'Automotive',
  62. 13 => 'Chemiczna',
  63. 14 => 'Elektroniczna i elektrotechniczna',
  64. 15 => 'Materiały budowlane',
  65. 16 => 'Medyczna i farmaceutyczna',
  66. 17 => 'Metalowa i metalurgiczna',
  67. 18 => 'Spożywcza'
  68. ];
  69.  
  70. public static $voivode = [
  71. 1 => 'zachodniopomorskie',
  72. 2 => 'pomorskie',
  73. 3 => 'warmińsko-mazurskie',
  74. 4 => 'podlaskie',
  75. 5 => 'lubuskie',
  76. 6 => 'wielkopolskie',
  77. 7 => 'kujawsko-pomorskie',
  78. 8 => 'mazowieckie',
  79. 9 => 'łódzkie',
  80. 10 => 'lubelskie',
  81. 11 => 'dolnośląskie',
  82. 12 => 'opolskie',
  83. 13 => 'śląskie',
  84. 14 => 'małopolskie',
  85. 15 => 'podkarpackie',
  86. 16 => 'świętokrzyskie'
  87. ];
  88.  
  89. public static $capital = [
  90. 1 => 'Przewaga kapitału polskiego (powyżej 50%)',
  91. 2 => 'Przewaga kapitału zagranicznego (powyżej 50%)'
  92. ];
  93.  
  94. public static $region = [
  95. 1 => [
  96. 'name' => 'Wschód',
  97. 'voivodes' => [10, 15, 4, 16],
  98. ],
  99. 2 => [
  100. 'name' => 'Zachód',
  101. 'voivodes' => [5, 6, 1, 11, 12]
  102. ],
  103. 3 => [
  104. 'name' => 'Północ',
  105. 'voivodes' => [7, 2, 3]
  106. ],
  107. 4 => [
  108. 'name' => 'Południe',
  109. 'voivodes' => [13, 14]
  110. ],
  111. 4 => [
  112. 'name' => 'Centrum',
  113. 'voivodes' => [8, 9]
  114. ],
  115. ];
  116.  
  117. public static $income = [
  118. 1 => 'do 100 mln PLN',
  119. 2 => 'od 100 do 1000 mln PLN',
  120. 3 => 'powyżej 1000 mln PLN'
  121. ];
  122.  
  123. public static function get($criterium, $subcriterium=0) {
  124. self::throwExceptionIfCriteriumIsNotExists((int)$criterium);
  125. return self::getOneSubcriteriumIfSelectedOrAll((int)$criterium, (int)$subcriterium);
  126. }
  127.  
  128. public static function getNames($criterium) {
  129. self::throwExceptionIfCriteriumIsNotExists((int)$criterium);
  130.  
  131. return self::extractNamesForCriterium((int)$criterium);
  132. }
  133.  
  134. private static function extractNamesForCriterium($criterium) {
  135. $subcriterias = self::getOneSubcriteriumIfSelectedOrAll((int)$criterium, self::NO_SUBCRITERIUM);
  136.  
  137. switch ($criterium) {
  138. case self::TYPE_SIZE:
  139. case self::TYPE_REGION:
  140. $names = [];
  141. foreach ($subcriterias as $subcriteriumId => $subcriterium) {
  142. $names[$subcriteriumId] = $subcriterium['name'];
  143. }
  144. default:
  145. $names =& $subcriterias;
  146. break;
  147. }
  148.  
  149. return $names;
  150. }
  151.  
  152. private static function throwExceptionIfCriteriumIsNotExists($criterium) {
  153. if (self::isCriteriumExists($criterium) === false) {
  154. throw new Exception('Kryterium(numer '.$criterium.'), które jest wymaganie nie istnieje w słowniku');
  155. }
  156. }
  157.  
  158. private static function getOneSubcriteriumIfSelectedOrAll($criterium, $subcriterium) {
  159. if (self::isSelectedSubcriterium($subcriterium)) {
  160. self::throwExceptionIfSubcriteriumIsNotExists($criterium, $subcriterium);
  161.  
  162. return self::getData($criterium, $subcriterium);
  163. }
  164.  
  165. return $criterium;
  166. }
  167.  
  168. private static function isSelectedSubcriterium($subcriterium) {
  169. return $subcriterium > 0;
  170. }
  171.  
  172. private function throwExceptionIfSubcriteriumIsNotExists($criterium, $subcriterium) {
  173. if (isSubcriteriumExists($criterium, $subcriterium) === false) {
  174. throw new Exception('Subkryterium '.$subcriterium.' dla kryterium '.$criterium.' które jest wymaganie nie istnieje');
  175. }
  176. }
  177.  
  178. private static function getData($criterium, $subcriterium=0) {
  179. if (!isset(self::$type[$criterium])) {
  180. return [];
  181. }
  182.  
  183. if (isset(self::$type[$criterium][$subcriterium])) {
  184. return self::$type[$criterium][$subcriterium];
  185. }
  186.  
  187. return self::$type[$criterium];
  188. }
  189.  
  190. public static function isCriteriumExists($criterium) {
  191. return isset(self::$type[$criterium]);
  192. }
  193.  
  194. public static function isSubcriteriumExists($criterium, $subcriterium) {
  195. return isset(self::$type[$criterium]) && isset(self::$type[$criterium][$subcriterium]);
  196. }
  197. }



Czy z tymi nazwami to nie przesada? Wzorowałem się na ksiązce czysty kod. Jak wy rozwiązujecie sprawy nazw lepiej dać:
  1. if (isset($this->accountData[12]) && !empty($this->accountData[12])) {
  2. //do something
  3. }

czy może:
  1. if ($this->companyNameIsFilled()) {
  2. //do something
  3. }


Jakie inne dobre praktyki Wy polecacie? Czy takie tworzenie metod to nie przesada?

Ten post edytował daniel1302 9.10.2016, 10:57:27
Go to the top of the page
+Quote Post
nospor
post 9.10.2016, 13:35:30
Post #2





Grupa: Moderatorzy
Postów: 36 429
Pomógł: 6289
Dołączył: 27.12.2004




nie:
if (isset($this->accountData[12]) && !empty($this->accountData[12])) {

a poprostu
if (!empty($this->accountData[12])) {

Zas co do reszty twojego kodu to strasznie kola w oczy bledy w angielskim oraz nazwy funkcji,eg:

nie:
Is criterium exists
a:
Does criterium exist

Analogicznie cala reszta


Rowniez dlugosc nazw jest zdziebko zadluga
Zamiast
throwExceptionIfCriteriumIsNotExists
daj poprostu
checkIfCriteriumExists

Ponadto generujesz wszystko na static a potem jedno nie jako static, ale i tak wywolujesz jak static

Dodatkowo raz uzywasz stalych a raz nie

Za duzo tez rzeczy trzymasz jako wartosci w klasie. W zasadzie wiekszosc (jak nie wszystkie) powinny lezec w tabelach slownikowych



--------------------

"Myśl, myśl, myśl..." - Kubuś Puchatek || "Manual, manual, manual..." - Kubuś Programista
"Szukaj, szukaj, szukaj..." - Kubuś Odkrywca || "Debuguj, debuguj, debuguj..." - Kubuś Developer

Go to the top of the page
+Quote Post
daniel1302
post 9.10.2016, 17:41:07
Post #3





Grupa: Zarejestrowani
Postów: 602
Pomógł: 30
Dołączył: 1.08.2007
Skąd: Nowy Sącz

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


No tak, nie zastanowiłem się,
Co do tabel słownikowych tak była napisana aplikacja a jako, że jestem w trakcie refaktoryzacji kodu, chętnie przyjmę każdą poradę.

Mam kolegę który jest przeciwnego zdania, żeby nie trzymać takich rzeczy które raczej bardzo rzadko się zmieniają w tabelach w BD, że jest to strata optymalizacji.
Ten jeden brak statica to tez błąd, chciałem wysłać posta a musiałem wychodzić i to wszystko stąd, przerpaszam za takie olewnictwo sad.gif

A jak byś rozwiązał taki słownik, zrobił tabelę do każdego typu osobno i zrobił klasę abstrakcyjną lub interfejs, czy razem w jednej tabeli trzymał wszystko i jedną klasa to obsługiwał.

Niektóre wartości w słowniku mają tylko nazwe niektore wiecej informacji.
Go to the top of the page
+Quote Post
nospor
post 9.10.2016, 18:58:51
Post #4





Grupa: Moderatorzy
Postów: 36 429
Pomógł: 6289
Dołączył: 27.12.2004




Jakby chciec zrobic to super poprawnie, to na kazda tabele slownikowa by sie przydala klasa - model.
Zas ja osobiscie na taka rzecz uzywam jednej klasy, ktora zwraca mi co trzeba na podstawie parametru bedacego nazwa slownika.
Twoje tabele slownikowe powinny zawierac pola ID, NAME, DESC i zaspokoi to potrzeby w 99%
Raz widze masz jeszcze tam wojewodztwa. To albo dodac kolejne pole do tej tabeli i po przecinku ID wojewodztw albo tabele laczaca.

Zas co do optymalnosci na ktora skarzy sie kolega - no po to takie rzeczy trzyma sie w cache a nie pobiera za kazdym razem wink.gif


--------------------

"Myśl, myśl, myśl..." - Kubuś Puchatek || "Manual, manual, manual..." - Kubuś Programista
"Szukaj, szukaj, szukaj..." - Kubuś Odkrywca || "Debuguj, debuguj, debuguj..." - Kubuś Developer

Go to the top of the page
+Quote Post
kpt_lucek
post 10.10.2016, 08:32:04
Post #5





Grupa: Zarejestrowani
Postów: 428
Pomógł: 77
Dołączył: 10.07.2011
Skąd: Warszawa

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


@nospor

Zgodzę się z tabelami słownikowymi, pod warunkiem nie trzymania tam AI, poza tym, imo i tak lepiej trzymać stałą w obiekcie, zdecydowanie lepiej jest zrobić warunek:
  1. class SomeDictionary
  2. {
  3. const MIGHTY_PROPERTY = 1;
  4. }
  5.  
  6. if($someObject->someMethod(SomeDictionary::MIGHTY_PROPERTY)){/**/}


zamiast
  1. if($someObject->someMethod(1)){/**/}


Chociażby dlatego, że nie ważne w którym miejscu systemu odwołam się do const'a, to zawsze mam pewność co do jego wartości (zakładając zachowanie wartości const :: id z tabeli), dodatkowo, jeżeli z jakiegoś głupiego powodu chcę zmienić ten ID, to wystarczy zrobić to w miejscu, gdzie go definiuję.

Ten post edytował kpt_lucek 10.10.2016, 08:35:18


--------------------


Cytat
There is a Bundle for that
Lukas Kahwe Smith - October 31th, 2014
Go to the top of the page
+Quote Post
viking
post 10.10.2016, 08:34:51
Post #6





Grupa: Zarejestrowani
Postów: 6 365
Pomógł: 1113
Dołączył: 30.08.2006

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


A co więcej PHP ma http://php.net/manual/en/class.splenum.php


--------------------
Go to the top of the page
+Quote Post
Pyton_000
post 10.10.2016, 09:41:37
Post #7





Grupa: Zarejestrowani
Postów: 8 068
Pomógł: 1414
Dołączył: 26.10.2005

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


Co z tego skoro ma wmaganie (PECL spl_types >= 0.1.0) smile.gif
Go to the top of the page
+Quote Post
viking
post 10.10.2016, 10:22:47
Post #8





Grupa: Zarejestrowani
Postów: 6 365
Pomógł: 1113
Dołączył: 30.08.2006

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


Ano nic wink.gif Używam od dawna tego https://github.com/garoevans/php-enum
Jak jest natywnie to korzysta, jak nie to tworzy wrapper.


--------------------
Go to the top of the page
+Quote Post
nospor
post 10.10.2016, 10:29:28
Post #9





Grupa: Moderatorzy
Postów: 36 429
Pomógł: 6289
Dołączył: 27.12.2004




To ta klaska sluzy tylko i wylacznie do sprawdzania czy istnieja stale. Rety... czego to ludzie nie wymysla.... biggrin.gif


--------------------

"Myśl, myśl, myśl..." - Kubuś Puchatek || "Manual, manual, manual..." - Kubuś Programista
"Szukaj, szukaj, szukaj..." - Kubuś Odkrywca || "Debuguj, debuguj, debuguj..." - Kubuś Developer

Go to the top of the page
+Quote Post
viking
post 10.10.2016, 10:40:48
Post #10





Grupa: Zarejestrowani
Postów: 6 365
Pomógł: 1113
Dołączył: 30.08.2006

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


Jak przechodzisz z Javy to właśnie enumeratorów brakuje. Lepsze to niż interface symylujący.


--------------------
Go to the top of the page
+Quote Post
nospor
post 10.10.2016, 10:47:34
Post #11





Grupa: Moderatorzy
Postów: 36 429
Pomógł: 6289
Dołączył: 27.12.2004




No tak... java... jedno slowo warte 1000 innych wink.gif


--------------------

"Myśl, myśl, myśl..." - Kubuś Puchatek || "Manual, manual, manual..." - Kubuś Programista
"Szukaj, szukaj, szukaj..." - Kubuś Odkrywca || "Debuguj, debuguj, debuguj..." - Kubuś Developer

Go to the top of the page
+Quote Post

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

 



RSS Wersja Lo-Fi Aktualny czas: 19.03.2024 - 04:11