Ocena kodu js

Question

Witam,

Jestem kiepski w javascript'cie i postanowiłem poćwiczyć.
Zrobiłem sobie coś takiego. Nie ważne co to robi. Ważne czy ma to sens i spójność?

const $ = function(el) {
    return document.querySelector(el);
}

const koloruj = function(col = "rgba(0, 0, 0, 0)") {
    $("header").style.backgroundColor = col;
}

const dopisz = function() {
    
    const opcja = document.createElement("option");
    const page = $(".howPage").value;
    
    opcja.text = page;
    
    $(".listLink").add(opcja);
    $(".iframe").src = "http://" + page;
}

const powrot = function() {
    
    const wybor = $(".listLink").value;
    
    $(".iframe").src = "http://" + wybor;
}

document.addEventListener("DOMContentLoaded", function() {
    
    const fieldNext = $(".next");
    const fieldPrevious = $(".previous");
    const fieldHeader = $("header");
    const go = $("#go");
    
    fieldNext.addEventListener("mouseenter", function() {
        koloruj("#995454");
    });
    
    fieldPrevious.addEventListener("mouseenter", function() {
        koloruj("#546D99");
    });
    
    fieldHeader.addEventListener("mouseleave", function() {
        koloruj();
    });
    
    go.addEventListener("click", function() {
        dopisz();
    });
});

 

Answer 1

Cześć,

generalnie w kodzie jest sporo błędów, ale postaram się podać Ci kilka moich subiektywnych wskazówek:

1 - nie twórz zmiennych globalnych. Poczytaj o modułach, ale na początku nauki o IIFE (tzw. natychmiastowe wyrażenia funkcyjne). Zamkniesz wtedy w zakresie IIFE wszystkie swoje zmienne.

2 - Twój kod:

const $ = function(el) {
    return document.querySelector(el);
}

można zapisać również np. poprzez zbindowanie metody querySelector, ale raczej proponowałbym querySelectorAll:

const $ = document.querySelectorAll.bind(document);

3 - staraj się robić angielskie nazwy zmiennych i funkcji, ale o ile jeszcze wersję PL można by na początku dopuścić, o tyle nazwa "koloruj" jest błędna - nic nie mówi o tym co funkcja robi, co koloruje? po co? gdzie? Ty ustawiasz w niej kolor dla header, więc może lepiej np. nazwa setHeaderColor ?

4 - Co to za metoda "add" dla obiektu zwracanego metodą querySelector? Działa Ci ten kod prawidłowo? Metoda add z tego co pamiętam chyba jest w jQuery (ale nie jestem pewien teraz na 100% bo dawno nie używałem jQuery).

5 - j.w. ale z właściością "value". w vanilla JS raczej musiałbyć użyć innerHTML albo textContent (dla węzła tekstowego). 

6 -strasznie mieszasz... raz pobierasz referencje po ID, za chwilę po klasie, a jeszcze gdzie indziej po typie elementu... W tak małym projekcie proponuję używać jednolitej zasady.

7 - nie musisz używać zdarzenia DomContentLoaded, w zupełności wystarczy podpięcie skryptu przez znacznikiem </body> albo nadanie mu atrybutu async lub defer (w zależności co robi).

To tak na szybko, pewnie inni dadzą Ci jeszcze parę cennych uwag. Działaj i twórz dalej, powodzenia :)

 

Comments

metodę querySelectorAll i ustawić, że jeśli znalazło więcej niż jeden element, to zwróć tablicę, a w przeciwnym razie zwróć jeden element.

Tylko pytanie po co? Mając tablicę nawet jednoelementową możesz swobodnie korzystać z wielu metod Array.prototype jak filter, map itp. Na przykład prostym trikiem:

const elem = [...document.querySelectorAll('selector')];

tworzymy tablicę referencji DOM z pełnym Array.prototype. Daje nam to bezpieczeństwo gdyż wiemy, że zawsze mając co najmniej jeden element możemy operować wieloma metodami np. every, some, filter, map, includes itp. itd. Nie musimy tworzyć dwóch wersji kodu: dla jednego elementu + dla wielu jako tablica.

---

Witam po długiej przerwie,

Przekształciłem kod na taki:

const $ = (function(el = null) {
    return document.querySelector(el);
});

