Drukowana wersja tematu

Kliknij tu, aby zobaczyć temat w orginalnym formacie

Forum PHP.pl _ Oceny _ [www]Proste forum - PHP bez frameworków.

Napisany przez: smk 11.07.2018, 16:01:01

Cześć.
Prosiłbym Was o opinię kodu, do którego link podrzucam niżej. Jest to proste forum - nie ma tu raczej jakiś rozbudowanych rzeczy, bo jest to mój pierwszy projekt w PHP, ale nawet pisząc takie coś, poznałem wiele nowych zagadnień wink.gif Chciałbym dowiedzieć się co należy poprawić w przyszłości. Jest to projekt bez frameworków do php. Struktura - brak podfolderów leży, ale nie chciało mi się już z tym mieszać smile.gif Poza tym wydaje mi się, że trochę pomieszałem rozbicie funkcji na klasy - na przykład komentarze w klasie postów. Dlatego dobrze gdybyście podpowiedzieli mi, co warto zmienić na przyszłość wink.gif

https://github.com/selfmadeking/simple-forum-php

oraz podgląd jak to działa (trzeba założyć konto, dowolny e-mail jakiś login i hasło (szyfrowanie password_hash hehe), aby mieć dostęp do forum - swoje posty, komentarze, sygnaturę można edytować):

https://imlimitless.000webhostapp.com/index.php

Dodatkowe pytanie - czy Waszym zdaniem to już czas, żeby próbować pisać coś z frameworkiem w php np. Symfony? Czy jeszcze coś napisać? Macie pomysł co? smile.gif

Pozdrawiam i krytykujcie jak się tylko da! smile.gif

Napisany przez: nospor 11.07.2018, 16:42:24

Klasa nie ma prawa mi robic zadnego echo. Leci wyjatek to leci.

Takie potworki jak ten
if(isset($_SESSION['user_session']))
{
return true;
}
else{
return false;
}

zapisuje sie poprostu tak
return isset($_SESSION['user_session']);
Nie ma sensu tworzyc 1000 lini jak mozna 10

Naucz sie oddzielac logike od wygladu, akcje do widoku. MVC sie klania.

Wypadaloby juz pisac nowe rzeczy w php7

Funkcja nie moze zwracac albo tekstu albo false


Jak post ma milion odpowidzi to skrypt ci padnie. Poczytaj o stronicowaniu

thePosts - co to za nazwa funkcji? Za cholere nie wiem co to ma niby robic po nazwie

Plik db.php a ty tam inicjalizujesz forum temat post i usera... eeee?

Ten kod to generalnie sieczka do poprawy. Jedyne na plus ze jakos ustrzegles sie przed banalnymi dziurami.

edit:a nie, cofam ostatnie. Plik podatny na banalny XSS

edit2:
https://imlimitless.000webhostapp.com/topic.php?id=100000
Topic not found.
a pod spodem
Posts not found for this topic.
No jak nie ma tematu to i nie dziwota ze nie ma postow. Do etapu pobierania postow gdy nie ma tematu nie powinno w ogole dochodzic

edit x:
if($content == "" || strlen($content) == 0 || !$content)
a moze poprostu
if(empty($content))
? Prawda ze krocej i bardziej czytelniej?

Cytat
Dodatkowe pytanie - czy Waszym zdaniem to już czas, żeby próbować pisać coś z frameworkiem w php np. Symfony? Czy jeszcze coś napisać? Macie pomysł co?
Napisac? Nie. Douczyc, TAK!

edit x
foreach($info as $info)
ouch

<?php echo $info; ?>
Juz od dawna
<?= $info ?>
jest standardem

Takie kody tez nie maja zadnego sensu:
if($stmt)
{
return true;
}
else
{
return false;
}
bo jak zapytanie sie nie powiedzie na jakims etapie to poleci poprosty wyjatek i do tego IFa nie dojdzie w ogole. Wiec wystarczy samo:
return true;

