Pomoc - Szukaj - Użytkownicy - Kalendarz
Pełna wersja: [PHP]co można zarzucić tej klasie? (refaktoryzacja, wzorce)
Forum PHP.pl > Forum > Przedszkole
jojnejojnejojne
taki pierwszy etap na pracę Pana programisty. "uwagi do poniższego kodu"

  1. class DiscountService
  2. {
  3. protected $allDiscounts = [
  4. 'group', 'code'
  5. ];
  6. public function count($conferenceId, $attendantsCount = null, $price = null, $discountCode = null, $discountsTypes = [], &$is_error = false, &$error_message = null)
  7. {
  8. if (empty($discountsTypes)) {
  9. $discountsToProcess = $this->allDiscounts;
  10. } else {
  11. $discountsToProcess = array_intersect($this->allDiscounts, $discountsTypes);
  12. }
  13. $totalDiscount = 0;
  14. $excludeCodeDiscount = false;
  15. foreach ($discountsToProcess as $discount) {
  16. switch ($discount) {
  17. case 'group':
  18. $conference = $this->getConferencesRepository()->getConference($conferenceId);
  19. if ($conference === null) {
  20. throw new \InvalidArgumentException(sprintf("Conference with id %s not exist", $conferenceId));
  21. }
  22. $groupDiscount = $conference->getGroupDiscount();
  23. if (!is_array($groupDiscount)) {
  24. $is_error = true;
  25. $error_message = 'Error';
  26. return;
  27. }
  28. $matchingDiscountPercent = 0;
  29. foreach ($groupDiscount as $minAttendantsCount => $discountPercent) {
  30. if ($attendantsCount >= $minAttendantsCount) {
  31. $matchingDiscountPercent = $discountPercent;
  32. }
  33. }
  34. $totalDiscount += $price * (float)"0.{$matchingDiscountPercent}";
  35. $excludeCodeDiscount = true;
  36. break;
  37. case 'code':
  38. if ($excludeCodeDiscount == true) {
  39. continue;
  40. }
  41. $conference = $this->getConferencesRepository()->getConference($conferenceId);
  42. if ($conference === null) {
  43. throw new \InvalidArgumentException(sprintf("Conference with id %s not exist", $conferenceId));
  44. }
  45. if ($conference->isCodeNotUsed($discountCode)) {
  46. list($type, $discount) = $conference->getDiscountForCode($discountCode);
  47. if ($type == 'percent') {
  48. $totalDiscount += $price * (float)"0.{$discount}";
  49. } else if ($type == 'money') {
  50. $totalDiscount += $discount;
  51. } else {
  52. $is_error = true;
  53. $error_message = 'Error';
  54. return;
  55. }
  56. $conference->markCodeAsUsed($discountCode);
  57. }
  58. break;
  59. }
  60. }
  61. return (float)$totalDiscount;
  62. }
  63. protected function getConferencesRepository()
  64. {
  65. return new ConferenceRepository();
  66. }
  67. }


powiedziałbym:
- zbyt wiele robiąca metoda count(), należało by ją podzielić na mniejsze.
- za dużo parametrów w metodzie count(),
- zagnieżdżone foreach wewnątrz foreach
- zagnieżdżone if/else/else if, należałoby zrobić więcej małych bloków z exit points.
- należało by wstrzyknąć zewnętrzny obiekt do konstruktora lub setter i uzależnić się od abstrakcji.

mam rację? coś jeszcze?
kayman
imo napisane tak by było wszystko źle, opowiadanie można napisać i chyba o to chodzi, więc czym więcej błędów wymienisz tym lepiej smile.gif
To jest wersja lo-fi głównej zawartości. Aby zobaczyć pełną wersję z większą zawartością, obrazkami i formatowaniem proszę kliknij tutaj.
Invision Power Board © 2001-2025 Invision Power Services, Inc.