JS - ocena kodu

pytanie zadane 22 kwietnia 2017 w JavaScript przez hoktaur Pasjonat (22,250 p.)
zamknięte 23 kwietnia 2017 przez hoktaur

Proszę o ocenę mojego zegarka binarnego:

<!doctype html>

<html lang="pl">
<head>
  <meta charset="utf-8">

  <title>Zegar binarny</title>
  <meta name="description" content="">
  <meta name="author" content="">

  <link rel="stylesheet" href="css/styles.css?v=1.0">

  <!--[if lt IE 9]>
    <script src="https://cdnjs.cloudflare.com/ajax/libs/html5shiv/3.7.3/html5shiv.js"></script>
  <![endif]-->
  <style>
    body {
      margin: 0px;
      padding: 0px;
    } 
    .container {
      background-image: url('https://static.pexels.com/photos/99577/barn-lightning-bolt-storm-99577.jpeg');
      background-size: cover;
      background-position: center;
      display: flex;
      justify-content: center;
      align-items: center; 
      position: fixed;
      height: 100%;
      width: 100%
    }
    .clock {
      display: flex;
    }
    .clock > .digit {
      display: flex;
      flex-direction: column-reverse;
    } 
    .clock > .digit > div {
      width: 3vw;
      height: 3vw;
      margin: 5px;
      border: 1px solid white;
      transition: all 0.5s;
      box-shadow: 0px 0px 5px lightgrey, 0px 0px 15px grey;
    }
    .active {
      background-color: rgba(255, 255, 255, 0.9);
      border-radius: 50px;
      transform: rotate(180deg);
    }
  </style>
</head>

<body>
  <div class="container">
    <div class="clock">
    </div>  
  </div>
  <script>
    function Clock(node) {
      this.node = node,
      this.colNumber;

      this.init = function() {
      var numberBitsInCols = [4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4],
          numberBitsInCol,
          clockDigit,
          clockDigitBit,
          i,
          j;

        this.colNumber = numberBitsInCols.length;

        for(numberBitsInCol of numberBitsInCols) {
          clockDigit = document.createElement('div');
          clockDigit.classList.add('digit');

          for(j = 0; j < numberBitsInCol; j++) {
            clockDigitBit = document.createElement('div');
            clockDigit.appendChild(clockDigitBit);
          }
          this.node.appendChild(clockDigit);
        }
      } 

      this.update = function() {
        var curDate = new Date(),
            y = curDate.getFullYear(),
            mo = curDate.getMonth() + 1,
            d = curDate.getDate(),
            h = curDate.getHours(),
            m = curDate.getMinutes(),
            s = curDate.getSeconds(),
            i,
            j;

        if(mo < 10) mo = '0' + mo;
        if(d < 10) d = '0' + d;
        if(h < 10) h = '0' + h;
        if(m < 10) m = '0' + m;
        if(s < 10) s = '0' + s;
        
        var time = '' + y + mo + d + h + m + s;

        for(i = 0; i < this.colNumber; i++) {
          var bit = 1,
              nodeLength = this.node.children[i].children.length;
          
          for(j = 0; j < nodeLength; j++) {
            if(time[i] & bit) {
              this.node.children[i].children[j].classList.add('active');
            } else {
              this.node.children[i].children[j].classList.remove('active');
            }
            bit *= 2;
          } 
        } 
      }  
    }      
  
    var clock1 = new Clock(document.querySelector('.clock'));
    clock1.init();
    setInterval(function() { clock1.update() }, 1000);
  </script>
</body>
</html>

P.S. Wiem że to jest w jednym kodzie pomieszane z HTML i CSS ale tak to chciałem zamieścić...

komentarz zamknięcia: Nie było więcej uwag dzięki dla tych co zabrali głos :)

1 odpowiedź

odpowiedź 22 kwietnia 2017 przez Kornelia Kobiela Nałogowiec (33,340 p.)
wybrane 23 kwietnia 2017 przez Kornelia Kobiela

Najlepsza

Całkiem ładnie i składnie ci to wyszło. Co do moich uwag, to nie ma ich dużo:

Unikałabym jednoznakowych nazw zmiennych - niech mówią same za siebie, oprócz iteratorów. Ale też nie widzę sensu deklarowania iteratora przed pętlą.
Zero przed liczbą mniejszą od 10 wyrzuciłabym do osobnej funkcji.
Jak się bawisz obiektowo, to metody chyba lepiej dodać do prototypu, a nie konstruktora

Poza tym jest ok. Pozdrawiam

