• Najnowsze pytania
  • Bez odpowiedzi
  • Zadaj pytanie
  • Kategorie
  • Tagi
  • Zdobyte punkty
  • Ekipa ninja
  • IRC
  • FAQ
  • Regulamin
  • Książki warte uwagi

Mój CMS, prośba o code review

VPS Starter Arubacloud
0 głosów
432 wizyt
pytanie zadane 11 grudnia 2017 w PHP przez jpacanowski VIP (101,940 p.)

Witajcie,

Od jakiegoś czasu piszę własny system CMS w PHP (OOP & MVC) na którym zamierzam postawić własnego bloga. Prosiłbym Was o zrobienie code review tego systemu jeśli tylko możecie i macie czas, abym mógł wprowadzić poprawki kodu. Oczywiście jak blog tylko powstanie to dam Wam znać ;) Własne propozycje również mile widziane.

CMS jest na licencji MIT, a więc możecie robić z nim co tylko chcecie, ale byłoby miło gdybyście zgłosili własne ulepszenia bądź poprawki w kodzie.

Repozytorium na Githubie:
https://github.com/jpacanowski/MicroCMS

MicroCMS:
https://cms.whcn.xyz/
https://cms.whcn.xyz/?url=dashboard

Login: demo
Hasło: haslo1234

2
komentarz 11 grudnia 2017 przez xandros Nałogowiec (29,450 p.)
LF composer :V
2
komentarz 11 grudnia 2017 przez efiku Szeryf (75,160 p.)

PSR-2/1/4

Zbyt duże zależności warstw, baza, widok, model.

Tak trochę próbujesz odświeżyć kod, którego styl pisania wygląda na czasy php 5.3.

PS: W dokumentacji twiga jest jak można rozbić i includować odpowiednie bloki kodu niż całą stronę pakować do każdego szablonu.

Ja bym zaczął od modelu graficznego co jak ma wyglądać. 

komentarz 11 grudnia 2017 przez jpacanowski VIP (101,940 p.)
WIelkie dzięki chłopaki ;)

1 odpowiedź

+2 głosów
odpowiedź 11 grudnia 2017 przez Arkadiusz Waluk Ekspert (287,550 p.)
wybrane 11 grudnia 2017 przez jpacanowski
 
