Ocena małego projektu JS (Node.js) + Bash

1

Chciałbym poprosić Was o ocenę mojego niedawno rozpoczętego projektu z logiką w JavaScript w Node.js i z interfejsem CLI w Bashu:

https://github.com/silvuss/silvuss-bracket-string-validator

Projekt jest niewielki, jego funkcjonalność jest znikoma (zresztą przeczytajcie README). Niemniej pokładam w nim dwie nadzieje:

  • stanie się kiedyś wizytówką mnie dla samego siebie; co przez to rozumiem? między innymi: będzie dobrze wypozycjonowany, będzie mieć dobrze wymyśloną architekturę, dobrze napisany kod i dokumentację, ukończone wszystkie issues, zrozumiałe commit messages i naprawione wszystkie błędy, jakie można w nim znaleźć (pomijając rzeczy niezależne ode mnie, np. pewne niezbyt trafne konstrukcje języków);
  • będę mógł podpiąć pod niego – jako GUI – interfejs w Angularze; dzięki temu moja nauka podstaw Angulara pójdzie do przodu; obecnie jest jakby zamrożona.

Zależy mi na ocenie całego projektu na GutHubie (nie tylko aplikacji). M.in. chodzi mi o ocenę następujących rzeczy:

  • wydań – czy opisy są zrozumiałe; może dobrze będzie dołączać jakieś pliki, może cały projekt w innym formacie niż obecnie (obecnie jest tylko skompresowany kod źródłowy);
  • issues – szczególnie istotne; czy są dobrze wydzielone, czy nie za dużo / za mało;
  • architektury, w tym obsługi błędów – szczególnie istotne; do poprawy, ale nie mam za bardzo pomysłów, więc chętnie poznam jakiekolwiek sugestie;
  • kodu, w tym komentarze – podział na funkcje, pliki, czytelność, zwięzłość/rozwklekłość, nazwy zmiennych itd.;
  • dokumentacji – szczególnie istotne; np. czy jest czytelna i zrozumiała, czy nie za długa, czy czegoś w niej nie brakuje;
  • czy logika jest w odpowiednim miejscu; może np. trzeba przenieść część z CLI do logiki, a może część z logiki do CLI;
  • czy struktura katalogów w projekcie jest w porządku (jest moja – nie sugerowałem się niczym konkretnym).

Z racji niewielkiego rozmiaru projektu (nie bardzo może być co krytykować :P) najbardziej proszę w powyższych punktach o wskazówki dotyczące wzorców projektowych, które mógłbym użyć, metodologii, bibliotek, frameworków oraz co jeszcze mógłbym dodać jako funkcjonalność do tego projektu. Nowa funkcjonalność powinna być jakoś powiązana ze starą; w tym projekcie nie zależy mi na robieniu aplikacji, która pierze, sprząta i gotuje.

Jak zwykle – wszelkie sugestie są mile widziane. :) W tym – literówki i błędy w dokumentacji czy w komentarzach (tym bardziej, że są po angielsku).

Postaram się tutaj opisywać zmiany w projekcie, dopóki wszystkich sugestii nie rozpatrzę.


Na koniec, korzystając z okazji, chciałbym podziękować dwóm osobom:

  • użytkownikowi @Mr.YaHooo za umieszczenie na naszym forum pierwszej wersji algorytmu sprawdzającego, czy ciąg znaków jest wyrażeniem nawiasowym w tym poście;
  • użytkowniczce @Kate321 za założenie tego wątku; dzięki temu w ogóle poznałem ideę wyrażeń nawiasowych. Trochę szkoda, że już nie wchodzi na forum. @Kate321, jeśli to czytasz, napisz proszę.

UPDATE 19 lipca 2019: Dodałem w projekcie komentarze JSDoc (jak ktoś zna Javę – Wikipedia określa je jako podobne do komentarzy Javadoc). Tutaj wydanie, które to opisuje: https://github.com/silvuss/silvuss-bracket-string-validator/releases/tag/v1.1.0


