Czy funkcja nie łamie zasady SOLID ?

phpowiec napisał(a):

jurek1980 napisał(a):

Abstrahując troszkę od tematu, a może i nie.
Nie wiem, czy do nauki SOLID nie spróbować czystego OOP bez framworka. Tak dochodzą jeszcze warstwy architektura. Jak się ogarnie podstawy to potem łatwiej to zobaczyć w bardziej rozbudowanym kodzie.

@jurek1980: nie mam problemu ze stosowaniem solid w projekcie napisanym w czystym php, problem jaki mi sprawia to do jakich katalogów powinienem wydzielić odpowiednie funkcje

No to nie wydzielaj, dopóki nie stanie się dla Ciebie jasne że niektóre klasy "należą" blisko siebie.

@phpowiec: Dokładnie tak jak pisał @jurek1980. Jeżeli chodzi o strukturę katalogów oraz ich "powiązanie" z SOLID to dwie różne rzeczy. Na razie nie myśl gdzie dany plik wyląduje tylko za co on będzie odpowiadał. I teraz @Riddle dobrze napisał - dopóki masz jedną metodę z jednym kontrolerem to nie potrzebna Ci struktura katalogów :) Jak projekt zacznie Ci "puchnąć" sam się zorientujesz, że Twoja struktura wymaga zmiany.

Co do Twojego pytania do mnie w komentarzy to repository zostaw do SAMEJ komunikacji z modelem - tak unikniesz problemu, który opisał @jurek1980 : "Wybrałeś jeszcze Laravel które oparty jest o ActiveRecord i w zasadzie każdy model łamie SRP.". W service trzymaj tylko rzeczy, które mają "coś robić" na dostarczonych danych (czyli tzw. logika biznesowa).

@leonpro778:
Stworzyłem serwis który wygląda tak,

class UserService
{

    public function createUser($request): void
    {
        User::create($request);
    }

    public function hashPassword($request): string
    {
        return Hash::make($request);
    }

}

Natomiast kontroler wygląda tak teraz:

public function store(RegistrationRequest $request, UserService $user)
    {
        $validated = $request->correctValidate();

        $validated['password'] = $user->hashPassword($validated['password']);
        $user->createUser($validated);

        return redirect()->route(
            'index.store'
        )->with('success', 'Success registration');
    }

W kontrolerze wywołuję teraz 3 funkcje, tylko nadal mam wątpliwości czy teraz nie są łamane zasady SOLID, a jeżeli tak to co jeszcze mogę wydzielić z kontrolera ?

Według mnie ok pod względem SRP. Chociaż funkcja hashujaca powinna być prywatna i powinna być wykonywana wraz z wywołaniem publicznej funkcji tworzącej użytkownika.
Teraz inne zasady, OLI pewnie ok, a co z D?

phpowiec napisał(a):

W kontrolerze wywołuję teraz 3 funkcje, tylko nadal mam wątpliwości czy teraz nie są łamane zasady SOLID, a jeżeli tak to co jeszcze mogę wydzielić z kontrolera ?

Moim zdaniem to co zrobiłeś nie pomogło za wiele, i już lepiej było to zostawić tak jak było.

@Riddle: Z tego co wiem kontroler powinien otrzymać żądanie i zwrócić odpowiedź, a cała logika biznesowa powinna być w serwisie, a wywołanie tych funkcji w kontrolerze nie powoduje że łamane jest SRP

phpowiec napisał(a):

@Riddle: Z tego co wiem kontroler powinien otrzymać żądanie i zwrócić odpowiedź, a cała logika biznesowa powinna być w serwisie, a wywołanie tych funkcji w kontrolerze nie powoduje że łamane jest SRP

To nie jest takie proste.

Zasady SOLID są po to żeby sprawić że oprogramowanie jako całość jest bardziej utrzymywalne, odporniejsze na zmiany i bardziej testowalne. Ty wsadziłeś User::create() oraz Hash::make() do klasy UserService. I zgadzam się, że masz dobre intencje, ale ta zmiana raczej nie wniosła nic dobrego do tej aplikacji, a jedynie zaciemniła całość.

@Riddle: A mógłbyś podpowiedzieć co w tym przypadku mógłbym zrobić?

phpowiec napisał(a):

@Riddle: A mógłbyś podpowiedzieć co w tym przypadku mógłbym zrobić?

Jeśli na prawdę chcesz usprawnić swoją aplikację, to powinieneś napisać dobre testy automatyczne.

A co do kodu, to ja bym go zostawił tak jak w pierwszym poście był.

W jaki sposób mam napisać testy skoro nie mam w aplikacji nic oprócz rejestracji usera, no nic w takim razie pozostałe funkcje aplikacji będę pisał w podobny sposób czyli cała logika w kontrolerze

phpowiec napisał(a):

