class DiscountService { protected $allDiscounts = [ 'group', 'code' ]; public function count($conferenceId, $attendantsCount = null, $price = null, $discountCode = null, $discountsTypes = [], &$is_error = false, &$error_message = null) { $discountsToProcess = $this->allDiscounts; } else { } $totalDiscount = 0; $excludeCodeDiscount = false; foreach ($discountsToProcess as $discount) { switch ($discount) { case 'group': $conference = $this->getConferencesRepository()->getConference($conferenceId); if ($conference === null) { } $groupDiscount = $conference->getGroupDiscount(); $is_error = true; $error_message = 'Error'; return; } $matchingDiscountPercent = 0; foreach ($groupDiscount as $minAttendantsCount => $discountPercent) { if ($attendantsCount >= $minAttendantsCount) { $matchingDiscountPercent = $discountPercent; } } $totalDiscount += $price * (float)"0.{$matchingDiscountPercent}"; $excludeCodeDiscount = true; break; case 'code': if ($excludeCodeDiscount == true) { continue; } $conference = $this->getConferencesRepository()->getConference($conferenceId); if ($conference === null) { } if ($conference->isCodeNotUsed($discountCode)) { list($type, $discount) = $conference->getDiscountForCode($discountCode); if ($type == 'percent') { $totalDiscount += $price * (float)"0.{$discount}"; } else if ($type == 'money') { $totalDiscount += $discount; } else { $is_error = true; $error_message = 'Error'; return; } $conference->markCodeAsUsed($discountCode); } break; } } return (float)$totalDiscount; } protected function getConferencesRepository() { return new ConferenceRepository(); } }
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?