• Najnowsze pytania
  • Bez odpowiedzi
  • Zadaj pytanie
  • Kategorie
  • Tagi
  • Zdobyte punkty
  • Ekipa ninja
  • IRC
  • FAQ
  • Regulamin
  • Książki warte uwagi

[CODE-REVIEW] Game of life - drugi projekt - javascript

VPS Starter Arubacloud
+4 głosów
404 wizyt
pytanie zadane 19 października 2016 w JavaScript przez Codeboy Stary wyjadacz (12,120 p.)

Mam dla Was drugi projekt do oceny. Jest to Game of life Johna Conwaya napisany "moimi sposobami". Poprzedni projekt (Wątek na forum)  wiele mnie nauczył i dużo poprawiłem, głównie dzięki Krzycho92 i Argeento, za co jestem im bardzo wdzięczny. Mam nadzieję, że nie popełniłem poprzednich "złych" praktyk ;)

Proszę o wytknięcie błędów, rady i naprowadzenie na właściwą drogę ;)

Dzięki za poświęcony czas.

GitHub: https://github.com/NoMoExcuses/Game-of-life
Online: http://www.game-of-life.awsome.pl/

2 odpowiedzi

+3 głosów
odpowiedź 20 października 2016 przez ScriptyChris Mędrzec (190,190 p.)

Mam nadzieję, że nie popełniłem poprzednich "złych" praktyk ;)

