Ocena mojej przykładowej strony (kodu źródłowego)

Chciałbym poprosić osoby mające doświadczenie w programowaniu webowym o ocenę mojej "przykładowej" strony internetowej (jakoś nie mogę przekonać się do wyrażenia web development w środku zdania).

Piszę "przykładowej", a nie "pierwszej", bo in fact nie jest to moja pierwsza strona w życiu, ale w zasadzie "pierwsza" od dłuższego czasu (gdy jeszcze na studiach przerabiałem ten materiał).

Strona jest napisana w HTML 5, w CSS 3, w czystym Javascripcie – EcmaScript w wersji 6 – oraz w PHP 7. (Mam nadzieję, że kwestia wersji PHP nie jest jakaś złożona i polecenie php -v zwróciło właściwą wartość, a nie np. wersję środowiska). Pisząc w CSS korzystałem z metodologii BEM (Block, Element, Modifier) – http://getbem.com/.

Stronę udostępniam na GitHubie, a więc chodzi mi najbardziej o ocenę kodu źródłowego. (Postawiłbym ją na jakimś darmowym serwerze, ale z pewnych względów uznałem, że to generowałoby dla mnie taką ilość problemów, która przekraczałaby korzyści).

Co mnie najbardziej interesuje?

1. "Wartość" kodu PHP (wam pozostawiam decyzję, co przez to rozumieć). Najmniejsze doświadczenie mam właśnie w PHP (moje podejście aktualnie jest takie: "działa? W porządku. Co tam było dalej do zrobienia...").
2. Responsive design (szeroko rozumiany), choć rozumiem, że tutaj przydałoby się raczej zobaczyć stronę w działaniu.
3. Projekt kodu (rozplanowanie kodu w plikach, funkcjach, metodach i skryptach; to już dla wszystkich technologii).
4. Czytelność i zgodność z dobrymi praktykami pisania w tych technologiach.
5. Każda inna uwaga będzie mile widziana.

Myślę, że treść strony mówi sama za siebie; jeśli nie, pytajcie tutaj, wyjaśnię, co będzie trzeba.

Link do projektu: https://github.com/silvuss/silvuss-website-1

Mała uwaga: jeżeli ktoś ma konto na GitHubie, to w ramach nauki GitHuba bardzo chętnie przeczytam tam uwagi, także bezpośrednio do kodu. Tylko bardzo proszę o podanie tam swojego nicku na 4programmers (jeśli komentujący posiada), żebym po prostu wiedział, kto zacz.

IMO masz za dużo klas w swoich stylach. Przykładowo: https://github.com/silvuss/silvuss-website-1/blob/master/php/commons/main-menu-en.php#L3

Masz Nav klasy main-menu, w nim Ul klasy main-menu__list, które ma Li klasy main-menu__list-item. Dlaczego nie możesz zdefiniować stylu dla nav.main-menu ul li? Podobnie w https://github.com/silvuss/silvuss-website-1/blob/master/php/websites-en.php#L18.

w ogóle strasznie dużo tych pliczków styli, które są ładowane w plikach php które są w pliczkach php :) to nie są kontrolery żebyś musiał tworzyć takie krótkie, jak dla mnie zwykłe style.css wystarczy do tak małego projektu i wrzucić to w main-header/main-footer skoro i tak go wszędzie ładujesz

Dzięki, @Ktos, za uwagi! Rzeczywiście, zapomniałem dodać, że przy pisaniu CSS skorzystałem z metodologii BEM (http://getbem.com/). Już dopisuję w pierwszym poście. Było to moje pierwsze zetknięcie się z nią, więc nie wiem, czy dobrze wykorzystałem jej zalecenia. Może rzeczywiście nie daje mi podstaw do nadawania tylu klas?

Inna sprawa, że nie zauważyłem, by taka liczba klas sprawiała mi problemy w tym projekcie (choć jest bardzo prosty).

Jeszcze będę się doszkalać w dziedzinie klas CSS, bo rzeczywiście cały czas mam wrażenie, że można dane mapowanie styli na elementy zrobić na 100 sposobów, a żaden nie jest dobry.

Dzięki, @czysteskarpety, za odpowiedź! Masz rację, że plików jest dużo. To świadome. Postanowiłem sobie, że zrobię ten projekt tak modularny, jak tylko się da, żeby nauczyć się, jak ładnie dzielić kod. Jest to poza tym bardziej dla mnie przejrzyste i nie muszę się obawiać (przynajmniej nie tak bardzo), że kiedyś "stracę kontrolę" nad kodem. Tak ja myślę, niemniej zdaję sobie sprawę, że są różne podejścia, i że nie w każdym małym projekcie należy robić sobie dodatkową robotę.

nie chodzi o podejścia, jak ktoś po tobie przejmie projekt to pierwsze co sprawdza header->style.css, footer->main.js i tyle, przejrzyście i czytelnie dla innych (bo nie sądzę abyś od razu robił projekty z sass, less, mvc itp.)

https://github.com/silvuss/silvuss-website-1/blob/master/js/image-slider.js#L27

zamiast:

1. tworzyć nową tablicę
2. przechodzić przez forEach
3. i robić push w każdej iteracji

to możesz użyć po prostu metody map, która robi to samo mniej więcej, tylko prościej:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/map

Poza tym nie musisz nawet tworzyć zmiennej, bo możesz w jednym ciągu mieć coś takiego:

return foo.map(tutajFunkcja).join(", ")

(żeby było estetyczniej rozbijesz to pewnie na kilka linijek).

Ale z drugiej strony przydałoby się wydzielić zmienną do tego groupedImagesProperties[currentImageIndex], bo powtarzasz 5 razy to wyrażenie.
Pytanie tylko - jak nazwać tę zmienną? Ale tego nie ogarniam. Jak patrzę na to, w jaki sposób jest tworzony ten obiekt:
https://github.com/silvuss/silvuss-website-1/blob/5b6c763d846cf7adf0ae642c614abcc5e4434c0d/js/image-slider-helpers.js#L5
to dla mnie jest to dość nieczytelna struktura danych, ciężko mi sobie wyobrazić jak to działa. Nazwy też mi nic nie mówią.

(Oczywiście to tylko moje wrażenie. Bo może być tak, że to ma sens, że zmienne są odpowiednio nazwane itp. Po prostu na pierwszy rzut oka tego nie widzę).

dodatkowo jak tworzysz repo to najlepiej aby projekt działał od razu po rozpakowaniu/ściągnięciu u ciebie tak nie jest, bo index.php jest schowany w folderze, slider nie działa, a buttony nie mają cursor: pointer;
ale walcz dalej :)