const myModule = (function() {
    
    const _setColorHeader = function(col = "rgba(0, 0, 0, 0)") {
        $("header").style.backgroundColor = col;
    };

    const _setOptionToSelect = function() {
        const opcja = document.createElement("option");
        const page = $(".howPage").value;

        opcja.text = page;

        $(".listLink").add(opcja);
        $(".iframe").src = "http://" + page;
    };
   
    const _goBackSite = function() {
        const s = $(".listLink").value;

        $(".iframe").src = "http://" + s;
    };

    const _allEvents = function() {
        $(".next").addEventListener("mouseenter", _setColorHeader("#995454"));
        $(".previous").addEventListener("mouseenter", _setColorHeader("#546D99"));
        $("header").addEventListener("mouseleave", _setColorHeader());
        $("#go").addEventListener("click", _setOptionToSelect());
        $("#goBack").addEventListener("click", _goBackSite());
    };

    const _init = function() {
        _allEvents();
        _setColorHeader();
        _setOptionToSelect();
        _goBackSite();
    };
    
    return {
        init: _init(),
		setColorHeader: _setColorHeader,
		setOptionToSelect: _setOptionToSelect,
		goBackSite: _goBackSite,
    }
    
})();

Nadal pewnie jest bez sensu, ale robiłem z tutorialem.

Problem jest taki, że setColorHeader i goBackSite  w ogóle nie działają, a setOptionToSelect działa kiedy... wpisze coś w pole, okno przeglądarki ze stroną przestanie być aktywne, po powrocie oraz odświeżeniu działa.

Dziwne strasznie

---

Działa!

Pytanie czy napisane "poprawną polszczyzną" :)

const $ = (function(el = null) {
    return document.querySelector(el);
});

const myModule = (function() {
    
    const _setColorHeader = function(col = "rgba(0, 0, 0, 0)") {
        $("header").style.backgroundColor = col;
    };

    const _setOptionToSelect = function() {
        const option = document.createElement("option"),
              page = $(".howPage").value;

        option.text = page;

        $(".listLink").add(option);
        $(".iframe").src = "http://" + page;
    };
   
    const _goBackSite = function() {
        const s = $(".listLink").value;

        $(".iframe").src = "http://" + s;
    };

    const _init = function() {
        $(".next").addEventListener("mouseenter", function() { _setColorHeader("#995454"); });
        $(".previous").addEventListener("mouseenter", function() { _setColorHeader("#546D99"); });
        $("header").addEventListener("mouseleave", function() { _setColorHeader(); });
        $("#go").addEventListener("click", function() { _setOptionToSelect(); });
        $("#goBack").addEventListener("click", function() { _goBackSite(); });
    };
    
    return {
        init: _init()
    }
    
})();

 

---

const $ = (function(el = null) {
    return document.querySelector(el);
});

Czemu ma służyć zamknięcie funkcji w nawiasach? Nie jest to IIFE, bo nie wywołujesz funkcji od razu.

        const option = document.createElement("option"),
              page = $(".howPage").value;

Czytelniej i bezpieczniej jest każdą deklarację i inicjalizację kończyć średnikiem i zaczynać od słówka var/let/const. Widząc słówko kluczowe od razu wiesz, że zmienną tworzysz pierwszy raz (chyba, że korzystasz z var, wtedy możesz ją reinicjalizować - pozostałe słówka służące do tworzenia zmiennych wykluczają wielokrotną inicjalizację w tym samym scope), a tak na pierwszy rzut oka nie wiesz, czy zmienna jest tutaj deklarowana czy coś do niej tylko przypisujesz.

---

Dobrze, funkcja $ poprawiona, ale...

Rozpisałeś mi się o najmniej istotnej rzeczy jakim jest łańcuchowa deklaracja zmiennych...
Nie wydaje mi się, żeby to był, aż tak duży problem i głównie sposób zapisu zależy od osoby, jaki styl pisania preferuje.

Jakiej jakości jest reszta kodu, bo fajnie jest komuś pisać o takich pierdołach jak wyżej, a trudniejsze tematy omijać. Skoro Krzycho92 nie podałeś innych uwag to znaczy, że reszta jest dobrze zrobiona?

PS. Sorka, ale trochę mi ciśnienie skoczyło jak ktoś mi opowiada o... zmiennych, a widzi, że początki mam już dawno za sobą i chce wejść na wyższy poziom kodowania.