Witam! Napisałem swój pierwszy pełny skrypt pomijając oczywiście wcześniejsze zabawy w pisanie krótkich kodów php na 50 linijek. Próbowałem stosować się do wszystkich standardów php, html, css.

Skrypt skierowany jest dla posiadaczy domowego kina multimedialnego opartego o program XBMC i posiadanej bazy filmów. Ma on za zadanie parsować pliki xml dostarczone z tegoż programu wraz z grafiką, uporządkować je w bazie danych i wyświetlić ładnie wyglądającą prezentację.

Proszę Was o uwagi odnośnie samego skryptu bo to jest dla mnie najważniejsze aby na początku oduczyć się złych nawyków i zdobywać poprawną wiedzę w pisaniu w php. Oczywiście na temat wyglądu uwagi też mile widziane.

strona główna:
http://regss.sk1.pl/test/index.php

strona panelu administratora:
http://regss.sk1.pl/test/panel.php

hasło do panelu: test

Śmiało można usuwać, dodawać, importować. (celowo na potrzeby testów wyłączyłem usuwanie plików po udanym imporcie).

A oto kod źródłowy:

google code

paczka z kodem: ZIP

Pozdrawiam.

Nieźle, tylko jak już używasz html5, to używaj tagów jakie daje, a nie pakujesz wszytko w div, umieść też ten kod JS w osobnym pliku.
Jak zmieniasz film, to tło jest ładowane tzn widać to. Zrób to tak, aby było już w cache przeglądarki i pokazywało się jako fade?

1) config.php wygląda jak WYMAGANY plik (w końcu bez configu raczej strona nie pójdzie). Czemu to jest dołączane za pomocą include a nie require?

2) function.php to nie jest dobra nazwa

3) Gdybyś skorzystał z np. Doxygena do komentarzy, to miałbyś już dokumentację projektu.

login.php

[PHP] pobierz, plaintext 
    unset($_SESSION['id']);
    session_destroy();
[PHP] pobierz, plaintext 

Pierwsze unset jest zbędne.

[PHP] pobierz, plaintext 
// Logout
if (isset($_GET['logout'])) {
    unset($_SESSION['id']);
    session_destroy();
    header('location:index.php');
}
 
// Login panel
if (!isset($_POST['pass'])) {
    $content_output = '<form action="login.php" method="POST">' . $lang['l_panel_pass'] . ':<br/><input id="l_pass" name="pass" type="password" /></form>';
} else {
    if ($_POST['pass'] === $panel_pass) {
        $_SESSION['id'] = md5($_SERVER['REMOTE_ADDR']) . md5($panel_pass);
        header('location: panel.php');
    } else {
        $content_output = $lang['l_panel_wrong'] . '. <a href="login.php">' . $lang['l_panel_again'] . '</a>';
    }
}
[PHP] pobierz, plaintext 

W bloku login panel dałabym elseif - aktualnie sprawdza isset($_GET['logout']), a potem zawsze sprawdza !isset($_POST['pass']), a podejrzewam, że te dwie rzeczy wykluczają się.

config.php
Zabrakło mi zmiennej configuracyjnej ustalającej port do bazy. Jasne, można wepchnąć w host, natomiast lepiej IMO byłoby rozdzielić.

[PHP] pobierz, plaintext 
$perPage = 30; // Movies per page, If you do not want to have pagination, type a larger number than the number of movies
[PHP] pobierz, plaintext 

If you do not want to have a pagination type 0 - standardowo 0 znosi wszelkie limity

[PHP] pobierz, plaintext 
 * Don't edit nothing below this line!#
[PHP] pobierz, plaintext 

Do not edit anything...
Podwójne zaprzeczenie nie istnieje w angielskim

panel.php

[PHP] pobierz, plaintext 
    if ($_GET['option'] == 'del_all') {
        $content_output = delete_table($table_name, $lang);
    }
    if ($_GET['option'] == 'create_new') {
        $content_output = create_table($table_name, $lang);
    }
    if ($_GET['option'] == 'xml_file_info') {
        $content_output = xml_file_info($xml_file, $lang, $detect_encoding);
    }
    if ($_GET['option'] == 'nfo_file_info') {
        $content_output = nfo_file_info($table_name, $lang, $detect_encoding);
    }
    if ($_GET['option'] == 'xml_import') {
        $content_output = import_xml($xml_file, $table_name, $lang, $detect_encoding);
    }
    if ($_GET['option'] == 'clear_cache') {
        $content_output = clear_cache($lang);
    }