UPDATE 28 lipca 2019: Kolejne, duże, wydanie mojej aplikacji: 1.2.0 (link). Dodałem testy jednostkowe; wykorzystuję przy nich frameworki Mocha oraz Jest. Z innych zmian – nareszcie włączyłem wsparcie dla usługi Travis CI.

7

Zgadzam się z tym co piszesz - projekt jest dość mały, ciężko coś sensownie skomentować. Dlatego przyczepię się do drobiazgów :p

To, co zaraz napiszę, grozi flamem pokroju "tabs vs spaces", ale moim zdaniem te wcięcia są za duże. Nie lubię takich mocnych, sam zawsze ustawiam Tab na 2 spacje i tak jest moim zdaniem optymalnie. Większe wcięcia powodują, że po pierwsze kod wydaje się taki rozstrzelony/rozbity, a po drugie przy wielokrotnych zagnieżdżeniach (których oczywiście należy z całą stanowczością unikać ;)) nagle mamy wcięcie na pół ekranu.

Zastanawiam się też, na ile walenie takiej ilości komentarzy ma sens. Gdzieś z tyłu głowy mi dzwoni pewna opinia wyznawana przez bardzo wielu programistów (i jedna z głównych zasad przewijających się cały czas przez "Czysty kod" Martina), czyli że kod powinien sam się dokumentować, komentarze stosuj jedynie w sytuacji, gdzie są konieczne i to w minimalnej ilości.

// JavaScript does not have the stack implemented as such,
// therefore here it is substituted with Array
Wydaje mi się, że kod aplikacji nie jest odpowiednim miejscem do serwowania wykładów odnośnie struktury danego języka.

if (stack.length === 0) {
// We cannot pop from an empty stack,
//  therefore we have to return before
return false; 

Czy naprawdę jest sens takie oczywiste rzeczy wyjaśniać?
Jeśli ktoś nie zrozumie, co robi if (stack.length === 0) {return false} to znaczy, że Twój kod nie jest dla niego przeznaczony, bo koleś podstaw nie ogarnia. No chyba, że piszesz książkę dla początkujących, a ta aplikacja ma być kodem dołączonym do któregoś z rozdziałów ;)

3

Zazwyczaj pliki binarne umieszamy w katalogu /bin, jak widzę katalog bin w projekcie to mogę sobie go wylistować i już wiem jakie mogę wykonać polecenia.

Testy zazwyczaj są w katalogu /tests (wtedy nie musimy ich szukać po całym projekcie), dodatkowo nie musimy testować każdego przypadku, z tego co widzę masz ich kilkaset, wystarczyło by 10-20 przypadków testowych (uwzględniając te skrajne). Z założenia testy jednostkowe mają działać szybko, dlatego nie możemy testować wszystkiego. Dobra, widzę, że to testy generowane automatycznie - skoro jesteśmy w stanie je generować automatycznie to może niepotrzebe są te pliki *.json, wystarczyło by je sprawdzać "w locie" podczas uruchamiania testów.

Ogólnie bardzo, bardzo dużo linijek, kodu, dokumentacji i komentarzy a cały projekt mógłby się zamknąć w kilku plikach z kodem js (np. w katalogu src), 3 binach (do odpalenia walidacji, testów i benchmaraków), 1 pliku testowym (w tests) i ew. jednym pliku do benchmarku - często kod który piszemy jest używany przez inne osoby, im więcej zrobimy tego kodu tym więcej czasu zajmie komuś jego przeczytanie.

Ok, po głębszym wczytaniu rozumiem, że chciałeś zrobić porównanie różnych metod na wykonanie tego samego zadania.

Dostajesz od biznesu polecenie:
Panie Silv potrzebujemy dopisać jeszcze jeden algorytm o nazwie "ala-ma-kota" który działa tak - (... opis działania ...).

Pytanie do Ciebie: Ile plików musisz zmienić?

Odpowiedź w idealnym projekcie: 2 lub 3 (albo nawet 1 zakładając, że nie było by dokumentacji)
1 - zawierający algorytm
2 - jakiś config (można go ew. wyelminować jeżeli algorytmy były by w konkretnym katalogu po którym interujemy)
3 - dokumentacja

