Witam!
Ostatnio chciałem pogłębić moją znikomą wiedzę na temat javascript'u/jquery. Napisałem wczoraj prosty rotator zdjęć. Prosiłbym o jakąś opinie lub uwagi osób które mają ogarnięcie w js. Może informacje co mógłbym poprawić na co zwracać uwagę. Krytyka mile widziana

plik z kodem: image_rotator

html:

[HTML] pobierz, plaintext 
   <div class="wrapper">
      <div class="buttons">
	<a href="#" class="left"><</a>
	<a href="#" class="right">></a>
      </div>
      <div class="image-wrapper">
	<img src="img/1.jpg" class="active" alt="1">
	<img src="img/2.jpg" alt="2">
	<img src="img/3.jpg" alt="3">
      </div>
      <div class="pagination"></div>
   </div> 
[HTML] pobierz, plaintext 

css:

[HTML] pobierz, plaintext 
.wrapper {
  margin: 0 auto;
  position: relative;
  width: 600px;
  height: 350px;
  overflow: hidden;
  border: 4px solid grey;
} 
.wrapper .image-wrapper img{
  position: absolute;	
  display: none;
  height: 378px;
  margin: 0 auto;
  background: white;
}
.wrapper .image-wrapper img.active {
  z-index:1;
  display: inline-block;
}
.wrapper .buttons {
  font-family: arial;
  font-weight: bold;
  color: white;
  margin: 0 auto;
  width: 100%;
  position: absolute;
  z-index: 99;
  top: 150px;
 
}
.wrapper .buttons a {
  text-decoration: none;
  color: #fff;
}
.wrapper .buttons .left{
  position: absolute;
  left: 10px;
  display: block;
  background: rgba( 0,0,0,0.5);
  padding: 5px 10px;
 
}
.wrapper .buttons .right{
  position: absolute;
  right: 10px;
  display: block;
  background: rgba( 0,0,0,0.5);
  padding: 5px 10px;
}
 
.wrapper .pagination {
  background: rgba( 0,0,0,0.5);
  position: absolute;
  padding: 5px;
  bottom: 5px;
  left: 10px;
  z-index: 3;
}
.wrapper .pagination .page_el {
  float: left;
  height: 10px;
  width: 10px;
  margin: 1px;
  border: 1px solid #fff;
}
.wrapper .pagination .checked {
  background: #ddd;
}
[HTML] pobierz, plaintext 

js:

[HTML] pobierz, plaintext 
var nextImg = function() {    
     var imgs = $('.image-wrapper img');
     var active_obj = $('.active');
     var next_img = active_obj.next();
     var first_img = imgs.first();
 
 
    if( !imgs.last().hasClass('active')){    
     next_img.css("z-index", "2")
     next_img.fadeIn( 800, function() { 
	active_obj.css("z-index","1");
	active_obj.css("display","none");
	active_obj.removeClass('active');
        next_img.addClass('active');
      });
    }else{
      first_img.css("z-index", "2");
      active_obj.css("z-index", "1");
      first_img.fadeIn( 800, function() {   
	active_obj.css("z-index","1");
	active_obj.css("display","none");
	active_obj.removeClass('active');
	first_img.addClass('active');
      });
    };
    return 1;
}
 
var prevImg = function() {    
     var imgs = $('.image-wrapper img');
     var active_obj = $('.active');
     var prev_img = active_obj.prev();
     var last_img = imgs.last();
 
     if( !imgs.first().hasClass('active')){
 
     prev_img.css("z-index", "2")
     active_obj.css("z-index", "1");
     prev_img.fadeIn( 800, function() { 
	active_obj.css("z-index","1");
	active_obj.css("display","none");
	active_obj.removeClass('active');
        prev_img.addClass('active');
 
      });
    }else{
      last_img.css("z-index", "2");
 
      last_img.fadeIn( 800, function() {   
	active_obj.css("z-index","1");
	active_obj.css("display","none");
	active_obj.removeClass('active');
	last_img.addClass('active');
      });
    };
}
 
var pagination_build = function () {
    count_of_img = $(".image-wrapper img").length;
    console.log(count_of_img);
    for(i=0; i<count_of_img; i++){
      $(".pagination").append('<div class="page_el pid_'+i+'"></div>');
 
    }
}
 
var pagination_check = function(im_a, pa_a, im, pa) {
    for(i=0; i < im.length ; i++){
      if( im_a[i].hasClass('active') == true ){
	pa_a[i].addClass('checked');
      }else{
	pa_a[i].removeClass('checked');
      };
    } 
}
 
var display_img = function(im_a,pa_a, id) {
  for(i=0; i<im_a.length; i++){
    if( pa_a[i].hasClass(id)){
      im_a[i].fadeIn(800);
      im_a[i].addClass("active");
      pa_a[i].addClass("checked");
 
    }else{
      im_a[i].css("z-index","0");
      im_a[i].css("display","none");
      im_a[i].removeClass("active");
      pa_a[i].removeClass("checked");
    }
  }
}
 
 
$(document).ready( function(){
 
    pagination_build();
 
    var img_arr = [];  
    var pag_arr = [];
    var imgs = $(".image-wrapper img");
    var pags = $(".pagination div");
 
    imgs.each( function(){ 
      img_arr.push($(this));
    });
    pags.each( function(){
      pag_arr.push($(this));
    });
 
    for(i=0; i < imgs.length ; i++){
      img_arr[i].addClass("id_" + i);
    }
 
  $("a.right").click( function(){
    nextImg();
    setTimeout(function(){
     pagination_check(img_arr, pag_arr, imgs, pags);
    },900);
  });
  $("a.left").click( function(){
    prevImg();
    setTimeout(function(){
    pagination_check(img_arr, pag_arr, imgs, pags);
    },900);
  });
 
 
  pagination_check(img_arr, pag_arr, imgs, pags);
 
  var clicked_obj_name;
 
  $(".page_el").click( function(event){
    clicked_obj_name = event.target.className;
    console.log(clicked_obj_name);
    display_img(img_arr,pag_arr,clicked_obj_name);
 
 });
 
 
    ticker = setInterval( function(){	     
      pagination_check(img_arr, pag_arr, imgs, pags);
      nextImg();
      pagination_check(img_arr, pag_arr, imgs, pags);
    }, 5000 );
 
});
[HTML] pobierz, plaintext 