Najlepsza
  • Nieprzestrzegany PSR-2 (wygląd kodu). Nie jest to standard oficjalny, ale w PHP jest na tyle popularny, że dla wielu programistów jego użycie to oczywistość - dla mnie też.
  • https://github.com/jpacanowski/MicroCMS/tree/master/app/Twig - dobrze rozumiem, że tu jest wrzucony cały Twig? Composer to obecnie podstawa, jego użycie też powinno być oczywistością.
  • $content = trim($_POST['content']);

    A co jak ktoś nie prześle tego pola? W tablicy $_POST takiego elementu nie będzie, a więc będzie ostrzeżenie. Lepiej sprawdzić czy dana wartość istnieje przed użyciem, albo odczytywać przy użyciu filter_input().

  • header('Location: '.$_SERVER['HTTP_REFERER'].'#comments');

    Z tym refererem to jest różnie, przeglądarka może go nie przesłać. Wtedy jest problem z przekierowaniem, postawiłbym raczej na konkretny adres.

  • https://github.com/jpacanowski/MicroCMS/blob/master/app/controllers/Dashboard.php ta klasa jest bardzo duża, za duża. Jednocześnie odbiera i przygotowuje dane, waliduje, sprawdza metody HTTP... Rozbiłbym to wszystko na mniejsze klasy robiące konkretne rzeczy.

  • https://github.com/jpacanowski/MicroCMS/blob/master/app/controllers/Dashboard.php#L406 - jest kilka miejsce gdzie coś od tak sobie jest w całości zakomentowane, lepiej byłoby po prostu usunąć ten kod jeśli nie jest teraz potrzebny.

  • public function navigation($action = '', $itemId = '') {

    Nigdzie nie ma typów argumentów, nie ma typów zwracanych z funkcji, a PHP 7 już specjalną nowością nie jest - tu pytanie czy chcemy postawić na możliwość uruchomienia tego kodu byle gdzie, czy ma być to lepszy kod. Jeśli zostajemy przy takim zapisie, jaki jest do tej pory, skorzystałbym z phpDocumentora, aby to oznaczyć.

  • https://github.com/jpacanowski/MicroCMS/blob/master/app/controllers/Users.php#L35 - walidacja na masie warunków aż błaga o ulepszenie. Może wyjątki? Może lista reguł w jakiejś tablicy i robienie tego ręcznie? Może obiekt walidujący? Można pomyśleć, ale teraz jest chyba najgorsza możliwa opcja.

  •         catch(PDOException $e) {
                $this->error = $e->getMessage();
                echo $this->error;
            }

    Czy użytkownik powinien widzieć pełny błąd zwrócony przez bazę danych gdy coś pójdzie nie tak? Moim zdaniem nie.

  •         require_once '../app/controllers/' . $currentController . '.php';

    Tu znowu pomocny byłby Composer, a dokładniej autoloading przez PSR-4 po przestrzeniach nazw.

  • W widokach: czemu każda strona posiada swoją deklarację od zera, tj. cały tag html, head, mety itd.? Twig oferuje dziedziczenie, które jest bardzo dobre w takich sytuacjach - gdy trzeba coś zmienić robimy to raz, w szablonie bazowym.

Ze swojej strony poleciłbym dalszy rozwój zacząć od Composera, PSR-4 i PSR-2. A dalej może po prostu szukać tutoriali, przeglądać kod już napisanych aplikacji. Może troszkę zerkać na frameworki, część rozwiązań można dzięki nim zrozumieć, podpatrzeć (przynajmniej u mnie tak to wyglądało), choć też część rozwiązań trzeba znać.

Powodzenia

1
komentarz 11 grudnia 2017 przez jpacanowski VIP (101,940 p.)
Dzięki wielkie za odpowiedź i za poświęcony czas. Przeanalizuję sobie dokładnie to co napisałeś, a jest tego dużo, nadrobię braki i mam nadzieję, że mój kolejny kod w PHP będzie lepszy. W razie co, służę pomocą jak np. będziesz potrzebował zrobić jakiś frontend, a nie będziesz miał np. na to czasu.
komentarz 11 grudnia 2017 przez Arkadiusz Waluk Ekspert (287,550 p.)
Dzięki, miło. Mam nadzieję, że uda Ci się poduczyć i napisać lepszy kod.
komentarz 20 grudnia 2017 przez jpacanowski VIP (101,940 p.)
edycja 20 grudnia 2017 przez jpacanowski

Nieprzestrzegany PSR-2 (wygląd kodu). Nie jest to standard oficjalny, ale w PHP jest na tyle popularny, że dla wielu programistów jego użycie to oczywistość - dla mnie też.

Tu jak najbardziej rozumiem, i co nieco spróbuję poprawić, chociaż z niektórymi rzeczami się nie zgadzam, jak np. to, że elseif, czy else nie może być od nowej linii, tylko } elseif(...) {. Dla mnie to zaciemnia kod. Czytelne jest chyba tylko wtedy gdy zmienię TAB z 4 na 8 spacji. Z nazewnictwem się jak najbardziej zgadzam itp.

https://github.com/jpacanowski/MicroCMS/tree/master/app/Twig - dobrze rozumiem, że tu jest wrzucony cały Twig? Composer to obecnie podstawa, jego użycie też powinno być oczywistością.

Co w tym wypadku ten Composer zmieni? ;)

$content = trim($_POST['content']);

A co jak ktoś nie prześle tego pola? W tablicy $_POST takiego elementu nie będzie, a więc będzie ostrzeżenie. Lepiej sprawdzić czy dana wartość istnieje przed użyciem, albo odczytywać przy użyciu filter_input().

W Wordpressie wpisanie treści też nie jest wymagane ;) Wystarczy tylko podczas tworzenia nowego postu wprowadzić tytuł.

https://github.com/jpacanowski/MicroCMS/blob/master/app/controllers/Dashboard.php ta klasa jest bardzo duża, za duża. Jednocześnie odbiera i przygotowuje dane, waliduje, sprawdza metody HTTP... Rozbiłbym to wszystko na mniejsze klasy robiące konkretne rzeczy.

Jakbym miał tą klasę rozbić? Zrobić Dashboard_Part2Controller.php czy jak? Bo w ogóle nie wiem :D

https://github.com/jpacanowski/MicroCMS/blob/master/app/controllers/Dashboard.php#L406 - jest kilka miejsce gdzie coś od tak sobie jest w całości zakomentowane, lepiej byłoby po prostu usunąć ten kod jeśli nie jest teraz potrzebny.

Ja rozumiem, tylko kiedyś jak już będę wiedział jak to napisać dalej, bądź po prostu będzie mi się chciało dalej to napisać, to chcę wtedy ten kod odkomentować zamiast go usuwać.