I wlacz raportowanie wszystkich bledow bo po kodzie odnoasze wrazenie ze odpalasz bez tego

Napisany przez: Pyton_000 11.07.2018, 16:50:55

Powiem ta. Nie jest tragicznie. Jest słabo ale nie tragicznie.

Dodam od siebie kilka uwag ogólnych bo o kodzie to można godzinami gadać. No to lecimy:
- PHP7 - to już powinna być podstawa. Ja wiem że bardzo dużo (90%) kursów jest na podstawie PHP5 no ale warto poszukać w necie na zasadzie "Co nowego w PHP7"
- namespace - to już jest od bardzo dawna standard. Pozwala Ci logicznie uporządkować kod.
- struktura plików i konwencje nazwenictwa. - Poczytaj sobie o PSR-0 i PSR-2 bo bardzo ułatwi Ci to pracę. Nie trzymaj wszytskich plików w 1 katalogu. Rozdzielaj je logicznie.
- Nie mieszamy HTML z PHP.
- Używaj spacji jako indentacji. Ja wiem że gówno-burza zaraz z tego wyjdzie ale jednak spacja to spacja - wszędzie wygląda tak samo.
- Staraj się eliminować zagnieżdżenia oraz zbędne instrukcje IF np:

Kod
if($stmt->rowCount() > 0)
            {
                return true;
            }
            else
            {
                return false;
            }

Na:
Kod
if($stmt->rowCount())
{
    return true;
}
return false;


- Dobrze że używasz PDO
- używaj Unsigned w kolumnach tam gdzie nie przewidujes wartości ujemnych.
- W metodach staraj się stosować zasadę odwróconej logiki. Chodzi o to że zamiast sprawdzać czy warunek jest OK i umieszczać całą logikę w bloku warto sprawdzić najpierw czy NIE jest spełniony. Przykład:
Zamiast
Kod
if(count($comments) > 0)
{
    foreach($comments as $comment)
    {
        $content = $comment['comment_content'];
        $user = $comment['user_name'];
        $comment_id = $comment['comment_id'];
        $dateConverted = new DateTime($comment['comment_date']);
        $date = ' | <span class="comment-date">' . $dateConverted->format('d.m.Y H:i') . '</span>';
        $html.= '<div class="comment">';
        $html.= '<p>' .  $content;
        $html.= $date . ' <a href="#">' . $user .'</a></p>';
        if($_SESSION['user_session'] === $comment['user_id'])
        {
            $html.= '<a href="deleteComment.php?id=' . $comment_id . '" title="Delete comment"><i class="fas fa-trash-alt comment-i"></i></a>';
            $html.= '<a href="editComment.php?id=' . $comment_id . '" title="Edit comment"><i class="fas fa-edit comment-i"></i></a>';
        }
        $html.= '</div>';
    }
return $html;
}
else
{
    return false;
}


Lepiej:
Kod
if(count($comments) === 0)
{
    return false;
}

foreach($comments as $comment)
{
    $content = $comment['comment_content'];
    $user = $comment['user_name'];
    $comment_id = $comment['comment_id'];
    $dateConverted = new DateTime($comment['comment_date']);
    $date = ' | <span class="comment-date">' . $dateConverted->format('d.m.Y H:i') . '</span>';
    $html.= '<div class="comment">';
    $html.= '<p>' .  $content;
    $html.= $date . ' <a href="#">' . $user .'</a></p>';
    if($_SESSION['user_session'] === $comment['user_id'])
    {
        $html.= '<a href="deleteComment.php?id=' . $comment_id . '" title="Delete comment"><i class="fas fa-trash-alt comment-i"></i></a>';
        $html.= '<a href="editComment.php?id=' . $comment_id . '" title="Edit comment"><i class="fas fa-edit comment-i"></i></a>';
    }
    $html.= '</div>';
}
return $html;

Prawda że czytelniej? smile.gif

To tak na razie. + to co @nospor napisał odnośnie redukowania warunków (tam gdzie sprawdzasz czy pusty itp.)