Odpowiedź w Twoim projekcie: 9
dokumentacje, biny, json i właściwy kod

Jeżeli jednak ten skrypt byłby bardziej zaawansowany, albo nawet byłby to jakiś projekt czy coś który był pisany X lat temu i dostałbym go do utrzymywania - to ta cała otoczka mogła by się przydać. Jednak w tej formie, jest to przerost formy nad treścią.

Widać, że włożyłeś w to dużo serca i czasu - powodzenia w rozwijaniu ;)

2
cerrato napisał(a):

Zgadzam się z tym co piszesz - projekt jest dość mały, ciężko coś sensownie skomentować. Dlatego przyczepię się do drobiazgów :p

I o to chodzi!

To, co zaraz napiszę, grozi flamem pokroju "tabs vs spaces", ale moim zdaniem te wcięcia są za duże. Nie lubię takich mocnych, sam zawsze ustawiam Tab na 2 spacje i tak jest moim zdaniem optymalnie. Większe wcięcia powodują, że po pierwsze kod wydaje się taki rozstrzelony/rozbity, a po drugie przy wielokrotnych zagnieżdżeniach (których oczywiście należy z całą stanowczością unikać ;)) nagle mamy wcięcie na pół ekranu.

Ja mam odwrotnie niż Ty – nie lubię zmian i dlatego staram się nie zmieniać wartości domyślnych w edytorach. A że Visual Studio Code ma takie wcięcia domyślne dla kodu JS, kodu Basha oraz dla JSON-a, to i takie są.

Zastanawiam się też, na ile walenie takiej ilości komentarzy ma sens. Gdzieś z tyłu głowy mi dzwoni pewna opinia wyznawana przez bardzo wielu programistów (i jedna z głównych zasad przewijających się cały czas przez "Czysty kod" Martina), czyli że kod powinien sam się dokumentować, komentarze stosuj jedynie w sytuacji, gdzie są konieczne i to w minimalnej ilości.

That may be true. Fajnie, że dałeś odniesienie do Martina (kiedy ja tę książkę przeczytam?). Zobaczę, co się da z tym zrobić.

// JavaScript does not have the stack implemented as such,
// therefore here it is substituted with Array
Wydaje mi się, że kod aplikacji nie jest odpowiednim miejscem do serwowania wykładów odnośnie struktury danego języka.

Sam nie wiem, dlaczego, ale obecnie wydaje mi się to potrzebne.

if (stack.length === 0) {
// We cannot pop from an empty stack,
//  therefore we have to return before
return false; 

Czy naprawdę jest sens takie oczywiste rzeczy wyjaśniać?
Jeśli ktoś nie zrozumie, co robi if (stack.length === 0) {return false} to znaczy, że Twój kod nie jest dla niego przeznaczony, bo koleś podstaw nie ogarnia. No chyba, że piszesz książkę dla początkujących, a ta aplikacja ma być kodem dołączonym do któregoś z rozdziałów ;)

Nie planowałem tego kodu dla początkujących w JS (ale nie obraziłbym się, gdyby ktoś kiedyś ;) ). Niemniej akurat w tym wypadku uważam, że jest sens – jest to algorytm, który nie był dla mnie na początku oczywisty. I myślę, że dla niewielu osób będzie – w końcu tak proste i nieużyteczne algorytmy czyta się raz i zapomina. Więc dobrze, by był jasny w każdym calu.


Markuz napisał(a):

Zazwyczaj pliki binarne umieszamy w katalogu /bin, jak widzę katalog bin w projekcie to mogę sobie go wylistować i już wiem jakie mogę wykonać polecenia.

OK, to w zasadzie już wiem. Tylko widzisz – problem jest taki, że na razie nie wiem, jak taki plik binarny utworzyć i co powinien zawierać w przypadku tej aplikacji.