[PHP] pobierz, plaintext 

Aż się prosi o switch zamiast if.
Nie mówiąc już o tym, że chyba bardzo nie lubisz else. Jeśli $_GET['option'] będzie równe del_all, to skrypt zrobi sprawdzenie wszystkiego, mimo że dopasował się do pierwszego warunku. A warunki są rozłączne.

[PHP] pobierz, plaintext 
  $sql_movies = 'SELECT id FROM ' . $table_name;
    $result_movies = mysql_query($sql_movies);
    $table_rows = '<span class="green">' . mysql_num_rows($result_movies) . '</span>';
[PHP] pobierz, plaintext 

SELECT COUNT(id)
mysql_num_rows jest przydatne wtedy, gdy odpalasz normalne zapytanie i dodatkowo chcesz policzyć ile elementów zwróciło;
Jeśli zależy Ci tylko na liczbie - COUNT jest bardziej wydajne.

index.php

[PHP] pobierz, plaintext 
if ((!mysql_query('SELECT id FROM ' . $table_name . ' LIMIT 1')) or (mysql_num_rows(mysql_query('SELECT id FROM ' . $table_name . ' LIMIT 1')) < 1)) {
        header('Location: panel.php');
}
 
// sets the variables
if (!isset($_GET['id'])) {
    $id_result = mysql_query('SELECT id FROM ' . $table_name . ' LIMIT 1');
[PHP] pobierz, plaintext 

Trzy razy wykona się pobieranie toczka w toczkę tego samego zapytania.
Co prawda podejrzewam, że danych w bazie będzie bardzo niewiele, natomiast jest to zły nawyk.
Zrób mysql_query raz przed tymi wszystkimi ifami.

Zastanawiam się również czy koniecznie potrzebujesz regexpów w zapytaniach:

[PHP] pobierz, plaintext 
$genre_mysql = $val;
[PHP] pobierz, plaintext 

Gdzie val wydaje się pochodzić z zapytania z tabeli dotyczącej gatunków filmów. Na szybki rzut oka nie ma tam nic dotyczącego konieczności użycia regexpa. Like mysqlowy, mimo że tragicznie wolny jest dużo szybszy od regexpów.

To tak ogólnie. Na pierwszy rzut oka. Mam nadzieję, że co nieco Ci się z tego przyda.

Na początku chciałem podziękować za poświęcony czas i analizę kodu. Na pewno porady bardzo się przydadzą, zabieram się do usprawniania kodu. Po przeanalizowaniu każdej sugestii pozostaje mi zgodzić się ze wszystkimi, muszę podszlifować jeszcze logiczne myślenie. Nawet mały kurs języka angielskiego mi się trafił.

Co do regexp przeczytałem gdzieś, że jest szybszy i dlatego go stosowałem okazuje się jednak, że nie zawsze jest to dobre wyjście.

Dlaczego nazwa funcion.php to zły pomysł?

!*!, chodzi Ci o to aby wywalić divy tam gdzie to możliwe i stosować same tagi np. <img> i opisywać je w css?

Pozdrawiam.

Nie określa do czego służy plik. Już lepiej jakbyś miał np podział na user_function, film_function i w każdym pliku kod odpowiedzialny za wykonywanie operacji na użytkowniku lub filmie.
Zmniejszy to też rozmiar ładowanego pliku (lepiej wczytać 1 potrzebny plik o wadze np 3 KB niż 1 o wadze 20 KB zawierającym wszystkie funkcje).
Ryzykiem takiego podziału jest nadpisywanie funkcji, więc może lepiej zainteresuj się OOP.

A to nie jest tak, że "Do not edit nothing" byłoby błędne, a "Do not edit anything" jest poprawne?

"Do not edit anything" jest poprawnie.

Nie. IMG służy do obrazu i nie jest nowością w HTML5. Resztę tak, wywal div, użyj <header, section, nav i aside>. Opisz i dziedzicz je w CSS.

Właśnie spróbowałem, zapomniałem że html5 ma nowe tagi jednak czy nie będzie wtedy problemu ze starszymi przeglądarkami? IE w trybie zgodności robi sieczkę.