Ocena kodu JS - aplikacja Pomodoro Timer

pytanie zadane 1 września 2016 w JavaScript przez pietrzakacper Mądrala (7,480 p.)

Cześć!

Od początku lipca zacząłem się uczyć JavaScript. Właśnie skończyłem jeden z wielu małych projektów jakim jest Pomodoro Timer. Prosiłbym o obiektywną ocenę kodu który napisałem, chciałbym wiedzieć co można było zrobić lepiej :)

Sam kod JS jest pisany w ES6, ale w pliku html są podpięte skrypty po trans-kompilacji Babelem na ES5.

Kod: LINK

Aplikacja: LINK

Pozdrawiam i z góry dziękuje za ocenę :)

komentarz 1 września 2016 przez Spiral Obywatel (1,330 p.)

Na pierwszy rzut oka na samą aplikację w działaniu, mogę powiedzieć że jest dobrze, ale brakuje jakiejkolwiek reakcji wizualnej aplikacji na to, że kliknąłem start lub stop.
Po kliknięciu user przez moment ma wrażenie że coś nie działa, dopiero po chwili zauważa że timer ruszył. Także dodaj jakąś zmianę koloru przycisku na zielony czy coś podobnego żeby było czuć że timer ruszył, podobnie z zatrzymywaniem.

komentarz 1 września 2016 przez pietrzakacper Mądrala (7,480 p.)

W sumie nie pomyślałem o tym, dzięki jak tylko usiądę do komputera to to zmienie.

3 odpowiedzi

odpowiedź 2 września 2016 przez Comandeer Guru (606,240 p.)

https://github.com/pietrzakacper/Pomodoro/blob/gh-pages/es6/progress-bar.js

Czemu tak dziwacznie? Czemu nie zrobisz z tego klasy i nie zwrócisz jako moduł ES?

class progressBar {
	constructor( element ) {
		
		//cache DOM
		this.bar = element;
	}

	setValue( initialTime, actualTime ) {
		const percentageWidth= ( ( initialTime - actualTime ) / initialTime ) * 100;

		this.bar.style.width = `${ percentageWidth }%`;
	}
}

export progressBar;

I w tym momencie możesz mieć więcej niż jeden progress bar. BTW używaj const de facto zawsze. let używaj tylko do tych zmiennych, które będą się zmieniać. A zamiast łączenia stringów polecam template literals.

Ogólnie Twój kod jest średnio ES6… Raptem używasz tylko const i let, więc równie dobrze mógłbyś go przepisać na ES5 i pozbyć się Babela w całości.

W Timer zastanawia mnie też to przekazywanie renderFunc w konstruktorze. Dlaczego tak dziwnie?

BTW idealnie się tutaj nadadzą custom elements

komentarz 2 września 2016 przez pietrzakacper Mądrala (7,480 p.)

Ten dziwaczny zapis jest trochę moją zabawą z językiem i wynikiem chęci uzyskania enkapsulacji. Dla osoby która do tej pory pisała tylko w C++ i C# to taki zapis jest trochę egzotyczny, poza tym jest to pattern który kiedyś widziałem i wydał mi się sensowny: filmik Mógłbyś powiedzieć co konkretnie jest w nim złego, czy po prostu jest niestandardowy ?

O proszę nie wiedziałem o eksportowaniu i importowaniu modułów. W sumie ma to duży sens przy większych projektach.

Słyszałem o tej zasadzie (domyślne const ) i nawet myślałem, że ją stosuję, ale najwyraźniej jeszcze nie weszło mi to w nawyk.

Faktycznie całość spokojnie można by było szybko przepisać na ES5 ale, że czytam tę książkę to staram się stosować do jej treści. No i ten wygodny zapis nie był by możliwy bez ES6:

clock.innerHTML = `${(minutes<10)?'0'+minutes:minutes}:${(seconds<10)?'0'+seconds:seconds}`;

Co do przekazanie renderFunc to miało to służyć separacji warstw: logicznej Timera i obsługi DOM.

Poczytam o tych custom elements :) Może kolejny projekt (Tic Tac Toe) zrealizuje z ich użyciem.

Dzięki wielkie za odpowiedź :)

komentarz 2 września 2016 przez Comandeer Guru (606,240 p.)

Mógłbyś powiedzieć co konkretnie jest w nim złego, czy po prostu jest niestandardowy ?

ES6 wprowadziło rzeczywiste moduły, stąd ten pattern jest po prostu przestarzały.

komentarz 2 września 2016 przez pietrzakacper Mądrala (7,480 p.)

Ma sens, dzięki :)

odpowiedź 2 września 2016 przez uRTLy Bywalec (2,420 p.)
edycja 2 września 2016 przez uRTLy