komentarz 22 kwietnia 2017 przez hoktaur Pasjonat (22,250 p.)

Właśnie na takiej odp mi zależało thx :)

Przeglądając to co napisałąś:

Unikałabym jednoznakowych nazw zmiennych - niech mówią same za siebie, oprócz iteratorów

masz racje muszę to poprawić.

Ale też nie widzę sensu deklarowania iteratora przed pętlą.

Na MDN piszą że i tak wszystkie deklaracje zmienny w funkcji są dokonywane na początku (bez względu na miejsce w kodzie) po odwołaniu się do niej aczkolwiek zalecają deklarację na początku ze względu na przejrzystość które zmienne są globalne które lokalne

Zero przed liczbą mniejszą od 10 wyrzuciłabym do osobnej funkcji.

dobry pomysł ... przeoczyłem

Jak się bawisz obiektowo, to metody chyba lepiej dodać do prototypu, a nie konstruktora

gdzieś tam doczytałem że włożenie do konstruktora może mieć wpływ na wydajność więc tak masz rację też do poprawy

komentarz 22 kwietnia 2017 przez hoktaur Pasjonat (22,250 p.)

Tak to wyszło po poprawkach:

<!doctype html>

<html lang="pl">
<head>
  <meta charset="utf-8">

  <title>Zegar binarny</title>
  <meta name="description" content="The HTML5 Herald">
  <meta name="author" content="SitePoint">

  <link rel="stylesheet" href="css/styles.css?v=1.0">

  <!--[if lt IE 9]>
    <script src="https://cdnjs.cloudflare.com/ajax/libs/html5shiv/3.7.3/html5shiv.js"></script>
  <![endif]-->
  <style>
    body {
      margin: 0px;
      padding: 0px;
    } 
    .container {
      background-image: url('https://static.pexels.com/photos/99577/barn-lightning-bolt-storm-99577.jpeg');
      background-size: cover;
      background-position: center;
      display: flex;
      justify-content: center;
      align-items: center; 
      position: fixed;
      height: 100%;
      width: 100%
    }
    .clock {
      display: flex;
    }
    .clock > .digit {
      display: flex;
      flex-direction: column-reverse;
    } 
    .clock > .digit > div {
      width: 3vw;
      height: 3vw;
      margin: 5px;
      border: 1px solid white;
      transition: all 0.5s;
      box-shadow: 0px 0px 5px lightgrey, 0px 0px 15px grey;
    }
    .active {
      background-color: rgba(255, 255, 255, 0.9);
      border-radius: 50px;
      transform: rotate(180deg);
    }
  </style>
</head>

<body>
  <div class="container">
    <div class="clock">
    </div>  
  </div>
  <script>
    function BinaryClock(node) {
      this.node = node,
      this.ColsBitsCombination = [4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4],
      this.colsNumber = this.ColsBitsCombination.length;
    }
 
    BinaryClock.prototype.init = function() {
      var numberBitsInCol,
          clockDigit,
          clockDigitBit,
          i,
          j;

      for(numberBitsInCol of this.ColsBitsCombination) {
        clockDigit = document.createElement('div');
        clockDigit.classList.add('digit');

        for(j = 0; j < numberBitsInCol; j++) {
          clockDigitBit = document.createElement('div');
          clockDigit.appendChild(clockDigitBit);
        }
        this.node.appendChild(clockDigit);
      }
    } 

    BinaryClock.prototype.update = function() {
      var nodeCurrentChildClassList,
          curDate = new Date(),
          curDateYear = this.concat0IfLessThan10(curDate.getFullYear()),
          curDateMonth = this.concat0IfLessThan10(curDate.getMonth() + 1),
          curDateDay = this.concat0IfLessThan10(curDate.getDate()),
          curDateHours = this.concat0IfLessThan10(curDate.getHours()),
          curDateMinutes = this.concat0IfLessThan10(curDate.getMinutes()),
          curDateSeconds = this.concat0IfLessThan10(curDate.getSeconds()),
          curDateString = '' + curDateYear + curDateMonth + curDateDay + curDateHours + curDateMinutes + curDateSeconds,
          bit,
          i,
          j;

      for(i = 0; i < this.colsNumber; i++) {
        bit = 1;
        
        for(j = 0; j < this.ColsBitsCombination[i]; j++) {
          nodeCurrentChildClassList = this.node.children[i].children[j].classList;
          (curDateString[i] & bit) ? nodeCurrentChildClassList.add('active') : nodeCurrentChildClassList.remove('active');
       
          bit *= 2;
        } 
      } 
    }
    
    BinaryClock.prototype.concat0IfLessThan10 = function(digit) {
      return (digit < 10) ? '0' + digit : digit;
    } 
  
    var clock1 = new BinaryClock(document.querySelector('.clock'));
    clock1.init();
    setInterval(function() { clock1.update() }, 1000);
  </script>