Testy zazwyczaj są w katalogu /tests (wtedy nie musimy ich szukać po całym projekcie), dodatkowo nie musimy testować każdego przypadku, z tego co widzę masz ich kilkaset, wystarczyło by 10-20 przypadków testowych (uwzględniając te skrajne). Z założenia testy jednostkowe mają działać szybko, dlatego nie możemy testować wszystkiego. Dobra, widzę, że to testy generowane automatycznie - skoro jesteśmy w stanie je generować automatycznie to może niepotrzebe są te pliki *.json, wystarczyło by je sprawdzać "w locie" podczas uruchamiania testów.

Testy były generowanie, jak napisałem, semi-automatically (chyba że mówisz o danych do benchmarku?) – przypadki były generowane automatycznie, ale te przypadki, które są poprawne, musiałem sam wyszukiwać i opisywać jako true. :) Ogólnie wolę czegoś takiego nie robić, bo człowiek jest omylny... Ale inaczej się nie dało.

Ogólnie bardzo, bardzo dużo linijek, kodu, dokumentacji i komentarzy a cały projekt mógłby się zamknąć w kilku plikach z kodem js (np. w katalogu src), 3 binach (do odpalenia walidacji, testów i benchmaraków), 1 pliku testowym (w tests) i ew. jednym pliku do benchmarku - często kod który piszemy jest używany przez inne osoby, im więcej zrobimy tego kodu tym więcej czasu zajmie komuś jego przeczytanie.

Do przemyślenia, bo poruszasz wiele spraw. Część z nich mam już w głowie.

Ok, po głębszym wczytaniu rozumiem, że chciałeś zrobić porównanie różnych metod na wykonanie tego samego zadania.

No, wydaje się to dziwne na początku. :P

Dostajesz od biznesu polecenie:
Panie Silv potrzebujemy dopisać jeszcze jeden algorytm o nazwie "ala-ma-kota" który działa tak - (... opis działania ...).

Pytanie do Ciebie: Ile plików musisz zmienić?

Odpowiedź w idealnym projekcie: 2 lub 3 (albo nawet 1 zakładając, że nie było by dokumentacji)
1 - zawierający algorytm
2 - jakiś config (można go ew. wyelminować jeżeli algorytmy były by w konkretnym katalogu po którym interujemy)
3 - dokumentacja

Odpowiedź w Twoim projekcie: 9
dokumentacje, biny, json i właściwy kod

Jeżeli jednak ten skrypt byłby bardziej zaawansowany, albo nawet byłby to jakiś projekt czy coś który był pisany X lat temu i dostałbym go do utrzymywania - to ta cała otoczka mogła by się przydać. Jednak w tej formie, jest to przerost formy nad treścią.

To jest ważna kwestia do rozważenia. Dzięki!

Widać, że włożyłeś w to dużo serca i czasu - powodzenia w rozwijaniu ;)

Dzięki. :)

2

Dziękujesz dwóm osobom za ich wątki na tym forum a do Sources w README tych linków już nie dodałeś :)

0

@baant: rzecz wygląda tak: jeden ze wspomnianych linków już jest w Credits. W Sources podaję odniesienia tylko do takich źródeł, które w mojej ocenie przekazują odpowiednio dużą wiedzę na temat dziedzin i technologii, które wykorzystuje projekt. Drugi ze wspomnianych przez Ciebie linków mógłbym też dać do Credits. I chyba to zrobię.

0
if (!bracketString) {
        // The argument is a falsy value
        //  (which includes being an empty string)
        // See https://developer.mozilla.org/en-US/docs/Glossary/Falsy
        return false;
    }

    if (!bracketString.substring) {
        // The argument is not a string
        return false;
    }

IMO tego typu komentarze są zdecydowanie zbędne i tylko utrudniają czytanie całości

0

@danek: pierwszy rzeczywiście można wyrzucić. Drugiego nie, bo sprawdzanie istnienia metody substring jest moim zdaniem wystarczająco niestandardowe, by nie być jasnym bez takiego komentarza.

0