Nigdzie nie ma typów argumentów, nie ma typów zwracanych z funkcji, a PHP 7 już specjalną nowością nie jest

A co daje definiowanie typów argumentów oraz typów zwracanych, skoro nie trzeba? Z resztą dziwny jest zapis -> : int

W widokach: czemu każda strona posiada swoją deklarację od zera, tj. cały tag html, head, mety itd.? Twig oferuje dziedziczenie, które jest bardzo dobre w takich sytuacjach - gdy trzeba coś zmienić robimy to raz, w szablonie bazowym.

W widoku Dashboardu tak właśnie mam, tylko że tam jest więcej niż 2 pliki .html

Oczywiście to nie są złośliwe pytania. Z większością z tych rzeczy nie potrafię sobie poradzić... Chodzi o to, że coś mam zmienić i nie wiem do końca czemu... ;(

komentarz 20 grudnia 2017 przez Arkadiusz Waluk Ekspert (287,550 p.)

Tu jak najbardziej rozumiem, i co nieco spróbuję poprawić, chociaż z niektórymi rzeczami się nie zgadzam, jak np. to, że elseif, czy else nie może być od nowej linii, tylko } elseif(...) {. Dla mnie to zaciemnia kod. Z nazewnictwem się jak najbardziej zgadzam itp.

Jak kto lubi, dla mnie PSR-2 jest czytelny - może to też kwestia przyzwyczajenia. 

Co w tym wypadku ten Composer zmieni?

Wszystko. Nie będziesz miał w repozytorium tych wszystkich plików Twiga a tylko jeden plik composer.json (ewentualnie drugi composer.lock). Na podstawie tego jednego pliku jednym poleceniem Composera będzie można pobrać zdefiniowane w composer.json rzeczy, np. właśnie Twiga. Dołączanie tego do kodu też nie wymaga specjalnego wysiłku, wystarczy jeden plik - o resztę dba autoloading generowany przez Composera.

W Wordpressie wpisanie treści też nie jest wymagane ;) Wystarczy tylko podczas tworzenia nowego postu wprowadzić tytuł.

A co to ma do tego? Nie narzucam co jest wymagane, zwracam tylko uwagę, iż ktoś może danego pola do skryptu nie przesłać. Nikt nie powiedział, że ktoś np. nie spróbuje dowolnym klientem HTTP wywołać Twój skrypt przesyłając tylko jeden z parametrów. A ty nie sprawdzając czy on istnieje czy nie próbujesz się do niego odwołać, co powinno spowodować wyjątek o nieznanym elemencie tablicy. I tylko o to mi chodziło.

Jakbym miał tą klasę rozbić? Zrobić Dashboard_Part2Controller.php czy jak? Bo w ogóle nie wiem :D

Jak dla mnie podzielić na mniejsze części odpowiadające za dane rzeczy. Masz tu walidację, to można wydzielić jako osobna klasa/klasy tylko do tego. Masz przygotowanie danych (trim itp.), to też można jakoś wydzielić. Z resztą całe akcje można wydzielić, nie muszą (a nawet nie powinny) być w kontrolerach. Można zrobić coś w rodzaju serwisów, które będą wykonywały określone rzeczy. Tu naprawdę wiele zależy od pomysłu i podejścia.

Ja rozumiem, tylko kiedyś jak już będę wiedział jak to napisać dalej to chcę wtedy ten kod odkomentować zamiast go usuwać.

Ja też rozumiem, ale mi jako czytającemu to zaciemnia kod - patrzę i nie wiem czy to tam potrzebne, czy ktoś coś testował, czy zapomniał to usunąć, a może zapomniał odkomentować i część aplikacji się wysypie? Nie wiadomo, z pozycji czytającego kod to jest złe.

A co daje definiowanie typów argumentów oraz typów zwracanych, skoro nie trzeba? Z resztą dziwny jest zapis -> : int

To że czegoś nie trzeba robić to nie znaczy, że nie warto. Jak dla mnie zwiększa czytelność kodu - od razu widzę, co dana funkcja ma zwrócić, albo jakiego argumentu oczekuje. Bazuje też na tym IDE przy sugerowaniu składni. No i jest to zabezpieczenie - gdy określisz argument jako obiekt klasy X to nie podasz tam obiektu klasy Y (spowoduje to błąd).

W widoku Dashboardu tak właśnie mam, tylko że tam jest więcej niż 2 pliki .html