Dzięki, @LukeJL !

zamiast (...) to możesz użyć po prostu metody map, która robi to samo mniej więcej, tylko prościej:

Przyjrzę się tej metodzie.

(żeby było estetyczniej rozbijesz to pewnie na kilka linijek).

Oj, rozbiję, rozbiję, jeśli wyjdzie długie. ;) Lubię jednolinijkowe wywołania, nawet bardzo, ale tylko jeśli są one krótsze niż moje okno edytora. Przewijanie w prawo to nie jest to, co trygrysy lubią najbardziej...

Ale z drugiej strony przydałoby się wydzielić zmienną do tego groupedImagesProperties[currentImageIndex], bo powtarzasz 5 razy to wyrażenie.

Dobrze, że to zauważyłeś. Pierwotnie nie byłem pewien, czy taka zmienna byłaby live, więc żeby już nie szukać w internecie (jestem leniwy), postanowiłem zrobić tak, jak wiedziałem, że zadziała. Należałoby to zmienić, bo jak myślę teraz, to zmienna będzie live.

Pytanie tylko - jak nazwać tę zmienną? Ale tego nie ogarniam. Jak patrzę na to, w jaki sposób jest tworzony ten obiekt: (...) to dla mnie jest to dość nieczytelna struktura danych, ciężko mi sobie wyobrazić jak to działa. Nazwy też mi nic nie mówią. (Oczywiście to tylko moje wrażenie. Bo może być tak, że to ma sens, że zmienne są odpowiednio nazwane itp. Po prostu na pierwszy rzut oka tego nie widzę).

Cóż, tu mnie znów masz, no nie jestem dumny z funkcji function groupImagesProperties(imageFilesNames, imagesDirectory), ale uznałem, że muszę ją wydzielić (i jakoś nazwać), bo inaczej byłbym jeszcze mniej dumny z czytelności kodu. Napiszę tu jeszcze później, co ta funkcja dokładnie robi, tylko muszę sobie przypomnieć (mam słabą pamięć do czegoś, co uznałem za zakończone, ale to może też znak, że kod jest za mało intuicyjny w projekcie). Poza tym dobrze, że piszesz o tym, bo przy okazji zauważyłem, że można by zmienić pętle for-in na pętle for-of (jako że są bardziej bezpieczne).

@czysteskarpety, no jak mogłem zapomnieć o index.php! U siebie na serwerze mam go w głównym katalogu xD, ale jest mi z tym źle, ale już nie chciałem mieszać w Apache'u (nie cierpię tego). Oczywiście, sprawa do zmiany.

Ale: co to znaczy, że slider nie działa?

Że przyciski mają mieć łapkę, to w porządku, może i byłoby tak bardziej intuicyjnie. Do zmiany.

PS. Dzięki, @czysteskarpety, że tak ująłeś ostatnie zdanie. Mówiąc mimochodem, czasami mam wrażenie, że rzeczywiście walczę, chyba najbardziej ze sobą, żeby moje aspiracje odpowiadały mojej inteligencji – albo jeszcze lepiej odwrotnie.

Odpowiedzi na wątpliwości

@LukeJL:

