Ocena kodu javascript

pytanie zadane 31 października 2016 w JavaScript przez dewe Gaduła (4,300 p.)

Witam, uczę się programowania bardzo krótki okres czasu, postanowiłem zacząć uczyć się JS i jest to mój pierwszy skrypt (obejrzałem odcinek od MZ a później sam kodowałem całość) i prosiłbym was o ocenę kodu.

Na pewno wiele osób napisze mi, że jest on do poprawy, ale mam pytanie, czy to w jaki sposób napisałem go jest jakimś tragicznym błędem, niewybaczalnym? Czy od biedy ujdzie? Co jest ewentualnie do poprawy? (prosiłbym o jakieś dokładne objaśnienie lub zalinkowanie) :) Z góry dziękuję.


	var numer = 1;
	var timer;
	
	function dodaj()
	{
		clearTimeout(timer);
		if (numer>4) numer = 1;
		numer++;
		var plik = "<img src=\"images/" + numer + ".png\" />" + "<span class=\"left\" onclick=\"odejmij()\"></span>" + "<span class=\"right\" onclick=\"dodaj()\"></span>";		
		document.getElementById("slider").innerHTML = plik;
		numer--;
		zmienslajd();
	}
	function odejmij()
	{
		clearTimeout(timer);
		numer--;	
		var plik = "<img src=\"images/" + numer + ".png\" />" + "<span class=\"left\" onclick=\"odejmij()\"></span>" + "<span class=\"right\" onclick=\"dodaj()\"></span>";		
		document.getElementById("slider").innerHTML = plik;
		numer--;
		if (numer>4) numer = 1;
		if (numer<1) numer = 4;			
		zmienslajd();	
	}
	function zmienslajd()
	{
		if (numer>4) numer = 1;
		var plik = "<img src=\"images/" + numer + ".png\" class=\"wow fadeIn animated\" data-wow-offset=\"10\" data-wow-duration=\"1s\" data-wow-delay=\"0.5s\" />" + "<span class=\"left\" onclick=\"odejmij()\"></span>" + "<span class=\"right\" onclick=\"dodaj()\"></span>";		
		document.getElementById("slider").innerHTML = plik;
		numer++;
		timer = setTimeout(zmienslajd, 5000);
	}

<!DOCTYPE html>
<html lang="pl">
<head>
<script src="js/jquery-3.1.1.min.js"></script>
<link rel="stylesheet" href="css/animate.css"/>
<link rel="stylesheet" href="css/main.css"/>
<script src="js/slider.js"></script>
</head>
<body>
<div id="slider"></div>
<script>
$( document ).ready(function() { zmienslajd(); });
</script>
</body>
</html>

1	komentarz 1 listopada 2016 przez xmentor Nałogowiec (49,520 p.) Dostałeś małe wskazówki tu: http://forum.pasja-informatyki.pl/192436/problem-z-sliderem?show=192459#c192459 Zastosowałeś się do 1/2. Zamiast onClick użyj addEventListener.

komentarz 1 listopada 2016 przez dewe Gaduła (4,300 p.)

Rozdzieliłem kod na trzy pliki,

Zastosowałem Animate.css,

Zastosowałem $( document ).ready(),

Ten kod który Ty podałeś natomiast (http://codepen.io/anon/pen/PGMwqx?editors=1111) jest dla mnie mało zrozumiały :P i chyba wybiega za mocno w przyszłość mojej nauki, uczę się js'a może 3 albo 4 godzinę. Ale dziękuję bardzo :)

1	komentarz 1 listopada 2016 przez xmentor Nałogowiec (49,520 p.) Chodzi mi o 1 komentarz :) To nie mój kod.

komentarz 1 listopada 2016 przez dewe Gaduła (4,300 p.)

Aaa, wybacz :)

A w jakim celu stosuje się to IIFE?

Oraz... Hmmm.. Wiem jak w JS złapać element (getElementById) natomiast tych <img> w html'u byłoby powiedzmy 4/5 (bo tyle slajdów) to jak mam to zrobić? Złapać elementy po Klasie, ale jak zmienić tylko i wyłącznie "src" a nie całą zawartość (innerHTML)?

komentarz 1 listopada 2016 przez xmentor Nałogowiec (49,520 p.)

A w jakim celu stosuje się to IIFE?

Np. żeby nie tworzyć zmiennych globalnych. Potestuj jak to działa.

(function() {
   var x = 10;
}());
console.log(x);

Oraz... Hmmm.. Wiem jak w JS złapać element (getElementById) natomiast tych <img> w html'u byłoby powiedzmy 4/5 (bo tyle slajdów) to jak mam to zrobić? Złapać elementy po Klasie, ale jak zmienić tylko i wyłącznie "src" a nie całą zawartość (innerHTML)?

Wystarczy jeden obrazek wrzucić do tego diva, nadać dla img klase/id i w JS zmieniać src złapanego elementu img

var slide = document.getElementById('slide');
slide.src = 'scieżka/img_'+i+'.png';

komentarz 1 listopada 2016 przez dewe Gaduła (4,300 p.)

No tak, ale co złego jest w globalnych zmiennych? Co do tego przykładu który podałeś od razu wiedziałem jaki będzie rezultat, bo zdaje sobie sprawę co to są zmienne globalne oraz lokalne, ale nie rozumiem dlaczego nie można stosować globalnych, oraz np. gdybym miał 2 funkcje i w 1 funkcji umieściłbym jakaś zmienną to jak zmienić wartość tej zmiennej w drugiej funkcji? Wtedy trzeba wykonać polecenie "return" dla tej zmiennej aby była widoczna, tak?