Niestety, popełniłeś - nawet te o których wspominaliśmy z @niezalogowany w Twoim poprzednim temacie.

  • 6 krotnie korzystasz z pętli na tej samej zmiennej
    for(var y=0;y<yNumb;y++)
    7 krotnie loopujesz po innej zmiennej
    for(var x = 0;x<xNumb;x++)
    Mógłbyś zrobić sobie funkcję, w której miałbyś pętle for a warunek jej zakończenia (zmienne xNumber i yNumber) przekazywałbyś jako parametr.
    Przykład powtarzalności (od linijki 76):
    		//Zmienianie stanu tabeli stateTable:
    		
    		for(var y=0;y<yNumb;y++)
    		{
    			for(var x=0;x<xNumb;x++)
    			{
    				stateTable[x][y]=futureStateTable[x][y];
    			}
    		}
    		
    		refreshCells();

    i w następnej funkcji robisz prawie to samo (linijka 89):
     

    	function clearCells()
    	{
    		for(var y=0;y<yNumb;y++)
    		{
    			for(var x=0;x<xNumb;x++)
    			{
    				stateTable[x][y]=0;
    			}
    		}
    		refreshCells();
    	}

    Dlaczego w 76 linijce nie wywołasz sobie funkcji clearCells() z przekazaniem parametru, którym byś sobie umieścił w stateTable[x[y] zero albo jakąś zmienną (np. tą futureStateTable[x][y])?

    Przeszukałem sobie ten skrypt, i praktycznie w żadnej własnej funkcji nie przekazujesz parametru - a przecież dzięki temu możesz sobie ułatwić życie i skrócić kod. O tym wspominałem Tobie właśnie w poprzednim wątku. Cytując:
     

Widzę natomiast, że w kodzie masz trzy razy tą samą pętlę, a dodatkowo funkcje draw() i win( who ) wykonują prawie identyczny kod. Dlatego proponuję obie te funkcje złączyć w jedną i sterować środkiem przez jakiś dodatkowy parametr, albo wewnątrz tych funkcji robić odniesienie do kolejnej funkcji, która będzie zawierać pętlę i tam sterować parametrami. Właśnie to jest reużywalność i do tego służą funkcję - aby kod o zbliżonym działaniu móc wykonywać wielokrotną ilość razy, sterując jakimiś różnymi zachowaniami przez parametry. 

  •  troszkę brak konsekwencji:
    	//Key listener
    	window.onkeypress = function(e) 
    	{
    		var key = e.keyCode ? e.keyCode : e.which;
    		if (key == 32) 
    		{
    		   evolution();
    		}
    	}

    Dlaczego to nie jest EventListener? Poniżej już dobrze robisz:
     

    //Eventy
    document.getElementById("click").addEventListener("mousedown", evolution);
    document.getElementById("clear").addEventListener("mousedown", clearCells);
    document.getElementById("random").addEventListener("mousedown", fillRandom);
    document.getElementById("start").addEventListener('click', simulationStart);
    document.getElementById("stop").addEventListener('click', simulationStop);
    document.getElementById("speed").addEventListener("input", refreshSpeed);
    
  • zapomniałeś o stosowaniu potrójnego operatora porównania
  • dlaczego jedne zmienne deklarujesz poniżej funkcji, w której z nich korzystasz (np. wspomniane xNumb oraz yNumb - używasz już w pierwszej funkcji ,a deklarujesz w połowie skryptu), a inne (prawidłowo) deklarujesz przed użyciem w funkcji (np. simulation oraz speed)?
  • przy kilkunastokrotnym odwoływaniu się do obiektu document pokusiłbym się o przekazanie go do IIFE jako parametr, aby JS nie musiał każdorazowo wyłazić poza IIFE i szukać tego pola w obiekcie window. Dla przypomnienia: odwołując się do jakiejś zmiennej, JS szuka jej deklaracji najpierw w lokalnym scope, a potem coraz wyżej, aż do scope'u globalnego (czyli obiektu window). Jeśli przekażesz sobie daną zmienną jako parametr, to JS nie musi wyłazić z lokalnego scope'u = kod wykonuje się szybciej.
     
komentarz 20 października 2016 przez Codeboy Stary wyjadacz (12,120 p.)
Chyba mnie to wszystko przerasta, klika błędów oczywistych i łatwych do poprawy ale te parametry i pętle to cięższa sprawa :(. W ogóle sobie nie wyobrażam zamienieniu wszystkich pętli na jedną ;<
komentarz 20 października 2016 przez efiku Szeryf (75,160 p.)
To zanim się zabierzesz do kodowania to usiądź i pomyśl... rozplanuj..
komentarz 20 października 2016 przez ScriptyChris Mędrzec (190,190 p.)

@NoMoExcuses, możesz spróbować zrobić sobie taką fabrykę tych pętli. Jako naprowadzenie Cię napisałem taki przykładzik - nie testowałem go, napisałem z głowy abyś sobie zobaczył koncepcję. Ty sobie przejrzyj ten kod, poczytaj czym jest closure (też Ci linkowałem w poprzednim temacie) i spróbuj "uruchomić" ten kod :)
Kod poniżej komentarza "//Określanie przyszłego stanu komórki:" jest niezmieniony - więc możesz sobie już tam poćwiczyć zastosowanie takiej fabryki.

function loopFactory() {
   var x = 0;
   var y = 0;

   return {
      outerIterator: x,
      innerIterator: y,
      loop: function( callback ) {
        for( ; y < yNumb; y++ ) {
			for( ; x < xNumb; x++ )	{
              callback();
            }
        }
      }
   }
}


var myLoop = loopFactory();
myLoop.loop( function() {
	var neighborsNumber=0;
	try{ if( stateTable[ myLoop.outerIterator - 1 ][ myLoop.innerIterator - 1 ] === 1 ) neighborsNumber++; } catch(err){}
	try{ if( stateTable[ myLoop.outerIterator ][ myLoop.innerIterator - 1 ] === 1 ) neighborsNumber++; } catch(err){}
	try{ if( stateTable[ myLoop.outerIterator + 1 ][ myLoop.innerIterator - 1 ] === 1 ) neighborsNumber++; } catch(err){}
	try{ if( stateTable[ myLoop.outerIterator - 1 ][ myLoop.innerIterator ] ===1 ) neighborsNumber++; } catch(err){}
	try{ if( stateTable[ myLoop.outerIterator + 1 ][ myLoop.innerIterator ] === 1 ) neighborsNumber++; } catch(err){}
	try{ if( stateTable[ myLoop.outerIterator - 1 ][ myLoop.innerIterator + 1 ] === 1 ) neighborsNumber++; } catch(err){}
	try{ if( stateTable[ myLoop.outerIterator ][ myLoop.innerIterator + 1 ] ===1 ) neighborsNumber++; }catch(err){}
	try{ if( stateTable[ myLoop.outerIterator + 1 ][ myLoop.innerIterator + 1] === 1 ) neighborsNumber++; }catch(err){}
				
	//Określanie przyszłego stanu komórki:

       /** KOD PONIŻEJ NIE JEST ZMIENIONY - dostosuj go samodzielnie */
	
	if(neighborsNumber==3&&stateTable[x][y]==0)futureStateTable[x][y]=1;
	else if(neighborsNumber!=3&&stateTable[x][y]==0)futureStateTable[x][y]=0;
	else if((neighborsNumber>1&&neighborsNumber<4)&&stateTable[x][y]==1)futureStateTable[x][y]=1;
	else if((neighborsNumber<2||neighborsNumber>3)&&stateTable[x][y]==1)futureStateTable[x][y]=0;
} );

Istotne rzeczy. Funkcję loopFactory umieszczasz w IIFE (aby wszsytko w IIFE miało dostęp do tej funkcji). Funkcja ta jako zmienne lokalne posiada zadeklarowane iteratory, z przypisanymi wartościami 0. Po wykonaniu tej funkcji, masz je dostępne pod nazwami otuerIterator oraz innerIterator. Gdy już sobie wywołasz tą fabrykę, to do Twojej dyspozycji jest obiekt z 3 polami: wspomniane iteratory, oraz metoda, która oczekuje w parametrze funkcji (parametr callback) - właśnie tędy przekazujesz funkcje, która ma zostać przerobiona w pętli - pętla jest 2 poziomowa, więc też możesz się pobawić w używanie jednego, bądź dwóch poziomów jakimś dodatkowym parametrem. Trudna kwestia w tej"fabryce" to skorzystanie z iteratorów, w funkcji przekazywanej jako callback. Jak wspomniałem, tego kodu nie testowałem (to jedynie przykład mający Ci pokazać jak możesz sobie stworzyć funkcję przechowującą pętle, z których korzystasz kilka razy w kodzie). Dlatego spróbuj uruchomić to tak, aby zwrócone pola w postaci iteratorów były widoczne w przekazywanym callbacku.

Jeśli popróbujesz i nie będzie wychodzić - to pytaj, ale podawaj też kod który nie działa, aby było wiadomo co sprawia problem. Powodzenia :)

