[SKRYPT] Ocena skryptu do dodawania/usuwania/edytowania artykułów na blogu |
[SKRYPT] Ocena skryptu do dodawania/usuwania/edytowania artykułów na blogu |
15.06.2016, 14:31:44
Post
#1
|
|
Grupa: Zarejestrowani Postów: 1 Pomógł: 0 Dołączył: 15.06.2016 Ostrzeżenie: (0%) |
Witam, dopiero co uczę się programować obiektowo, jest te mój pierwszy skrypt napisany tym sposobem. Będę wdzięczny za konstruktywną krytykę, będę wrzucał poprawione fragmenty kodu właśnie tutaj.
Obsługa artykułów articleClass.php
Obsługa bazy danych db.php
|
|
|
15.06.2016, 14:37:17
Post
#2
|
|
Grupa: Zarejestrowani Postów: 1 240 Pomógł: 278 Dołączył: 11.03.2008 Ostrzeżenie: (0%) |
Niewiele ma to wspólnego z OOP, poza tym, że użyłeś klas Dużo nauki przed Tobą.
DbAction całkowicie bez sensu, już lepiej korzystać z czystego mysqli/PDO. Wyobraź sobie teraz, że artykuł posiada dodatkowe pole tekstowe o nazwie "poleTekstowe" - ilość kodu do zmiany jest bardzo bardzo duża w twoim "skrypcie". -------------------- |
|
|
15.06.2016, 15:13:21
Post
#3
|
|
Grupa: Zarejestrowani Postów: 6 365 Pomógł: 1114 Dołączył: 30.08.2006 Ostrzeżenie: (0%) |
Zobacz przykładowo https://github.com/RalfEggert/zend-expressi...part6/src/Album
Różnica taka że wszystko jest podzielone według zadań jakie ma pełnić. A zastosować można do dowolnych danych w tym takiego artykułu. -------------------- |
|
|
15.06.2016, 15:30:14
Post
#4
|
|
Grupa: Zarejestrowani Postów: 160 Pomógł: 27 Dołączył: 22.09.2008 Skąd: Tarnów Ostrzeżenie: (0%) |
Po 1. Komentarz zbędny. Po 2. Jeżeli jedna klasa ma dwie odpowiedzialności (robi dwie rzeczy), to rozbij ją na dwie klasy, które zajmują się jedną rzeczą. Po 3. Nie uzupełniaj kodu komentarzami, które opisują coś, co już zostało opisane kodem. Po 4.
Nie lepiej użyć autoloadera? Po 5. Brzydko formatujesz kod. -------------------- |
|
|
15.06.2016, 17:21:00
Post
#5
|
|
Grupa: Zarejestrowani Postów: 697 Pomógł: 47 Dołączył: 19.12.2003 Skąd: Lublin Ostrzeżenie: (0%) |
1. Pomijając to, że kod jest bez sensu mnie najbardziej bolą konstruktory, które są kompletnie zbędne. Wartość domyślną możesz określić przy tworzeniu zmiennej
2. Nie mieszałbym typów. Skoro masz articleId, to domyślam się, że będzie tam liczba. Bez sensu, że przypisujesz do tej zmiennej string. 3. Staraj się używać === zamiast == 4. Najlepiej wydrukuj ten kod i spal w kominku -------------------- Warsztat: Kubuntu, PhpStorm, Opera
|
|
|
15.06.2016, 18:55:32
Post
#6
|
|
Grupa: Zarejestrowani Postów: 3 033 Pomógł: 366 Dołączył: 24.05.2012 Ostrzeżenie: (0%) |
SHiP A kto Ci powiedział, że jakiekolwiek id to musi być liczba. Tu zapewne jest wiec dobrze zwrócić na to uwagę, ale nie kategoryzujmy tak
Co do autora to niestety sporo pracy przed tobą, jeśli trzymamy się podawania zasięgu metod to wszędzie, dla konstruktora również, nie ważne że jest domyślnie public Poczytaj o standardach w php czyli PSR
Coś takiego będzie generować noticy, a poza tym jest bez sensu bo klasę uzależniłeś od jednej słusznej implementacji, a nie o to w obiektowości chodzi |
|
|
15.06.2016, 20:09:25
Post
#7
|
|
Grupa: Zarejestrowani Postów: 697 Pomógł: 47 Dołączył: 19.12.2003 Skąd: Lublin Ostrzeżenie: (0%) |
@com: masz rację . Złą zmienną wybrałem. Po prostu zmyliło mnie to, że wszystkie mają przypisywane puste stringi. Wymieniam na $numRows
Ten post edytował SHiP 15.06.2016, 20:10:21 -------------------- Warsztat: Kubuntu, PhpStorm, Opera
|
|
|
15.06.2016, 20:21:00
Post
#8
|
|
Grupa: Zarejestrowani Postów: 520 Pomógł: 102 Dołączył: 15.07.2014 Skąd: NULL Ostrzeżenie: (0%) |
Mi się nie podoba:
* echo w metodach. Według mnie lepiej jakbyś to zwracał (return) i dopiero tam gdzie wywolujesz metodę dodawał przed nią echo lub i nie. * Poczytaj o SQL Injection oraz "Prepared statements".
* sprawdzaj jakoś czy zmienne istnieją przed przypisaniem bo mogły nie być wysłane ( isset )
Ten post edytował KsaR 15.06.2016, 20:24:18 -------------------- |
|
|
15.06.2016, 21:38:48
Post
#9
|
|
Grupa: Zarejestrowani Postów: 3 033 Pomógł: 366 Dołączył: 24.05.2012 Ostrzeżenie: (0%) |
Cytat sprawdzaj jakoś czy zmienne istnieją przed przypisaniem bo mogły nie być wysłane KsaR a właśnie że nie, tego wgl tam ma nie być Ale generalnie to się zgadzam, ze trzeba sprawdzać SHiP jasne rozumiem |
|
|
22.06.2016, 08:40:43
Post
#10
|
|
Grupa: Zarejestrowani Postów: 148 Pomógł: 14 Dołączył: 23.02.2013 Ostrzeżenie: (0%) |
1. Nie podobają mi się komentarze, ich umiejscowienie, zbędność. Przede wszystkim komentarze po bloku metody, są one zbędne, a jeśli musisz je mieć żeby wiedzieć czego dotyczy dana klamerka to oznacza to że metoda jest zbyt długa by ją ogarnąć na jednym ekranie, jednym rzutem oka.
2. Klasa DbAction mogłaby być jakimś wrapperem na PDO, w środku produkującym prepared statement, lub zostać zastąpiona po prostu samym PDO. Niezależnie od tego jaką drogę byś wybrał to dobrze by było ją w jakiś sposób wstrzyknąć, czy to w konstruktorze, czy w samej metodzie, tak by odseparować się od tej zależności. -------------------- |
|
|
Wersja Lo-Fi | Aktualny czas: 10.05.2024 - 00:12 |