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>

Pokaż 8 poprzednich komentarzy

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 (601,590 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 (601,590 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.