komentarz 21 października 2016 przez Codeboy Stary wyjadacz (12,120 p.)

To zanim się zabierzesz do kodowania to usiądź i pomyśl... rozplanuj..

Zrobiłem to, zanim to napisałem miałem mniej więcej rozplanowane co i jak, ale jak mogłem wziąć pod uwagę metody, których nie znam jeszcze? ;) 

1
komentarz 21 października 2016 przez Codeboy Stary wyjadacz (12,120 p.)

//Key listener

Dlaczego to nie jest EventListener? 

 Poprawione.

zapomniałeś o stosowaniu potrójnego operatora porównania

Poprawione.

 dlaczego jedne zmienne deklarujesz poniżej funkcji, w której z nich korzystasz (np. wspomniane xNumb oraz yNumb - używasz już w pierwszej funkcji ,a deklarujesz w połowie skryptu), a inne (prawidłowo) deklarujesz przed użyciem w funkcji (np. simulation oraz speed)?

Nie wiedziałem, że to ma znaczenie, bo funkcje są dopiero używane jak już wszystko się załaduje, czyli zmienne też więc w moim kodzie nie robiło to różnicy.

 przy kilkunastokrotnym odwoływaniu się do obiektu document pokusiłbym się o przekazanie go do IIFE jako parametr

Zrobione.

Co do 'loopFactory" to ciężko mi to zaimplementować w kod, bo przy małym błędzie wywala się już cały kod. Próbowałem zobaczyć jak to działa na prostym przykładzie nie związanym z moim kodem i też mi się nie udało. Napisałem coś takiego:

function loopFactory() {
   var x = 0;
   var y = 0;
 
   return {
      outerIterator: x,
      innerIterator: y,
      loop: function( callback ) {
        for( ; y < 10; y++ ) {
            for( ; x < 10; x++ )  {
              callback();
            }
        }
      }
   }
}

var id= 0;
 