Hej, dodałem w aplikacji komentarze JSDoc (jak ktoś zna Javę – Wikipedia określa je jako podobne do komentarzy Javadoc). Tutaj wydanie, które to opisuje: https://github.com/silvuss/silvuss-bracket-string-validator/releases/tag/v1.1.0

JSDoc wcześniej prawie nie znałem. Zapraszam do oceny, czy mają sens. :)

2

Co to za aplikacja której nie możesz odpalić. Jak się boisz, że coś skopałeś to wpakuj to w Dockera. Odpaliłem, działa ok, możesz dodać:

Dockerfile

FROM node:10.16.0
WORKDIR /usr/src/app
COPY . .

Odpalanie:

W katalogu głównym aplikacji gdzie znajduje się Dockerfile uruchom jednorazowo:

`docker build -t bracket-validator .`

Aby odpalić apkę wpisz:

`docker run -it bracket-validator /bin/bash`

0

@Maciej Cąderek: nie znam Dockera, ale domyślam się, że to coś do wirtualizacji pojedynczych aplikacji. Poczytam. Może przydać się w przyszłości. Dzięki!

Co do uruchomienia aplikacji jako takiego – nie rozumiem, dlaczego piszesz, że nie możesz odpalić, skoro potem piszesz, że możesz?

0

Ty piszesz, że nie powinienem. Odpaliłem w Dockerze - docker jest odizolowany, mogę tam nawet rm -rf --no-preserve-root / odpalić bez szkody dla własnego komptera.

0

A, odnosisz się do mojego README. ;) Tak, rzeczywiście napisałem w ten deseń, ale pozwolisz, że podam dokładny cytat:

This application is an example application that is not intended to be run. Its purpose is only to show code that is known to work. While probably nothing dangerous would happen, you may run it only under your own responsibility.

Mam na myśli tutaj, że nie rekomenduję żadnego bezpiecznego środowiska do uruchomienia tej aplikacji. Ponieważ ludzie są omylni, tak więc wskazywanie (np. przeze mnie) konkretnych zewnętrznych środowisk do uruchamiania konkretnych moich aplikacji byłoby obarczone pewnym błędem. Jest on dla mnie za duży do przyjęcia. Aby go uniknąć, ograniczam ryzyko do minimum. Uważam, że to, co napisałem, zgadza się z tym, czego sam potrzebuję jako twórca aplikacji.

Powyżej ograniczam margines błędu do świadomego wyboru użytkownika. W przypadku tej aplikacji rekomenduję ograniczenie marginesu błędu jeszcze bardziej – do uruchamiania w tym samym środowisku, co i ja. Pro forma zacytuję README:

Info: It is adviced to run this application in the same environment as it has been tested. In case of other environments this application may work properly, or its usage may cause unknown side effects, or it may not work at all.


UPDATE: I prawdopodobnie będę dodawać takie ograniczenia jak to drugie w przypadku każdej mojej aplikacji, która będzie zawierać skrypty powłoki.

0

@Maciej Cąderek: przeczytałem dwa artykuły o Dockerze i nie wydaje mi się, żeby był istotny dla mojego dewelopmentu, przynajmniej w tej chwili. Jeśli miałbyś jakieś wskazówki odnośnie tego, czy i kiedy będę mógł go użyć przy tej aplikacji, to chętnie.

W tej chwili, z rzeczy przyszłych – bardziej widziałbym wirtualizację na poziomie systemu operacyjnego. Przyda się, kiedy będę wprowadzać kompatybilność z różnymi systemami. Na podstawowym poziomie, mam nadzieję, zostanie to zapewnione przez Travis CI.


UPDATE: Gdyby bardziej zaawansowany poziom konfiguracji był potrzebny, zawsze mogę spróbować postawić u siebie wirtualkę. Słyszałem o darmowych z Windowsem. Nie wiem nic o wirtualizacji urządzeń mobilnych. Trochę mnie martwi MacOS, gdybym go planował; albo mogę odpuścić go w ogóle, albo spróbować coś innego niż Travis. A może wtedy już aplikacja się tak rozrośnie, że będzie sama na siebie zarabiać = będę mógł zacząć na nią wydawać pieniądze? <zastanawia się>