Funkcja groupImagesProperties(...) robi (niestety) to, do czego predysponuje ją jej nazwa: grupuje cechy poszczególnych obrazów, które mają być wyświetlane w image sliderze, żeby łatwiej mi było zarządzać nimi później w kodzie. Jest wywoływana tylko w jednym miejscu (co wydaje mi się właściwe ze względu na jej, hm, "rodzaj", to jest: "tworząca obiekt") – a zmienna zawierająca utworzony obiekt była mi potrzebna przede wszystkim w tym fragmencie: https://github.com/silvuss/silvuss-website-1/blob/master/js/image-slider.js#L28-L36

Przy okazji: mam nadzieję, że nie masz nic przeciwko, że wzmiankuję na GitHubie Twoje porady tutaj linkiem do Twojego postu.

Postępy w ulepszaniu projektu

Stworzyłem trzy issue na GitHubie, dotyczące wspomnianych porad, i będę wprowadzać je w życie (mam nadzieję):
https://github.com/silvuss/silvuss-website-1/issues

Mam chwilowe problemy i w związku z tym chciałbym zapytać Was, w szczególności @czysteskarpety oraz @LukeJL, czy nie wiecie, jak mógłby to rozwiązać. Opisałem to w tym wątku: https://4programmers.net/Forum/PHP/307861-apachephp_powielanie_czlonow_w_sciezce_adresu_url. Trudno mi przejść nad tym do porządku dziennego, więc projekt aktualnie posuwa się w tempie let speed = (function (speed) { return speed; })(null);. Oczywiście, może coś sam wymyślę, ale nie zanosi się na to szybko.

(Piszę post pod postem, by były powiadomienia).

UPDATE: Działa! :] Napisałem wszystko w wyżej wzmiankowanym wątku. Ale nadal możecie mi pomóc, ponieważ nie wiem, dlaczego akurat tak działa (chodzi najprawdopodobniej o parsowanie ścieżek w PHP). No, mogę zabrać się wreszcie do właściwej roboty.

Odpowiedzi na wątpliwości

@LukeJL:

1. Przyjrzałem się metodzie Array.map i uznałem, że nie ma w projekcie miejsca, w którym jej zastosowanie spowodowałoby, że kod będzie bardziej przejrzysty. Dodałem wyjaśniający komentarz w miejscu, w którym pasowałaby najbardziej: https://github.com/silvuss/silvuss-website-1/blob/master/js/image-slider.js#L29-L45
2. Zamieniłem wywołanie groupedImagesProperties[currentImageIndex] na zmienną currentImageProperties (https://github.com/silvuss/silvuss-website-1/blob/master/js/image-slider.js#L12-L13).

@czysteskarpety:

1. Zmieniłem kursor nad przyciskami (https://github.com/silvuss/silvuss-website-1/blob/master/css/image-slider.css#L21).
2. Co do umieszczenia index-en.php, nie jestem przekonany, czy warto oddzielać go od innych plików PHP. Jeszcze się nad tym zastanowię.

Postępy w ulepszaniu projektu

Wszystkie issues zostały zamknięte (hurra!).

Mam jednak do Was pytanie: chciałbym pozbyć się funkcji groupImagesProperties(...) (opisałem ją już pokrótce kilka postów wyżej), bo jest nieintuicyjna. Jednak to stawia mnie w następującej sytuacji problemowej.

Mam image slider, który sobie wyświetla aktualnie dwa obrazy. Każdy obraz mam w czterech wersjach rozmiarowych. I teraz: listę obrazów pobieram przeglądając katalog za pomocą PHP, a następnie uzyskaną listę przesyłam do Javascriptu JSON-em. Jednak jest to lista niepogrupowana – wszystkie wersje rozmiarowe każdego obrazu są na jednej liście (czyli dla dwóch obrazów i czterech wersji lista ma 8 elementów). Żeby móc zarządzać wersjami rozmiarowymi, uzyskane nazwy plików grupuję właśnie w funkcji groupImagesProperties(...) po nazwie obrazu (parsuję nazwę wyrażeniem regularnym) – to jej główne zadanie. Jeśli miałbym ją usunąć/zmodyfikować, to muszę jakoś inaczej grupować te obrazy.

Przyszły mi na myśl trzy podejścia:

1. Modyfikacja tej funkcji, żeby wykonywała jedynie grupowanie, a przesunięcie pozostałych instrukcji z niej tam, gdzie jest ona wywoływana.
2. Usunięcie tej funkcji, zrobienie grupowania w PHP (tzn. jakby poziom niżej, bo jeszcze przed przesłaniem JSON-u) i obsłużenie jakoś przesyłania w JSON-ie dwóch tablic z nazwami plików zamiast jednej (?).
3. Usunięcie tej funkcji, zrobienie oddzielnych folderów na każdy obraz (w każdym folderze byłyby wersje rozmiarowe tylko jednego obrazu), obsłużenie w PHP przeglądania kilku folderów zamiast jednego i obsłużenie jakoś przesyłania w JSON-ie dwóch tablic z nazwami plików zamiast jednej (?).

Które według Was jest najlepsze?

Jeśli ktoś ma jeszcze jakieś uwagi, bardzo chętnie je przyjmę.