W jaki sposób mam napisać testy skoro nie mam w aplikacji nic oprócz rejestracji usera, no nic w takim razie pozostałe funkcje aplikacji będę pisał w podobny sposób czyli cała logika w kontrolerze

No normalnie.

• Strzel pod endpoint, zobacz czy user jest dodany w bazie
• Strzel pod endpoint z niepoprawnym emailem, zobacz czy dostaniesz odpowiedni kod błędu
• Strzel pod endpoint ze zduplikowanym emailem, zobaczy czy dostaniesz odpowiedni błąd
• Strzel pod endpoint z niezgadzającymi się password i password_repeat, zobacz czy dostaniesz odpowiedni kod błędu
• Strzel pod endpoint, zobaczy czy renderujesz odpowiedni widok
• Strzel pod endpoint ze zduplikowanym loginem, zobacz czy nie udało się stworzyć drugi raz usera o takim samym loginie
• Strzel pod endpoint z wartościami większymi niż max: oraz min: z requestu

Masę rzeczy powinieneś sprawdzić.

Wydaje Ci się, że nie masz żadnej logiki w aplikacji; ale masz dużo kodu wpisanego zarówno w kontrolerze oraz requeście.

@Riddle: I jeżeli testy przejdą to znaczy że ten kod będzie ok ?

phpowiec napisał(a):

@Riddle: I jeżeli testy przejdą to znaczy że ten kod będzie ok ?

Przechodzące testy niekoniecznie muszą oznaczać że kod jest okej. Jeśli napisałes testy niepoprawnie, to wtedy jest kappa.

Testy trzeba napisać w odpowiedni sposób, napisz je i wrzuć na forum.

phpowiec napisał(a):

W jaki sposób mam napisać testy skoro nie mam w aplikacji nic oprócz rejestracji usera, no nic w takim razie pozostałe funkcje aplikacji będę pisał w podobny sposób czyli cała logika w kontrolerze

Zakładam, że wiążesz swoją przyszłość zawodową z tym właśnie frameworkiem dlatego od razu napiszę, robisz to źle - zarówno dla SIEBIE (uczysz się źle pisać) jak i dla osób, które będą oceniać Twój kod pod względem "czytelności" (piszę tutaj o potencjalnych rekruterach).

@Riddle: Nie za bardzo wiem co znaczy dla Ciebie "zaciemnianie" kodu używając UserService (chociaż sam stwierdziłeś, że autor "miał dobre intencje" :) ). Weź jednak pod uwagę to, że ten kod jest pisany w Laravel'u i pod takim właśnie kątem należałoby ten kod ocenić a nie pod 100% poprawnością SOLID (bo jak wszyscy tutaj się wcześniej zgodzili to 99% framworków nie jest zgodne z SOLID).
Fajnie napisałeś te trzy punkty odnośnie potrzeby SOLID.

Łatwość utrzymania - póki Laravel istnieje to łatwe to będzie na tyle łątwe, na ile jest dobrze napisane POD KĄTEM właśnie Laravel. Ja wiem, że istnieje "niezerowa" szansa, że Laravel pewnego dnia "wyparuje", zniknie czy trzeba będzie się przesiąść na coś innego i wówczas wszystkie helpery, wszystkie extends'y będą "bezużyteczne" i trzeba będzie pisać od nowa ale tak jak pisałem wcześniej, tutaj zasadę SOLID rozpatrywałbym w kontekście właśnie danego frameworka. (sam napisałeś również, że czasami nie da się przestrzegać tych wszystkich zasad).

Odporniejsze na zmiany - w tym przykładzie ciężko stwierdzić czy będzie ono odporne na zmiany (mamy JEDNĄ metodą tworzącą użytkownika i nic więcej) - no, chyba, że zmienimy framework.

Testy - już opisałeś :)

@phpowiec: Podsumowując jeszcze Twój kod:

// $validated = $request->correctValidate(); - po co to jest?
// $validated['password'] = $user->hashPassword($validated['password']); - tak jak pisał jurek1980 - do UserService i jako prywatne
$user->createUser($request);

Kolejna rzecz odnośnie trzymania wszystkiego w kontrolerze - wyobraź sobie, że masz kilka typów użytkowników, ADMIN, USER, CUSTOMER.

Przykład 1 - wszyscy użytkownicy są tworzeni w ten sam sposób - ok, Twoja metoda się sprawdzi.
Przykład 2 - customer ma mieć dodatkowe pole takie jak "ważność konta" - i wtedy co robisz? Zgodnie z Twoim rozumowaniem to powinieneś napisać jakiegoś IF'a, który sprawdza, czy nowy użytkownik będzie customerem i dodawać to pole i to będzie bez sensu bo im więcej tych typów tym więcej zmian (i tych IF'ów)

I to taki pierwszy z brzegu przykład także moja rada - nie trzymaj całej logiki w kontrolerze - nic z tego dobrego nie wyjdzie.