Wyrzuć single var/let/const pattern bo jest słaby ( okej - kwestia sporna)

if(timer==null){
  renderTimer(initialMinutes,0);
}

Popracuj może nad jednolitym stylem, pod względem estetyki jest słabo bo nie oddzielasz spacjami operatorów, nie masz spacji po if, nawiasach, przed klamrami, po przecinkach albo są niejednolite.

zamiast sprawdzać czy coś jest null zrob

if (!!timer)

Poczytaj o Single Responsibility Pattern i np funkcja renderTimer

function renderTimer(minutes, seconds, initialMinutes) {
		clock.innerHTML = (minutes < 10 ? '0' + minutes : minutes) + ':' + (seconds < 10 ? '0' + seconds : seconds);
		progressBar.setValue(initialMinutes * 60, minutes * 60 + seconds);
	}

niech będzie odpowiedzialna tylko za render. Formatuj czas w innej funkcji stworzonej do tego - niech funkcje robią to o czym mówia ich nazwy . Moze ci sie wydawac ze nie warto robic oddzielnej funkcji dla jednej linijki kodu ale to dobra praktyka.

Mógłbyś dodać możliwość ustawienia czasu przerwy pomiedzy sesjami.

Oprócz tego nie rób niczego hardkodowanego. Zrób z tego moduł który każdy może przyczepić na swoja stronę i podać mu własne elementy DOM.

Co jeśli ktoś chciałby mieć dwa całkowicie niezalezne zegary na stronie które się ze sobą nie gryzą ? Pomyśl nad tym :D.

komentarz 2 września 2016 przez pietrzakacper Mądrala (7,480 p.)

Nie do końca rozumiem, co masz na myśli pisząc var/const/let pattern, a już na pewno nie wiem czemu miałby być zły :)

Faktycznie nie zwracam dość dużej uwagi na spacing w moim stylu pisania kodu. Ale od teraz zacznę :) .

Co do zasady pojedynczej odpowiedzialności to rozumiem potrzebę wykreowania takiego nawyku, w kolejnych projektach będę się stosował.

Fajny pomysł z robieniem uniwersalnych modułów, nastepny projekt postaram się stworzyć w ten sposób.

Dzięki za odpowiedź!

komentarz 2 września 2016 przez uRTLy Bywalec (2,420 p.)

widziałem gdzieś poprostu coś takiego

let x, y = "cos tam", z;

To jest single var pattern. Ogólnie lepszą praktyką jest pisanie każdej zmiennej z nowym let/var/const. Chociaż to kwestia sporna i nie będe nikogo przekonywał co do świętości moich pogladów to wydaje mi się że kod jest jednak mniej problematyczny i jaśniejszy.

komentarz 2 września 2016 przez pietrzakacper Mądrala (7,480 p.)

a ok, rozumiem. Moim zdaniem też deklaracja zmiennych każdej w nowej linii jest czytelniejsza, jednak przy tak małej ilośc zmiennych nie ma to raczej znaczenia i mi nie przeszkadzało :)

odpowiedź 2 września 2016 przez pietrzakacper Mądrala (7,480 p.)

Podbijam, bo może komuś się zachce ocenić, a ja zaczynam właśnie kolejny projekt i nie chcę robić tych samych błędów :)

komentarz 2 września 2016 przez Adam Jakś Dyskutant (8,940 p.)

Czasem brak odpowiedzi jest znaczącą odpowiedzią :)

Kod wygląda całkiem profesjonalnie, wiele osób nie potrafiłoby go napisać w ten sposób dlatego zostawia plusa i wychodzi.

Ode mnie:

1. Nie wiem po co do każdego dołączanego pliku dodajesz tyle przeróżnych atrybutów, typu:

charset="utf-8"

2. Osobiście chyba uznałbym za trochę bezsensowne dodawanie Bootstrapa do tak prostego projektu.

3. Dodałbym blokowanie reakcji przycisków gdy istotnie nie powinny one reagować (np. przy kliknięciu stop przy wyłączonym timerze Firebuga wyrzuca błąd).

komentarz 2 września 2016 przez pietrzakacper Mądrala (7,480 p.)

1. Atom mi generuje ten zapis przy wpisaniu "script" :)

2. Tutaj przyznam, że Bootstrapa załączam domyślnie do każdego projektu, bo bez większego myślnienia mogę zrobić szybko responsywny layout. Trochę zaniedbałem CSS-a, bo celuje w backend. No ale fakt, do projektu tej skali to bez sensu dołączać zewnętrzną bibliotekę jak dość łatwo można samemu uzyskać ten sam efekt.

3. Kurde, wstawiłem parę godzin temu małą "poprawkę", w której zapomniałem sprawdzić czy timer istnieje, pół linii kodu i będzie znowu działać :)

Dzięki za odpowiedź!