Garść nocnych uwag

1. Dla bloku obejmującego cały slider użyłbym na Twoim miejscu ID (id="wrapper") i później przy wyszukiwaniu elementów w DOM korzystał z kontekstu. W efekcie przyspieszy to skrypt, bo raz, że samo znalezienie kontekstu będzie dla silnika JavaScript szybkie, bo jQuery skorzysta z natywnej metody getElementById(), a dwa, że każde kolejne przeszukiwanie będzie przeprowadzane w obrębie kontekstu. Przykład poniżej:



2. Korzystaj z chaining'u, większość metod jQuery zwraca przetworzony zbiór. Dla jasności:



3. Dla metody .css() przy większej ilości parametrów przekazuj obiekt. Przykład:



4. Przechowuj w zmiennej długość tablicy, po której iterujesz. Dzięki temu pętla za każdym razem nie będzie musiała przeliczać długości tablicy. Ewentualnie jeżeli masz możliwość możesz korzystać z pętli while i odliczać do 0 (porównywanie z 0 będzie szybsze).



5. Unikaj tego typu kodu:

Generalnie przeszukiwanie DOM jest mocno kosztowne. Lepiej zrobić tak, że w pamięci tworzysz sobie jakiś element i w pętli append'ujesz do niego kolejne elementy. A dopiero poza pętlą dodajesz to do drzewa DOM.

6. Ten fragment jest zupełnie niepotrzebny. Metoda .each() działa na zbiorach jQuery więc niepotrzebnie przenosisz zbiór do nowej tablicy.



7. Ogólnie masz trochę powtórzeń w kodzie. Postaraj się je wyeliminować ;].

8. Jeżeli piszesz to jako plugin jQuery to możesz skorzystać z wzorca pluginów. Zdaje mi się, że na stronie jQuery jest gdzieś przykład w jaki sposób je pisać. Weź pod uwagę dodanie obiektu konfiguracyjnego, dzięki któremu przy odpaleniu slidera będzie możliwe skonfigurowanie, np. interwału zmiany obrazków.

9. W instrukcjach warunkowych korzystaj z operatora identyczności (===) zamiast równości (==).

;]

Wielkie dzięki!
Naprawdę pomocna i konstruktywna odpowiedz. Jeśli chodzi o przepisanie tablicy w tablice to faktycznie to było dość głupie Mam pytanko jeśli chodzi o punkt 7 czy moglbys mi podać przykład takiego powtórzenia, które można wyeliminować.
Wiem ze sposób oznaczania zdjęcia względem paginacji jest trochę zamieszany w tej chwili jest całkowicie zależny od timingu w sensie jeśli w jednym miejscy czas wykonania (setTimeout(), setInterval()) jest większy niż w innym zaczyna się wszystko mieszać, może jest jakiś lepszy sposób na rozwiązanie tego.
Dzięki za każdą pomoc:)
A może, jakie macie sposoby na rozwijanie swoich umiejętności? Jakie sobie stawialiście zadania podczas nauki? Z jakich pomocy korzystacie, literatura, strony internetowe(nie chodzi mi o oficjalną dokumentację)?

W wielu miejscach umieszczasz kod manipulujący na klasach CSS i przełączający z-index. Spróbój sobie to wydzielić do osobnej metody, która wykonuje te przekształcenia. Dodatkowo, na przykład 3 razy przeszukujesz DOM zapisujac do zmiennej zbiór:



Zdefiniuj sobie to tylko raz. Zauważyłem jeszcze, że funkcja nextImgs() zwraca 1 (return 1). To chyba też Ci nie jest potrzebne.

Poczytaj sobie o konwencjach pisania kodu JavaScript. Dla mnie trochę ciężko czyta się ten kod, do tego zmienne powinny same tłumaczyć co przechowywują, dla przykładu: im_a[i] nie mówi mi nic . Nie jest to jakiś warunek konieczny, ale znacząco poprawi jakość kodu. Możesz jeszcze skorzystać z narzędzia JSLint.

Pozdrawiam ;]

Co do używania append w ciele pętli, lepiej to zrobić w następujący sposób:

[PHP] pobierz, plaintext 
var x,
len = 20,
animArr = [];
for(x=0;x<len;x +=1) {
            animArr[x] = '<div id="cos' + x +'"></div>';
        };
        $('jakisDiv').append(animArr);
 
[PHP] pobierz, plaintext 

lub

[PHP] pobierz, plaintext 
var mojedivy = "";			
for(x=0;x<20;x +=1) {
mojedivy += '<div id="cos' + x +'"></div>';
};
 $('jakisDiv').append(mojedivy);
[PHP] pobierz, plaintext 

Masz utworzonych np. 20 divów i teraz dodajesz tylko dla tych divów określone wartości.

Po drugie zamiast operować na klasach lepszym sposobem jest operować na indeksach.

Po trzecie zamiast each używaj szybszych pętli, jakie oferuje js.