PHP kawałek kodu do oceny - powrót projektu gra

pytanie zadane 26 kwietnia 2016 w Nasze projekty przez makoso Mądrala (7,380 p.)
edycja 26 kwietnia 2016 przez makoso

Tak jak w temacie, wróciłem do projektu swojej gry, gra strategia, a'la plemiona tylko w aktualnych czasach, w pewnej funkcji wyszło mi sporo ifów i chciałbym się dowiedzieć jak pod względem "czystości" kody to wygląda, ponieważ grę robię w celu nauki czyli także git-a ;) wrzucam kod na githuba

Całe repozytorium: https://github.com/makoso/My-Strategy-Game
Serwis wioski, obsługuje wszystkie jej funkcje: https://github.com/makoso/My-Strategy-Game/blob/master/src/AppBundle/Services/VillageService.php
Powyższa klasa zawiera funkcje o której mówiłem wyżej i zamieszczam ją także poniżej

//FUNKCJA ODPOWIADAJĄCA ZA SZKOLENIE JEDNOSTEK jednostki dodaje się grupowo do kolejki ale mogą opuszczać ją pojedynczo


public function updateUnits()
    {
        /**
         * @var $update \AppBundle\Entity\Game\BarracksQueue
         */
        foreach ($this->villages as $village) {
            $toUpdate = $this->em->getRepository('AppBundle:Game\BarracksQueue')->findEnd($village->getId());
            foreach ($toUpdate as $update) {
                $village = $this->em->getRepository('AppBundle:Player\Village\Village')->find($update->getVillage());
                if (!$village) {
                    $this->em->remove($update);
                } else {
                    $units = $village->getArmy();
                    $method = "set".ucfirst($update->getUnitName());
                    if (method_exists($units, $method)) {
                        if ($update->getTimeStart() < $_SERVER['REQUEST_TIME']) {
                            $all = $update->getUnitCount();
                            $end = $update->getUnitEndSchool();
                            if($update->getTimeEnd() < $_SERVER['REQUEST_TIME']){
                                $units->{$method}($units->{'get'.ucfirst($update->getUnitName())}() + ($all - $end) );
                                $this->em->persist($units);
                                $this->em->remove($update);
                            }else {
                                if($all > $end){
                                    $nowEnd = floor(($_SERVER['REQUEST_TIME'] - ($update->getTimeStart() + ($end * $update->getUnitOneTime()))) / $update->getUnitOneTime());
                                    if($nowEnd > 0){
                                        $units->{$method}($units->{'get'.ucfirst($update->getUnitName())}() + $nowEnd );
                                        $update->setUnitEndSchool($update->getUnitEndSchool() + $nowEnd);
                                        if($update->getUnitEndSchool() === $update->getUnitCount()){
                                            $this->em->remove($update);
                                        }
                                        $this->em->persist($units);
                                    }
                                } else {
                                    $this->em->remove($update);
                                    $this->em->persist($units);
                                }
                            }
                        }
                    }
                }
            }
            $this->em->flush();
        }
        return $this;
    }

komentarz 27 kwietnia 2016 przez xandros Nałogowiec (29,450 p.)

> wyszło mi sporo ifów

nie da się ukryć, ale bardziej rzuca sięw oczy używanie dużej ilości nestedów i długość metody (az 40 lini kodu w jednej metodzie).

Podziel to na mniejsze metody i staraj sie używać continiue/return/throw,try,catch

komentarz 27 kwietnia 2016 przez makoso Mądrala (7,380 p.)