To znaczy? Dalej wydaje mi się, że powinien być jeden plik bazowy i wszystkie dziedziczące po nim. Chyba że faktycznie jest potrzeba, aby inna podstrona była całkowicie inna od pierwszej to rozumiem.

komentarz 20 grudnia 2017 przez jpacanowski VIP (101,940 p.)
edycja 20 grudnia 2017 przez jpacanowski

Nie będziesz miał w repozytorium tych wszystkich plików Twiga a tylko jeden plik composer.json

A to tak działa... teraz już wszystko rozumiem i widzę w tym zaletę...

A co to ma do tego? Nie narzucam co jest wymagane, zwracam tylko uwagę, iż ktoś może danego pola do skryptu nie przesłać.

Wybacz, zrozumiałem to wcześniej inaczej...

Ja też rozumiem, ale mi jako czytającemu to zaciemnia kod - patrzę i nie wiem czy to tam potrzebne, czy ktoś coś testował, czy zapomniał to usunąć, a może zapomniał odkomentować i część aplikacji się wysypie? Nie wiadomo, z pozycji czytającego kod to jest złe.

Ja też ciebie całkowicie rozumiem, ale czy to jakaś moda na nieużywanie już w ogóle komentarzy w kodzie? Bo już nawet słyszałem nie raz żeby nie dodawać do kodu komentarzy, bo dobrze napisany kod mówi sam za siebie co robi. I to ok, rozumiem. Ale zakomentowywanie to też źle? O_o Napisałeś, że to może innym czytającym zaciemniać kod, czy w takim razie jest jakaś reguła, żeby nie commitowało zakomentowanego kodu?

Dzięki... To nie był atak, chociaż może tak wyglądać. Z tym definiowaniem typów też nie wiedziałem, jeszcze mi się nie zdarzyło zwrócić coś innego typu niż zwraca sama funkcja. A z tą czytelnością to i możliwe, chociaż taki JS jakoś żyje i jego programiści :D

komentarz 20 grudnia 2017 przez Arkadiusz Waluk Ekspert (287,550 p.)
Spoko.

Co do komentarzy to każdy głosi jakieś swoje teorie. Ja tam nie jestem za całkowitym pozbyciem się ich tylko racjonalnym używaniem - gdy faktycznie trzeba coś ważnego opisać. Albo aby oznaczyć szybkie TODO, abym nie zapomniał tego za jakiś moment zrobić, a jest to za dużo na tworzenie taska - choć komentarzy TODO też podobno powinno się unikać. Nie znam żadnej reguły gita na pomijanie fragmentów pliku - możesz za to coś usunąć, zacommitować i w razie potrzeby zrobić revert.

Podobne pytania

0 głosów
0 odpowiedzi 333 wizyt
pytanie zadane 30 maja 2020 w PHP przez Mr. PanKrok Nowicjusz (230 p.)
0 głosów
3 odpowiedzi 324 wizyt
pytanie zadane 29 lipca 2018 w PHP przez User007 Bywalec (2,400 p.)
+1 głos
0 odpowiedzi 260 wizyt
pytanie zadane 9 kwietnia 2021 w PHP przez Lopus Początkujący (360 p.)

92,452 zapytań

141,262 odpowiedzi

319,080 komentarzy

61,854 pasjonatów

Motyw:

Akcja Pajacyk

Pajacyk od wielu lat dożywia dzieci. Pomóż klikając w zielony brzuszek na stronie. Dziękujemy! ♡

Oto polecana książka warta uwagi.
Pełną listę książek znajdziesz tutaj.

Akademia Sekuraka

Akademia Sekuraka 2024 zapewnia dostęp do minimum 15 szkoleń online z bezpieczeństwa IT oraz dostęp także do materiałów z edycji Sekurak Academy z roku 2023!

Przy zakupie możecie skorzystać z kodu: pasja-akademia - użyjcie go w koszyku, a uzyskacie rabat -30% na bilety w wersji "Standard"! Więcej informacji na temat akademii 2024 znajdziecie tutaj. Dziękujemy ekipie Sekuraka za taką fajną zniżkę dla wszystkich Pasjonatów!

Akademia Sekuraka

Niedawno wystartował dodruk tej świetnej, rozchwytywanej książki (około 940 stron). Mamy dla Was kod: pasja (wpiszcie go w koszyku), dzięki któremu otrzymujemy 10% zniżki - dziękujemy zaprzyjaźnionej ekipie Sekuraka za taki bonus dla Pasjonatów! Książka to pierwszy tom z serii o ITsec, który łagodnie wprowadzi w świat bezpieczeństwa IT każdą osobę - warto, polecamy!

...