Prosba o code review

Question

Czesc!

Zrobilem prosta ksiazke adresowa gdzie tworzy sie uzytkownika a nastepnie mozna mu przypisac adres zamieszkania, dopiero zaczynam przygode z symfony i czuje sie zagubiony. Jezeli ktos mialby chwile i moglby zerknac w moj kod i powiedziec nad czym popracowac co poprawic albo poprostu co zrobilem slabo bylbym wielce wdzieczny.

https://github.com/Marduuk/AddressBook

Pozdrawiam serdecznie

Answer 1

Robisz metody które mają zbyt dużą odpowiedzialność z nic nie mówiącą nazwą.

https://github.com/Marduuk/AddressBook/blob/master/src/AddressBookBundle/Controller/UserController.php#L37

    public function fromPostToDb($form)
    {
        $post = $form->getData();
        $em = $this->getDoctrine()->getManager();
        $em->persist($post);
        var_dump($em->flush());
        $em->flush();
        return true;
    }

Ta metoda jest gdzieś używana?

Poczytaj o zasadzie Single Responsibility Pattern (SRP) a najlepiej o całym SOLID.

Stosuj częściej Dependency Injection (DI) i poznaj Inversion of Control (IoC).

Nie będę zaglądał w głębiny metod, są zbyt duże. Popatrz, gdyby pączek był wypełniony samym dżemem, bez ciasta w środku, tylko otoczka aby się nie wylał, to nie bałbyś się że się upier***isz podczas jedzenia (O ile już nie rozpadnie się podczas próby złapania)?

Gdy będziesz czytał artykuły wikipedii koniecznie przejrzyj źródła!

I tak z ciekawości, po co Ci encje z dopisaną tyldą?

Comments

Dzieki wielkie za odpowiedz!

Tej funkcji zeby zapisac do db nie uzywalem mialem lekki problem z podpieciem tego, powinno to isc do katalogu z repozytoriami czy w modelu dodac taka funkcje? Sam jej koncept jest niezly nie?

Biore sie powolutku do ogaqrniecia tego co opisales dzieki :)

Encje z dopisana tylda? Chodzi ci o 2 puste nie zaimplementowane encje, phone i mail?

---

IMO to to powinno znajdować się w repozytorium w metodzie commit. :D

Przykład:

    public function commit()
    {
        $this->entityManager->flush();
        $this->entityManager->getConnection()->commit();
    }

Może Cię zainteresować: https://zawarstwaabstrakcji.pl/20151020-save-repository-from-save/

Answer 2

1. Wrzuciłem Ci pull requesta.
2. Czemu używasz Symfony 2.8? Polecam używać 3.2/3.3
3. Skoro używasz: 

@Template("AddressBookBundle:User:new.html.twig")

To wystarczy jak zwrócisz samą tablicę z parametrami:
 

return ['form' => $form->createView()];

3. Zamiast 

$this->getDoctrine()->getManager()

możesz utworzyć serwis z odwołaniem się do odpowiedniej encji (przykład masz w pr)

4. Nie twórz FormBuildera w kontrollerze. http://symfony.com/doc/current/forms.html#creating-form-classes
Możesz łatwo utworzyć formularz przez konsolę.

builder->add("name","text")

2 parametr jako string jest zdeprecjonowany, użyj TextType:class.
 

$desc=$post->getDescription();// mam wrazenie ze to glupio zrobilem mozna inaczej?

Można lepiej. Nie rób tego w kontrollerze. Tylko w Twigu możesz odwołać się do getterów i setterów. np. post.getDescription, wersja skrócona: post.description.

Po co przekazujesz 'success' => "success" ? Wystarczy jak dasz 'post' => $post i to Ci wystarczy. 

Zamiast array(), użyj skróconej wersji [ ].

5. Zamiast return new Response("Usunieto"); możesz dać redirecta do controllera z listą userów, a przed redirectem flash-message.
6. Przy definiowaniu routingu warto dodać name. 
7. W metodzie modifyAction nie masz obsługi formularza formAddress.
8. Używaj PHPDoc. Zamiast "name"=>"$name", daj zmienną $name bez cudzysłowia. 
9. Możesz w configu dodać: 

twig:
    form_themes:
        - 'bootstrap_3_horizontal_layout.html.twig'

Wtedy będziesz miał ładniejsze bootstrapowe formularze.

10. Pisząc testy zamiast WebTestCase używaj  \PHPUnit_Framework_Test.
11. Pisząc tego typu CRUDY warto użyć http://symfony.com/doc/current/bundles/DoctrineFixturesBundle/index.html + https://packagist.org/packages/fzaninotto/faker do generowania randomowych danych.

Comments

Dzieki wielkie! Biore sie za zrozumienie tego :)