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

Ocena kodu JS - aplikacja Pomodoro Timer

Aruba Cloud VPS - 50% taniej przez 3 miesiące!
+5 głosów
485 wizyt
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

+2 głosów
odpowiedź 2 września 2016 przez Comandeer Guru (606,160 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.

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,160 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 :)
+1 głos
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 :)
0 głosów
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 :)
1
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ź!

Podobne pytania

+1 głos
1 odpowiedź 231 wizyt
pytanie zadane 24 sierpnia 2016 w JavaScript przez Kamil Naja Nałogowiec (27,550 p.)
+2 głosów
3 odpowiedzi 1,110 wizyt
pytanie zadane 9 lipca 2017 w JavaScript przez Ziken Początkujący (330 p.)
+1 głos
2 odpowiedzi 1,516 wizyt
pytanie zadane 10 lipca 2018 w JavaScript przez MrxCI Dyskutant (8,260 p.)

93,113 zapytań

142,093 odpowiedzi

321,656 komentarzy

62,453 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

Wprowadzenie do ITsec, tom 1 Wprowadzenie do ITsec, tom 2

Można już zamawiać dwa tomy książek o ITsec pt. "Wprowadzenie do bezpieczeństwa IT" - mamy dla Was kod: pasja (użyjcie go w koszyku), dzięki któremu uzyskamy aż 15% zniżki! Dziękujemy ekipie Sekuraka za fajny rabat dla naszej Społeczności!

...