Szczerze mówiąc nie potrafię używać continue, to można stosować tylko w while? mógłbyś mi opisać praktyczne zastosowanie tego? throw odpada w tym przypadku dlatego zastosowałem tyle ifów bo wykonywanie kodu nie może zostać przerwane w przypadku błędy em kasuje dany rekord i kod leci dalej dlatego return jest na końcu po wykonaniu flush() odnośnie długości metody to trafiają mi się i dłuższe, nie wiem, tłumacze sobie to że w grze mogę stosować takie długie funkcje ;) tej logiki nie użyje identycznie w innych miejscach, co mogłem to wyciągnąłem reszta musi być zapisana... i tak o to powstają takie metody:

 public function schoolAction($id, $unit, $count)
    {
        $securityService = $this->get('app.security');
        $securityService->setEm($this->getDoctrine()->getManager());
        if ($village = $securityService->securityVillage($id, $this->getUser())) {
            $villageService = $this->get('app.village');
            $villageService->setVillage($village);
            $villageData = $villageService->dataOfVillage();
            $em = $this->getDoctrine()->getManager();
            $toJSON = ['done' => false, 'msg' => ""];
            if (is_int($villageData[$unit])) {
                $cost = $villageService->costOfUnit($unit);
                if ($villageData[$cost['requirementResearch']] !== 1) {
                    $toJSON['msg'] = "Przeprowadź wymagane badanie";
                } else {
                    if (
                        ($village->getFood() - ($cost['food']) * $count) > 0 &&
                        ($village->getPopulation() - ($cost['people']) * $count) > 0 &&
                        ($village->getIron() - ($cost['iron']) * $count) > 0 &&
                        ($village->getGold() - ($cost['gold']) * $count) > 0
                    ) {
                        $basicTime = $_SERVER['REQUEST_TIME'];
                        $village
                            ->setFood($village->getFood() - ($cost['food'] * $count))
                            ->setPopulation($village->getPopulation() - ($cost['people'] * $count))
                            ->setIron($village->getIron() - ($cost['iron'] * $count))
                            ->setGold($village->getGold() - ($cost['gold'] * $count));
                        $inQueue = new BarracksQueue();
                        $queue = $this->getDoctrine()->getManager()->getRepository(
                            'AppBundle:Game\BarracksQueue'
                        )->findBy(['village' => $id]);
                        if (count($queue) > 0) {
                            $basicTime = $queue[count($queue) - 1]->getTimeEnd();
                        }
                        $inQueue->setVillage($village->getId())
                            ->setUnitName($unit)
                            ->setUnitCount($count)
                            ->setTimeStart($basicTime)
                            ->setUnitOneTime($cost['time'])
                            ->setTimeEnd($basicTime + ($cost['time'] * $count));
                        $em->persist($village);
                        $em->persist($inQueue);
                        $em->flush();
                        $toJSON['done'] = true;
                        $toJSON['msg'] = "Rozpocząto szkolenie $count jednostek";
                    } else {
                        $toJSON['msg'] = "Brakuje surowców";
                    }
                }
            } else {
                $toJSON['msg'] = "Wystąpił bład, badanie który chcesz przeprowadzić nie istnieje";
            }

            $toJSON['timezone'] = $_SERVER['REQUEST_TIME'];
            $toJSON['dataOfVillage'] = $villageService->dataOfVillage();
            $toJSON['queue'] = $this->renderView(
                ':Game/Game/Builds/Barracks:queue.html.twig',
                array(
                    'queue' => $this->getDoctrine()->getManager()->getRepository(
                        'AppBundle:Game\BarracksQueue'
                    )->findBy(['village' => $id]),
                )
            );
            $toJSON['rightVillageBar'] = $this->renderView(
                ':Game/Include:villageRightDataInfo.html.twig',
                array(
                    'village' => $villageService->dataOfVillage(),
                )
            );
            $toJSON['headVillageData'] = $this->renderView(
                ':Game/Include:villageHeader.html.twig',
                array(
                    'village' => $villageService->dataOfVillage(),
                )
            );
            $cost['recruit'] = ["pl" => "Rekrut", "en" => "recruit", "cost" => $villageService->costOfUnit('recruit')];
            $cost['soldier'] = [
                "pl" => "Zołnierz",
                "en" => "soldier",
                "cost" => $villageService->costOfUnit('soldier')
            ];
            $cost['terrorist'] = [
                "pl" => "Terrorysta",
                "en" => "terrorist",
                "cost" => $villageService->costOfUnit('terrorist')
            ];
            $cost['sniper'] = ["pl" => "Snajper", "en" => "sniper", "cost" => $villageService->costOfUnit('sniper')];
            $toJSON['unitsList'] = $this->renderView(
                ':Game/Game/Builds/Barracks:unitsList.html.twig',
                array(
                    'village' => $villageService->dataOfVillage(),
                    'cost' => $cost,
                )
            );

            return new JsonResponse($toJSON);
        } else {
            return $this->redirectToRoute('villages');
        }
    }

ps zwracam JSON ponieważ szkolenie odbywa się w tle, użytkownik dostaje informację zawartą w msg ;) to co renderuje do zmiennych to po prostu podmieniam/aktualizuje content ;)

komentarz 28 kwietnia 2016 przez xandros Nałogowiec (29,450 p.)

Pomyliłem odpowiedź z komentarzem. Dlatego tutaj odpowiem jedynie na temat continiue, a resztę odpiszę niżej w odpowiedziach.

while(1){
//jakis kod, ktory sie wykona
    if(true){
       //wraca na poczatek petli
        continue;
    }
  //jakis kod, ktory sie nie wykona, bo warunek zostal spelniony
}

foreach($array as $item){
//jakis kod, ktory sie wykona
    if(true){
        //wraca na poczatek petli z nastepnym elementem jako $item
        continue;
    }
  //jakis kod, ktory sie nie wykona, bo warunek zostal spelniony
}
//podobnie z petla `for`, tyle ze wykonuje sie zadanie po 2 sredniku,
// (przewaznie inkrementacja)