Napisany przez: nospor 11.07.2018, 16:55:34

Cytat
Powiem ta. Nie jest tragicznie. Jest słabo ale nie tragicznie.
Przed tragicznie ratuje go tylko PDO i bindowanie zmiennych. Gdyby nie to....

Napisany przez: Pyton_000 11.07.2018, 19:10:38

Nie przesadzaj. Widzieliśmy tu dużo więcej gorszego kodu. Ten trzyma się jako tako. Jak na początkującego albo młodego Juniora to nie jest źle smile.gif

Napisany przez: markonix 11.07.2018, 23:01:04

Cytat(Pyton_000 @ 11.07.2018, 17:50:55 ) *
- Staraj się eliminować zagnieżdżenia oraz zbędne instrukcje IF np:
Kod
if($stmt->rowCount() > 0)
            {
                return true;
            }
            else
            {
                return false;
            }

Na:
Kod
if($stmt->rowCount())
{
    return true;
}
return false;


  1. return $stmp->rowCount() > 0;

albo:
  1. return (bool)$stmp->rowCount();

smile.gif

Napisany przez: Pyton_000 12.07.2018, 08:14:14

Yup smile.gif jak najbardziej zalecane smile.gif

Napisany przez: Pilsener 12.07.2018, 08:20:16

Jak dla mnie bardzo słabo, podstawowy problem jak tu widzę, to brak celu.
Pisać takie coś, ale po co? By się nauczyć, jak się pisało skrypty PHP 20 lat temu, bez OOP, frameworków i bibliotek?
Nie ma sensu pisać takich rzeczy, bo nie dość, że się niczego nie nauczysz to jeszcze nabierzesz złych nawyków i potem trzeba Ciebie będzie oduczyć.
I nie chodzi tu o szczegóły typu:

  1. else if($user_email == "")
  2. {
  3. $info[] = "Provide e-mail!";
  4. }
  5. else if(!filter_var($user_email, FILTER_VALIDATE_EMAIL))
  6. {
  7. $info[] = "Enter a valid e-mail!";
  8. }
  9. else if($user_pass == "")
  10. {
  11. $info[] = "Provide password!";
  12. }

  1. if($user->login($user_name, $user_email, $user_pass))
  2. {
  3. http://www.php.net/echo '<meta http-equiv="refresh" content="0; url=home.php">';
  4. }
  5. else
  6. {
  7. $error[] = "Wrong login or password. Try again!";
  8. }


Tylko o podejście do projektowania aplikacji. Nawet 20 lat temu sposób myślenia był często taki:
1. Odbiorę i sprawdzę request
2. Pobiorę i przygotuje odpowiednie dane z bazy
3. Przygotuje odpowiedź - dokument HTML, 404 etc.
4. Wyślę to do przeglądarki

Tworząc apkę internetową trzeba najpierw się zastanowić, jakich użyć technologii i dlaczego a jeśli nie PHP 7.2 OOP to trzeba mieć dobry argument dlaczego.
Potem trzeba sobie zaprojektować szkielet aplikacji i przemyśleć takie rzeczy jak:
- error handler i narzędzia deweloperskie
- konfiguracja + środowiska (deweloperskie, produkcyjne etc.)
- obsługa bazy danych (ORM, gołe PDO, inny?)
- dołączanie bibliotek zewnętrznych (composer, yarn etc)
- i wiele innych

Jak chcesz się czegoś nauczyć praktycznego to dołącz do jakiegoś zespołu programistów. W pół roku nauczysz się więcej niż robiąc samemu latami.

Napisany przez: smk 12.07.2018, 11:01:30

Okej. A więc dziękuję Wam za solidne odpowiedzi. Widzę co robiłem tragicznie, co należy przerobić. Pilsener - wybacz, ale to mój pierwszy "większy" projekt w php i raczej nie jestem w stanie myśleć, tak jak Ty o środowiskach ... jakich? smile.gif i całej reszcie, ponieważ po prostu tego jeszcze nie poznałem. Mam się za takie rzeczy zabierać, znając ledwo podstawy składni php? Gdzie nawet teraz widać braki ("<?php echo" zamiast "<?= "). Od czegoś trzeba zacząć. Napisałem takie coś, kod jestem świadom, że jest mocno słaby, ale starałem się, czytałem różne tematy i próbowałem napisać to w miarę dobrze wink.gif Oby tak dalej i będzie lepiej.