Dobra, troszkę mi to objaśniłeś :) wezmę się do roboty w wolnym czasie.

komentarz 1 listopada 2016 przez xmentor Nałogowiec (49,520 p.)

ale co złego jest w globalnych zmiennych?

Wyobraź Sobie, że dodajesz do swojej strony jakiś zewnętrzny skrypt, który również posiada zmienne globalne i dziwnym trafem znajduje się jedna o takie samej nazwie jak Twoja - 'timer', wtedy mamy problem.

dlaczego nie można stosować globalnych

Można, ale jest to po prostu niezalecane :)

komentarz 1 listopada 2016 przez dewe Gaduła (4,300 p.)

Dzięki bardzo ;) właśnie mi się przypomniało przed chwilą, że istnieje coś takiego jak google - wstukałem w google dlaczego nie warto stosować zmiennych globalnych i wyczytałem to samo co powiedziałeś, dzięki ! :)

Czyli mam zrobić tak?:

(function (undefined) {

    var timer;
    var numer;

    function zmianaslajdu()
    {

    }
    function dodaj()
    {

    }
    function odejmij()
    {

    }

    (...)

})();

1	komentarz 1 listopada 2016 przez xmentor Nałogowiec (49,520 p.) No można tak to zrobić, ale jak Comandeer napisał: zrób z tego plugin JQuery - googluj :)

komentarz 1 listopada 2016 przez dewe Gaduła (4,300 p.)

Dopiero zaczynam przygodę z JS, nie wiem czy warto tak szybko zagłębiać się w jQuery :/ Namiesza mi to w głowie.

Jeśli napiszę ten kod w JS tak jak wyżej przedstawiłem to będzie to błąd? Nie chcę robić czegoś co działa a jest niezgodne ze standardami itp. Ale zależałoby mi aby najpierw "rozpracować" JS.

komentarz 1 listopada 2016 przez xmentor Nałogowiec (49,520 p.)

Jeśli napiszę ten kod w JS tak jak wyżej przedstawiłem to będzie to błąd

Nie, no i możesz usunąć ten argument 'undefined'.

komentarz 1 listopada 2016 przez dewe Gaduła (4,300 p.)
edycja 1 listopada 2016 przez dewe

Dobra :) Pod wieczór wstawię tu poprawiony kod, dziękuję bardzo za pomoc ;) Miłego dnia.

komentarz 1 listopada 2016 przez dewe Gaduła (4,300 p.)

A mam jeszcze pytanko jak wywołać tą funkcję IIEF?

W formie testu napisałem coś coś, ale nie działa, nie wiem jak ją wywołać. :/

<body><script>zmianaslajdu()</script></body>
<script>
(function () {
 
    var timer;
    var numer;
 
    function zmianaslajdu()
    {
		console.log("elo");
    }
    function dodaj()
    {
 
    }
    function odejmij()
    {
 
    }

})();
</script>

2 odpowiedzi

odpowiedź 1 listopada 2016 przez Finn Śpiewak Nowicjusz (240 p.)

Cześć!

Na plus:

- Kod jest podzielony na małe funkcje.

Na minus:

- Obliczenia na "magicznych" liczbach; Co to znaczy

if (numer > 4) numer = 1;

? Wg. mnie to znaczy, że twój slider nie może mieć więcej niż 4 slajdy. A co w przypadku gdy twój slider będzie miał 5, 10, 15 slajdów ? ;-)

- Zmienne Globalne numer i timer to zmienne globalne to jest złe po prostu z zasady ;-)
- Twój slider działa tylko w jednym przypadku dla bardzo specificznego html'a; A co w przypadku jeśli na jednej stronie będziesz chciał mieć 3 lub 4 niezależne slidery ?

Porady:

- Przepisz swój kod używając module pattern, Napisz swój kod w taki sposób żebyś mógł mieć 2-3 niezależne slidery na jednej stronie.

komentarz 1 listopada 2016 przez dewe Gaduła (4,300 p.)

Dziekuje ;) ale nie rozumiem po co sa te "module" i jak się tym stworzyc wlasna funkcje o nazwie ktora wymysle, lub jak odwolac sie do Module. Nie rozumiem zbytnio :<

odpowiedź 1 listopada 2016 przez Comandeer Guru (602,340 p.)

Jeśli to i tak działa z jQuery, to polecałbym zrobienie z tego pluginu jQuery.
Osobiście widziałbym to jako "klasę" Slider, która pozwalałaby tworzyć kilka sliderów naraz.
Timery wyrzuciłbym poza funkcję zmieniające slajdy.

komentarz 1 listopada 2016 przez dewe Gaduła (4,300 p.)

W jakim sensie Timer po za funkcję zmieniające slajdy i dlaczego tak?

Oraz jak go odpalić w takim razie aby slajdy się zmieniały skoro timer będzie po za tą funkcją?

komentarz 1 listopada 2016 przez Comandeer Guru (602,340 p.)

Bo planowanie zmiany slajdu a sama zmiana slajdu to jednak ciut inne rzeczy.

W przypadku gdy byłaby to "klasa", ustalanie timerów byłoby po prostu w innej metodzie.