Ale to pieśń przyszłości dalszej niż ten miesiąc.

0

Nie mówię, że jest to istotne dla twojego dewelopmentu, mówię, że to dobry dodatek dla osób, które chcą bez ryzyka pobawić się twoim programem, bez analizowania kodu źródłowego.

Co do kompatybilności i ogólnie basha - czemu nie wybrałeś cli w JSie? I tak apka Node'a wymaga ;)

0
Maciej Cąderek napisał(a):

Co do kompatybilności i ogólnie basha - czemu nie wybrałeś cli w JSie? I tak apka Node'a wymaga ;)

To jest dobre pytanie. Najbliższe prawdy będzie chyba, że chciałem pokazać, że umiem napisać coś w Bashu. A Bash wydaje mi się (wciąż) najbardziej oczywistym wyborem, jeśli chodzi o CLI. A CLI wybrałem dlatego, żeby było szybko. Np. GUI webowe jednak tworzy mi się wolniej, nie mówiąc już o GUI na desktopy. Jeszcze mógłbym zrobić samo API, ale mnie chodziło o to, by nie była to biblioteka, tylko coś, czego można od razu użyć.


PS. Użyć i zobaczyć efekty.


PS2. W zasadzie chodziło mi o to, żeby nie trzeba było innej aplikacji, zewnętrznej. Ech, nie mogę się wysłowić. :D

0

A Bash wydaje mi się (wciąż) najbardziej oczywistym wyborem, jeśli chodzi o CLI.

Nie, jeśli chcesz żeby można to było odpalić multiplatformowo bez dodatkowego zachodu, np w Powershellu/cmd ;)

1

Do sprawdzania skryptu w bashu, polecam używać shellcheck, jest dostępny w repo popularnych dystrybucji.

0

@TurkucPodjadek: dzięki, zobaczę, co to. Na razie koncentruję się na JS.

0

Kolejne, duże, wydanie mojej aplikacji: 1.2.0 (link). Dodałem testy jednostkowe; wykorzystuję przy nich frameworki Mocha oraz Jest.

Jeżeli ktoś będzie chcieć, może je sprawdzić. Byłoby mi było miło. :)


UPDATE: Z innych zmian – nareszcie włączyłem wsparcie dla usługi Travis CI. Teraz mam fajny obrazek w README z tekstem build passing. :)


PS. Powiem wam, że zmęczyło mnie to. Potrzebuję chwili odpoczynku. A potem... z powrotem do pracy!


UPDATE2: Testy jednostkowe są dla kodu JavaScript. Jeśli chodzi o kod w Bashu – nie chciałem już tracić czasu na kombinowanie. Planuję dodać coś później w tym temacie.


UPDATE3:

Miałbym jeszcze pytanie: jak często powinienem wypuszczać nowe wydania? Chodzi mi o to, że obecnie nie mam ustalonego harmonogramu; robię tak: znalazłem 1 bug (lub kilka, ale musi być naraz) -> naprawiam go -> nowe wydanie; wymyśliłem nową funkcjonalność lub chcę usunąć istniejącą -> implementuję to -> nowe wydanie.

Narzut czasowy dla mnie nie wydaje się być zbyt duży, a cały proces jest spójny = łatwo mi nad nim panować. Ale z punktu widzenia użytkownika lub innego dewelopera – na podstronie z wydaniami na GitHubie robi się tłok. I jakoś to tak nie wygląda mi profesjonalnie.

0

Kolejne, duże, wydanie mojej aplikacji: 1.3.0 (link).

Z większych zmian:

  • dodałem testy integracyjne (pisane we frameworku Jest);
  • nadałem status "deprecated" funkcjonalności testowania dostępnej przez public API.

Jeżeli ktoś będzie chcieć, może sprawdzić te testy. Byłoby mi było miło. :)

O, i przypomniało mi się – muszę jeszcze w dokumentacji dodać ten status. Ech.

1 użytkowników online, w tym zalogowanych: 0, gości: 1