[PHP]co można zarzucić tej klasie? (refaktoryzacja, wzorce), (refaktoryzacja, wzorce) Opcje

[jojnejojnejojne]

taki pierwszy etap na pracę Pana programisty. "uwagi do poniższego kodu"

[PHP] pobierz, plaintext 
class DiscountService
{
    protected $allDiscounts = [
        'group', 'code'
    ];
    public function count($conferenceId, $attendantsCount = null, $price = null, $discountCode = null, $discountsTypes = [], &$is_error = false, &$error_message = null)
    {
        if (empty($discountsTypes)) {
            $discountsToProcess = $this->allDiscounts;
        } else {
            $discountsToProcess = array_intersect($this->allDiscounts, $discountsTypes);
        }
        $totalDiscount = 0;
        $excludeCodeDiscount = false;
        foreach ($discountsToProcess as $discount) {
            switch ($discount) {
                case 'group':
                    $conference = $this->getConferencesRepository()->getConference($conferenceId);
                    if ($conference === null) {
                        throw new \InvalidArgumentException(sprintf("Conference with id %s not exist", $conferenceId));
                    }
                    $groupDiscount = $conference->getGroupDiscount();
                    if (!is_array($groupDiscount)) {
                        $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) {
                        throw new \InvalidArgumentException(sprintf("Conference with id %s not exist", $conferenceId));
                    }
                    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();
    }
}
[PHP] pobierz, plaintext 

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?

Ten post edytował jojnejojnejojne 11.03.2017, 16:54:47