Witam jeśli ktoś miałby czas i chęci do zrobienia CodeReview to bardzo będe wdzięczny za każdą wskazówke :)
0
0
Na wejście możesz wywalić folder .idea
1
Fajnie, że masz podział controllerów na View/Get/Edit itp., ale pogrupował bym je ficzerami lub encjami, np.:
App\Controller\Food\
- ViewFoodController
- EditFoodController
- GetFoodController
Dodatkowo jeżeli już zdecydowałeś się na takie podejście, to warto rozważyć Controllers as Services.
Wydaje mi się, że nazwa ViewFooController nie jest najlepsza. Chciałbym po nazwie klasy wiedzieć jaka jest różnica między controllerami View i Get.
Korzystaj z dobrodziejstw PHP 7 i używaj type hintów:
Jeżeli masz taką konstrukcję:
if ($condition === true) {
// jakiś kod...
return $foo;
} else {
// jakiś kod...
return $bar;
}
to możesz to zapisać używając early return:
if ($condition === true) {
return $foo;
}
// jakiś kod...
return $bar;
Co to u diabła jest $a? AddCastleController.php#L29
Używaj enumów lub chociaż stałych. AddCastleController.php#36
class Material {
public const CATEGORY_FOOD = 'Food';
public const CATEGORY_WOOD = 'Wood';
}
AddCastleController.php#L69 - to samo co wyżej. Co to znaczy 1000? Dlaczego 1000? Używaj stałych - Avoid magic numbers, albo zapakuj tego typu zmienne do konfiguracji. Wtedy masz plik konfiguracyjny (jakiś yml)
myGame:
populationGrowth:
requiredWoodSuppliesForOneMan: 500
i klasę, która łyka ten config:
class PopulationGrowthConfiguration {
// w konstruktorze przekazujesz config i z niego sobie tutaj wyciągasz
// wtedy dla 10 ludzi dostajesz że potrzeba 5000 drewna itd.
// A $checkFood->getValue() >= 1000 zamienia się w
// $checkFood->getValue() >= $this->configuration->getRequiredFoodSupplies($populationToGrow)
public function getRequiredWoodSupplies(int $populationToGrow): int {
return $this->configuration['requiredWoodSuppliesForOneMan'] * $populationToGrow;
}
public function getRequiredStoneSupplies(int $populationToGrow): int {}
}
Zawsze używaj nawiasów jak piszesz warunki. W kilku innych miejscach masz if'a, który nie ma nawiasu. Wygląda to po prostu niechlujnie. Is it a bad practice to use an if-statement without curly braces?. AddCastleController.php#L83
Jeżeli masz skomplikowane, piętrowe warunki (AddCastleController.php#L69), to możesz je wydzielić do osobnej klasy. Symfony ma od tego Votery. Możesz też zrobić coś takiego, że zamiast tego:
$checkFood->getValue() >= 1000 && $checkStone->getValue() >= 1000 && $checkWood->getValue() > 1000
robisz coś takiego:
class ActionPossible
{
private $isPossible;
private $reason;
public function __construct(bool $isPossible, string $reason = '')
{
$this->isPossible = $isPossible;
$this->reason = $reason;
}
public function isPossible(): bool
{
return $this->isPossible;
}
public function explainImpossible(): string
{
return $this->reason;
}
}
class PopulationGrowthAction
{
private $materialRepository;
private $buildRepository;
public function __construct(MaterialRepository $materialRepository, BuildRepository $buildRepository)
{
$this->materialRepository = $materialRepository;
$this->buildRepository = $buildRepository;
}
public function canGrowPopulation(User $user, int $population): ActionPossible
{
$checkFood = $this->materialRepository->findOneBy([
'user' => $user,
'category' => 'Food'
]);
$checkStone = $this->materialRepository->findOneBy([
'user' => $user,
'category' => 'Stone'
]);
$checkWood = $this->materialRepository->findOneBy([
'user' => $user,
'category' => 'Wood'
]);
if ($checkFood->getValue() < $population * 100) {
return new ActionPossible(false, 'Nie masz wystarczająco jedzenia');
}
if ($checkStone->getValue() < $population * 100) {
return new ActionPossible(false, 'Nie masz wystarczająco kamienia');
}
if ($checkWood->getValue() < $population * 100) {
return new ActionPossible(false, 'Nie masz wystarczająco drewna');
}
return new ActionPossible(true);
}
public function growPopulation(User $user, int $population, string $category): void
{
$actionPossible = $this->canGrowPopulation($user, $population);
if (false === $actionPossible->isPossible()) {
throw new UnableToGrowPopulationException($actionPossible);
}
$build = $this->buildRepository->findOneBy([
'category' => "Castle",
'user' => $user,
]);
if (false === $build instanceof Build) {
$build = new Build();
}
$this->buildRepository->addHouse($build, $population, $user, $category);
}
}
i używasz tego tak:
$actionPossible = $populationGrowthAction->canGrowPopulation($user, $population);
if ($actionPossible->isPossible()) {
$populationGrowthAction->growPopulation($user, $population, $category);
} else {
$this->addFlash("error", $actionPossible->explainImpossible());
}
Wtedy controller jest głupi i czysty, a Ty możesz to użyć ponownie w innym miejscu.
Powyższe rozwiązanie możesz zastosować do większości controllerów, bo jak tak po nich patrze, to sytuacja się powtarza.
MaterialRepository.php#L85 - nie musisz robić flusha po każdym persist. Wystarczy, że zrobisz go raz. Flush - o ile się nie mylę - powoduje wykonanie zapytań. Wystarczy to zrobić hurtem. Tak nawiasem mówiąc, to ten for tam jest niepotrzebny. Możesz równie dobrze wykonać wszystko jedno pod drugim.
Ogólnie nie jest źle, ale Twoje controllery mają za dużo logiki. Spróbuj to porozdzielać po innych klasach, tak jak zaproponowałem, albo inaczej (na przykład użyj zwykłego Votera + jakiegoś serwisu, który wykonuje akcję) + zrób coś z tymi magicznymi liczbami, bo to też plaga u Ciebie w kodzie :D
0
Desu napisał(a):
AddCastleController.php#L69 - to samo co wyżej. Co to znaczy
1000? Dlaczego1000? Używaj stałych - Avoid magic numbers, albo zapakuj tego typu zmienne do konfiguracji. Wtedy masz plik konfiguracyjny (jakiś yml)myGame: populationGrowth: requiredWoodSuppliesForOneMan: 500
W pliku konfiguracyjnym powinniśmy mieć tylko główny config dla apki. Stałe dane dotyczące klas powinny znaleźć się w constach. No i dużo łatwiej z każdego miejsca się potem do tego consta dostać, jeśli musimy go użyć.
0
@serek: zależy od projektu. Podałem to jako jedną z opcji. W moim przypadku jest to aplikacja, która działa w kilkunastu krajach i w zależności od kraju te zmienne są różne. Posiadanie tego w configu jest dużo łatwiejsze, bo:
- Jakby nie patrzeć jest to konfiguracja, więc rozsądnie jest trzymać konfigurację w jednym miejscu, a nie w miejscu zależnym od widzimisię autora kodu.
- Jeżeli te zmienne są różne w zależności od jakiegoś czynnika, to łatwiej jest używając DI wstrzyknąć odpowiedni config ładowany na zasadzie
config/localized/pl/foo.ymlniż bujać się z sufixem w nazwie stałej. Kolega ma tutaj jakąś gierkę przeglądarkową (tak to wygląda). Powiedzmy, że działa to tak jak plemiona, na których masz różne światy. Każdy nowy świat ma nową konfiguracją. Są np. światy szybkie i wolne. Wyobrażasz sobie teraz stałe w stylu:
public const WOOD_REQUIRED_FOR_4X_SPEED = 500;
public const WOOD_REQUIRED_FOR_3X_SPEED = 350;
public const WOOD_REQUIRED_FOR_1X_SPEED = 100;
public const WOOD_REQUIRED_FOR_15X_SPEED = 5000;
I te ify w 40 miejscach mmmm ;) Dużo łatwiej jest na początku sprawdzić, jaki mamy świat i załadować konfigurację dla niego.
3. Konfigurację możesz zamockować, przekazać inną (na potrzeby testów) itp., ze stałą już gorzej.
Oczywiście wszystko ma swoje plusy i minusy. Tutaj minusem jest to, że trzeba utworzyć dodatkową klasę, a później gdzieś ją tam wstrzyknąć, a dużo łatwiej jest zrobić:
MyClass::MY_CONFIG_VAR
Nie mniej jednak ja w większości przypadków wybrałbym rozwiązanie z configiem w pliku z uwagi na SRP. Konfiguracja, to konfiguracja, a porządek musi być.