</body>
</html>

Czy teraz jest ok? czy jeszcze coś da się ulepszyć?

komentarz 22 kwietnia 2017 przez Tomek Sochacki Ekspert (227,490 p.)

Na MDN piszą że i tak wszystkie deklaracje zmienny w funkcji są dokonywane na początku (bez względu na miejsce w kodzie) po odwołaniu się do niej aczkolwiek zalecają deklarację na początku ze względu na przejrzystość które zmienne są globalne które lokalne

Nie do końca... owszem hoisting działa w ten sposób, że deklaracje lecą na początek zakresu leksykalnego ale tylko w przypadku stosowania instrukcji var. Poczytaj sobie w wolnej chwili o deklaracjach let i const (tworzących w pewnym sensie zakres blokowy... choć w praktyce nie do końca). Pozdrawiam

komentarz 22 kwietnia 2017 przez hoktaur Pasjonat (22,250 p.)

thx tego nie wiedziałem, ale doczytałem i już wiem :)

komentarz 22 kwietnia 2017 przez Tomek Sochacki Ekspert (227,490 p.)

zanim jednak zaczniesz stosować nową składnię ES6+ przemyśl w jakich środowiskach będzie odpalany Twój kod i w razie czego doczytaj jeszcze na temat transpilacji kodu, np. Babel.

komentarz 22 kwietnia 2017 przez Kornelia Kobiela Nałogowiec (33,340 p.)

Widzę ciekawą dyskusję, a ze swojej strony dodam, że nie ma czegoś takiego jak idealny kod w JS. Jest tyle stylów pisania kodu, tyle konwencji, a ten twój jest już naprawdę w porządku

komentarz 22 kwietnia 2017 przez Tomek Sochacki Ekspert (227,490 p.)

Jeśli chciałbyś jeszcze bardziej wgłębić się w ES6 to możesz np. wykorzystać metodę Array.prototype.fill do stworzenia tablicy wypełnionej jednakowymi elementami:

//Zamiast:
let ColsBitsCombination = [4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4];

//Można zrobić:
let ColsBitsCombination = Array(14).fill(4)

Rozwiązanie to daje lepszą kontrolę nad ilością tworzonych elementów - w Twojej wersji musisz liczyć elementy w czasie deklarowania tablicy, a ja po prostu mogę w razie czego modyfikować jeden elementy Array(14). Wywołanie Array(14) tworzy co prawda tablicę z tzw. pustymi elementami, na co możesz natknąć się czasem w różnych artykułach czy książkach, ale na razie nie zaprzątaj sobie tym głowy.

Na koniec polecam jeszcze lekturę https://developer.mozilla.org/pl/docs/Web/API/Document/createDocumentFragment gdzie omówiono metodę createDocumentFragment. W dużym skrócie - jeśli stosujesz "standardowe" createElement i potem dla każdego z nich appendChild to de facto co chwilę modyfikujesz DOM. Jeśli takich operacji masz więcej to createDocumentFragment umożliwia wcześniejsze utworzenie całej struktury, i na koniec dopiero wrzucenie jej całej do DOM z jednym tylko odświeżeniem strony.

Poruszone przeze mnie kwestie nie odnoszą się do żadnych błędów w Twoim kodzie - pytałeś czy można go jakoś usprawnić, więc podrzucam tylko moje propozycje :)

Pozdrawiam

komentarz 22 kwietnia 2017 przez hoktaur Pasjonat (22,250 p.)
edycja 22 kwietnia 2017 przez hoktaur

Dzięki za kolejne podpowiedzi do createDocumentFragment napewno zajrze ale już nie dziś (P. S. już zajrzałem ... nie mogłem się powstrzymać :) )

co do zaś wyglądu samej tablicy ColsBitsCombination to na początku nie wyglądała ona tak jak na efekcie końcowym (4bity x 14 kolumn)zacząłem od takiego założenia

więc było mi to potrzebne i tak zostało bo nie zdecydowałem czy bardziej podoba mi się taki widok jak na efekcie końcowym czy taki jak zakładał projekt, ale sam pomysł z fill super może gdzieś go kiedyś wykorzystam