var myLoop = loopFactory();
myLoop.loop( function() {
   
   var field = document.createElement('div');
	field.textContent= id;
	document.getElementById('result').appendChild(field);
		
	id++;
} );

No i wynik to liczby od 0-9, a jest to pętla dwuwymiarowa więc powinno być 0-99. Więc kompletnie nie rozumiem jak to działa. Druga sprawa to że ja wprowadzam funkcje do pierwszego poziomu pętli jak i drugiego, a tutaj wprowadza się tylko w drugi poziom tam gdzie callback() jeśli dobrze to rozumiem :/. Więc jeśli miałbym to ogarnąć to musiałbyś napisać jakiś prosty kod(nie musi być związany z tym projektem) gdzie używasz obu poziomów pętli i ewentualnie opisał jak co działa, ale bez tego się obejdzie, pewnie dojdę do tego sam. Najlepiej bym to zrozumiał jeśli byś napisał tworzenie tej siatki z divów. Dajmy na to 100 na 100, co 10 wierszy:
 

var nr= 0;

for (var y=0; y<10; y++)
    {
        for (var x=0; x<10; x++)
        {
            var field = document.createElement('div');
            document.getElementById('result').appendChild(field);
            field.style.float="left";
            field.textContent=nr+", ";
            nr++;
        }
        
        var nextRow = document.createElement('div')
        nextRow.style.clear="both";
        document.getElementById('result').appendChild(nextRow);
    }

I wynik to:

 

Nie wiem też jak interpretować to(pierwszy raz się spotkałem z takim zapisem):

for( ; y < yNumb; y++ ) {

            for( ; x < xNumb; x++ )  {

              callback();

            }

1
komentarz 23 października 2016 przez ScriptyChris Mędrzec (190,190 p.)
edycja 23 października 2016 przez ScriptyChris

No i wynik to liczby od 0-9, a jest to pętla dwuwymiarowa więc powinno być 0-99. 

Jak zauważyłeś, pętle te zapisane są w postaci for( ; iterator < 10; iterator++){}. Oznacza, to że w deklaracji pętli nie podałem pierwszego parametru, w którym z reguły tworzy się zmienne dla pętli.

https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Statements/for#Optional_for_expressions

Przejrzyj sobie docsa - w pętli for wszystkie parametry są opcjonalne. Zarówno zmienną, którą inicjalizujesz w pętli, warunek jej zakończenia, jak i inkrementację/dekrementacje możesz przeprowadzić już w ciele pętli. Trzeba tu jednak uważać, aby nie stworzyć pętli nieskończonej, bo przeglądarka (w Chrome najpewniej pojedynczy wątek, w którym jest otwarta aktualna karta ze skryptem) się zawiesi :)

Wracając do sytuacji, której nie rozumiesz. Druga - wewnętrzna pętla odpalana jest tylko raz, ponieważ już za pierwszym razem wewnętrzny iterator dochodzi do limitu, przez co pętla się kończy. Każda następna iteracja zewnętrznej pętli nie tworzy od nowa zmiennej dla wewnętrznej pętli. Gdybyś wewnątrz wewnętrznej pętli dodał pierwszy parametr z utworzonej iteratora, to wtedy byś zobaczył 0-99.

Masz tutaj przykład wykorzystania fabryki dla Twojego przykładu z tworzeniem tych 100 <div>. Przestudiuj go sobie, skomentowałem ważniejsze rzeczy:

function loopFactory( _outerLimiter, _innerLimiter ) {
  var x = 0; // deklaracja iteratora
  var y = 0;
  var outerLimiter = Number( _outerLimiter ) || 10; // sprawdzenie czy podany zewnetrzny limiter jest liczba, jesli nie to jako limiter ustawiam 10
  var innerLimiter = Number( _innerLimiter ) || 10;
  
  return {
    outerIterator: x, // przypisanie iteratora do pola, aby miec do niego dostep przy przekazywaniu callbackow
    innerIterator: y,
    loop: function( outerCallback, innerCallback ) { // przekazanie callbackow
      var self = this; // // ustawienie referencji na self, aby wewnatrz petli w razie czego miec dostep do zwracanego obiektu
      var isOuterCallback = typeof outerCallback === 'function'; // upewnienie sie, ze przekazany callback jest funkcja
      var isInnerCallback = typeof innerCallback === 'function';

      for ( x = 0; x < outerLimiter; x++, self.outerIterator++ ) {
        if ( isOuterCallback ) {
          outerCallback();
        }
        
        for ( y = 0; y < innerLimiter; y++, self.innerIterator++ ) {
          if ( isInnerCallback ) {
            innerCallback();
          }
        }
      }
    }
  }
}