foreach($array as $item){
//jakis kod, ktory sie wykona
    while(1){
         if(true){
            //wraca na poczatek pierwszej petli 
            //(czyli foreach z nastepnym elementem jako $item)
            continue 2;
        }
         //kod, ktory sie nie wykona
    }
}

http://php.net/manual/en/control-structures.continue.php

komentarz 28 kwietnia 2016 przez makoso Mądrala (7,380 p.)

dzięki! przeglądałem manual ale nie bardzo rozumiałem zapis

while(1){
//jakis kod, ktory sie wykona
    if(true){
       //wraca na poczatek petli
        continue;
    }
  //jakis kod, ktory sie nie wykona, bo warunek zostal spelniony
}

teraz wszystko jasne :D dzięki zobaczę jak mogę skrócić swój kod z pomocą tego :)

2 odpowiedzi

odpowiedź 28 kwietnia 2016 przez xandros Nałogowiec (29,450 p.)

> throw odpada w tym przypadku dlatego zastosowałem tyle ifów bo wykonywanie kodu nie może zostać przerwane w przypadku błędy em kasuje dany rekord i kod leci dalej dlatego return

Jedna metoda jedno dzialanie. Cos tam probowalem w wolnej chwili refactorowac, ale troche tego jest: http://pastebin.com/YCLNAiMi

Popraw nazwy, bo nie wiem, co gdzie i jak. Co to w ogóle $all i $end ?

W sumie mógłbym tak dlugo refactorowac, ale mam nadzieje, że teraz wiesz, o co mi chodzilo

komentarz 28 kwietnia 2016 przez event15 Szeryf (93,790 p.)

Ja bym chciał zauważyć, że repository to takie odzwierciedlenie półki z książkami. Czyli nie mogę wziąć książki i zmienić sobie jej tytułu czy treści wykorzystując Read Model.

Dobrą praktyką jest wykorzystanie wzorca specyfikacji do podawania kryteriów wyszukiwania po repozytorium. No i lepiej jest unikać metod "getCoś" - tym bardziej, jeśli mówimy o repository.

Jeśli już chcesz uczyć dobrych praktyk, to pamiętaj, że:

if (!($this->getNowEnd > 0)) { }

Jest wierszem, na który się poświęci więcej czasu niż na zwykłe porównanie:

if($this->getNowEnd <= 0) {}

A wiele returnów w ramach jednej metody jest też źle widziane.

Tak, czy siak i tak jest nieźle

komentarz 28 kwietnia 2016 przez makoso Mądrala (7,380 p.)

Dzięki przejrzę dokładnie co opłaca się wyciągnąć :)
wiem że chodzi o podzielenie na mniejsze metody, nie stosuję interface-ów, ostatnio dopiero o nich czytałem ;) wielkie dzięki za poświęcenie czasu na ten kawałek kodu!

komentarz 28 kwietnia 2016 przez makoso Mądrala (7,380 p.)

Zrobiłem to co mogłem ;) i wydawało mi się słuszne, rozbiłem na 3 funkcje i teraz mam to funkcjonalne łącznie do 3 budynków które będą pozwalały na rekrutację, dodatkowo najważniejsze mogę oddzielnie aktualizować:
A) Całego gracza (wszystkie jego wioski)
B) Jedną wioskę
C) Konkretny budynek/i w konkretnej wiosce
Rozumiem że można to bardziej rozbijać ale po co jak nie będzie ponownie wykorzystywane w tej klasie (mam teraz parę poprawek w kilku klasach ;) )
nie wiem czy nie przesadziłem z continue teraz ;)
ale spodobało mi się ;) Dzięki!
http://pastebin.com/z5b29w5f

odpowiedź 27 kwietnia 2016 przez SyntaxError Pasjonat (17,170 p.)

Czemu Entity\Game\BarracksQueue::$village jest w doctrinie ustawione na integer? Skoro trzyma idka z relacji ManyToOne do Village.

komentarz 27 kwietnia 2016 przez makoso Mądrala (7,380 p.)

Hmmm musiałem się zapędzić, czyli taki błąd mam w 3 kolejkach, jednak łatwy do poprawienia, dzięki :)
Nie wiem czy otwierałeś cały serwis, jest on już spory ale będzie jeszcze około 2x dłuższy, czyli około 500-600 linijek kody będzie, jest to dopuszczalne? Chyba tutaj ktoś kiedyś mi napisał że klasa nie powinna mieć więcej jak 300 linijek kody.

Powiedz mi jeszcze jedno, jeżeli wiesz, czy jak skończę taki projekt wszystko będzie działało, ale wyglądało nie za ładnie będę mógł pokazywać ten projekt i szukać pracy jako junior backend developer?