Ktoś dobrze wspomniał, że wiele poradników, fragmentów kodu dostępnych w Internecie jest słabej jakości (np. mysqli w 2018 roku). Już nawet rejestracja i logowanie było trudne do napisania dla mnie, bo przeszukałem z 50, jak nie więcej poradników (głównie w języku angielskim) i dopiero znalazłem jeden w miarę solidny poradnik, który wykorzystywał password_hash i w miarę dobre praktyki.. No nic, spisuję swoje błędy i zabieram się do dalszej nauki.
Na pewno do usłyszenia, bo dzięki takiej ocenie kodu wiem co robię źle i mogę poprawiać swoje błędy w przyszłości wink.gif

Pozdrawiam i dziękuję Wszystkim za pomoc!

Napisany przez: Pilsener 13.07.2018, 08:56:56

Cytat
i raczej nie jestem w stanie myśleć, tak jak Ty
- jesteś jesteś, tylko musisz skierować wysiłek i energię na nieco inne tory.
Cytat
Mam się za takie rzeczy zabierać, znając ledwo podstawy składni php?
- tak, bo w programowaniu może pare procent to składnia, reszta to sposób myślenia, wzorce, narzędzia, metodyka etc. Co z tego, że będziesz znał super składnie i nawet cytował manuala z pamięci jak wywalisz się przy zaprojektowaniu nawet niewielkiej biblioteki? I nie musisz od razu zabierać się za szkielet aplikacji webowej, można zacząć od części a nawet czegoś nie związanego z architekturą (np. generator menu i breadcrumb).
Cytat
Gdzie nawet teraz widać braki ("<?php echo" zamiast "<?= ")
- takimi detalami na początku nie ma sensu się przejmować, jeśli przyłożysz się do struktury to i z czasem sam kod też się poprawi.

I zanim coś się zacznie trzeba mieć jakiś plan, pomysł, rozrysować sobie na papierze jakich obiektów będę potrzebował, jak to będzie współgrać ze sobą, potem jak to przenieść na klasy itp. itd. etc.
Zamiast powtarzać n razy "mysl_query() or die()" lepiej spróbować zrobić coś skromniejszego, ale lepiej przemyślanego, skupić się na logice działania a nie na obsłudze jak największej liczby żądań.

Napisany przez: Tomplus 13.07.2018, 14:11:42

Warto korzystać z katalogów:

taki plik jak:
class.post.php lepiej będzie mu w /class/Post.php

Wtedy możesz użyć autoloader i który będzie szukał klas tylko w jednym katalogu i jego podfolderach.

Dosyć normalne jest, chociaż zależy od hostingu, aby rozdzielić część kodu wykonawczego od dostępu przez HTTP i tworzy się wtedy katalog /public/ pod który jest podpinana domena/subdomena. A w nim wszystkie style .css, wszystkie obrazy i skrypty .js, tylko jeden index.php wskazujący pliki przed katalogiem /public/
Dzięki temu nikt np. przypadkiem nie podejrzy pliku: database.sql w którym będzie np. domyślnie ustawione hasło admina tongue.gif


Jeszcze hosting: najtańszy hosting będzie 1000x lepszy od tego bezpłatnego.
Polecam tutaj: https://linuxpl.com/ za 50zł spokojnie wystarczy.

A ma dostęp do SSH (czyli możesz łączyć się z githubem i aktualizować kod strony na serwerze) (zamiast FTP)
Przestrzeni także wystarczy na dużo, dużo projektów.

Powered by Invision Power Board (http://www.invisionboard.com)
© Invision Power Services (http://www.invisionpower.com)