var id = 0;

var myLoop = loopFactory(); // tutaj mozesz opcjonalnie przekazac zewnetrzny i wewnetrzny limiter dla pętl
myLoop.loop( false, function() {

  var field = document.createElement( 'div' );
  field.textContent = id;
  document.getElementById( 'result' ).appendChild( field );
  console.log( 'Iterators: outer/inner: ', myLoop.outerIterator, ' / ', myLoop.innerIterator ); // wypisanie obu referencyjnych iteratorow
  
  id++;
} );
komentarz 23 października 2016 przez Codeboy Stary wyjadacz (12,120 p.)
Bardzo dziękuje, przestudiuje i spróbuje zaimplementować w najbliższym czasie. Jak skończę to się pochwalę co tam naskrobałem ;D Jeśli masz jeszcze jakieś uwagi co do kodu to śmiało ;)
komentarz 25 października 2016 przez Codeboy Stary wyjadacz (12,120 p.)
Wiesz co, tak się zastanawiam, jak ta fabryka pętli poprawia kod? Takie coś jest wydajniejsze czy co? Nie za bardzo rozumiem jaki jest tego cel. Według mnie w tym kodzie jest cały czas tyle samo pętl co było, tylko są wywołane w innym miejscu ;< Do tego w mojej opinii kod straci na prostocie i łatwym rozumieniu, ale to może dlatego, że dopiero jestem początkujący. Rozjaśnisz mi to? ;)
0 głosów
odpowiedź 19 października 2016 przez niezalogowany
Gdy zaznaczę sobie kwadraciki to wyskakują pod nimi numery, nieładnie to wygląda
komentarz 20 października 2016 przez Codeboy Stary wyjadacz (12,120 p.)
A tak tak, zapomniałem je usunąć, one były robocze :')

Podobne pytania

0 głosów
0 odpowiedzi 249 wizyt
pytanie zadane 12 grudnia 2016 w JavaScript przez pietrzakacper Mądrala (7,480 p.)
0 głosów
2 odpowiedzi 642 wizyt
pytanie zadane 30 kwietnia 2017 w JavaScript przez dyzio Nowicjusz (120 p.)
0 głosów
0 odpowiedzi 173 wizyt

92,454 zapytań

141,262 odpowiedzi

319,089 komentarzy

61,854 pasjonatów

Motyw:

Akcja Pajacyk

Pajacyk od wielu lat dożywia dzieci. Pomóż klikając w zielony brzuszek na stronie. Dziękujemy! ♡

Oto polecana książka warta uwagi.
Pełną listę książek znajdziesz tutaj.

Akademia Sekuraka

Akademia Sekuraka 2024 zapewnia dostęp do minimum 15 szkoleń online z bezpieczeństwa IT oraz dostęp także do materiałów z edycji Sekurak Academy z roku 2023!

Przy zakupie możecie skorzystać z kodu: pasja-akademia - użyjcie go w koszyku, a uzyskacie rabat -30% na bilety w wersji "Standard"! Więcej informacji na temat akademii 2024 znajdziecie tutaj. Dziękujemy ekipie Sekuraka za taką fajną zniżkę dla wszystkich Pasjonatów!

Akademia Sekuraka

Niedawno wystartował dodruk tej świetnej, rozchwytywanej książki (około 940 stron). Mamy dla Was kod: pasja (wpiszcie go w koszyku), dzięki któremu otrzymujemy 10% zniżki - dziękujemy zaprzyjaźnionej ekipie Sekuraka za taki bonus dla Pasjonatów! Książka to pierwszy tom z serii o ITsec, który łagodnie wprowadzi w świat bezpieczeństwa IT każdą osobę - warto, polecamy!

...