LearnMorse jest aplikacją przeznaczoną dla początkujących radiotelegrafistów chcących się nauczyć alfabetu Morse'a. Zacząłem pracę nad nią ze względu na chęć sprawdzenia się w Javie i stworzenia czegoś pożytecznego dla siebie. Chciałbym się nauczyć alfabetu Morse'a i mam nadzieję że ta aplikacja mi w tym pomoże. Aplikacja ta nie powala designem. Jej oficjalny opis na GitHubie brzmi "An open-source application that makes learning Morse better.". Zdążyłem już zrobić parę commitów do repozytorium gita. Chciałbym ją udostępnić w postaci deba dla systemu Debian oraz jak się zapoznam z narzędziami pkgin i pkgsrc dokładniej to dla NetBSD i jak oczywiście uda mi się dogadać z odpowiednimi ekipami. Testy jednostkowe przygotuję po zakończeniu pisania samej aplikacji przy użyciu silnika junit. Korzystam oczywiście z Mavena przy tworzeniu tej aplikacji. Daję wam link do repozytorium mojej aplikacji, którą zacząłem pisać w Javie (https://github.com/hubotx/LearnMorse) i proszę abyście powiedzieli co robię dobrze a co źle. Szczególnie mile będzie widziana konstruktywna krytyka użytkownika @Shalom. Aplikacja została oczywiście udostępniona na licencji GNU/GPL. Nie jest jeszcze gotowa. Jest w trakcie dodawania różnych funkcjonalności.
LearnMorse jest aplikacją przeznaczoną dla początkujących radiotelegrafistów chcących się nauczyć alfabetu Morse'a. Zacząłem pracę nad nią ze względu na chęć sprawdzenia się w Javie i stworzenia czegoś pożytecznego dla siebie. Chciałbym się nauczyć alfabetu Morse'a i mam nadzieję że ta aplikacja mi w tym pomoże. Aplikacja ta nie powala designem. Jej oficjalny opis na GitHubie brzmi "An open-source application that makes learning Morse better.". Zdążyłem już zrobić parę commitów do repozytorium gita. Chciałbym ją udostępnić w postaci deba dla systemu Debian oraz jak się zapoznam z narzędziami pkgin i pkgsrc dokładniej to dla NetBSD i jak oczywiście uda mi się dogadać z odpowiednimi ekipami. Testy jednostkowe przygotuję po zakończeniu pisania samej aplikacji przy użyciu silnika junit. Korzystam oczywiście z Mavena przy tworzeniu tej aplikacji. Daję wam link do repozytorium mojej aplikacji, którą zacząłem pisać w Javie (https://github.com/hubotx/LearnMorse) i proszę abyście powiedzieli co robię dobrze a co źle. Szczególnie mile będzie widziana konstruktywna krytyka użytkownika @Shalom. Aplikacja została oczywiście udostępniona na licencji GNU/GPL. Nie jest jeszcze gotowa. Jest w trakcie dodawania różnych funkcjonalności.
-
Nie masz testów żadnego rodzaju. Co gorsza masz placeholder testów, i to w JUnicie 3 (aktualna wersja to 5, 4.x które od lat było de facto standardem też jest sensowne).
-
Historia to ledwie cztery commity, z czego dwa to "Initial commit"
-
Jeśli chcesz tę aplikację dystrybuować można to zapakować razem z JRE
-
W klasie MorseString masz jebutnego switcha z translacją - wrzuć to lepiej do jakiejś mapy żeby nie mieszać samego procesu translacji (iteracji itp) z odpowiednikami tokenów.
-
W tym samym miejscu ignorujesz znaki które nie należą do alfabetu Morse'a. Czemu?
-
Do tworzenia stringu używasz konkatenacji - to ci tworzy nowy string przy każdym +=. Używaj StringBuildera. Komentarze przy tych caseach też są zbędne
-
Ignorujesz warningi ze strony IDE - mi IntelliJ aż krzyczy o tej konkatenacji :(
-
Ta sama klasa ma wiele funkcjonalności - zakodowanie, zdekodowanie, odtwarzanie dźwięku - to ostatnie minimum musi mieć swoje własne miejsce - zresztą czemu tam jest zhardkodowana sekwencja dźwięków zamiast parsowania stringa - przecież zmienisz stringa (bo się np. pomyliłeś) to i musisz zmienić te wywołania, ale tego pewnie zapomnisz. Do tego z tym dźwiękiem nie masz tego jak przetestować (nie zapniesz się chyba na mikrofonie i nie będziesz słuchać :D?) - a nawet jakbyś miał to wątek ci śpi na tym, więc twoje testy będą szły dłużej niż regresja webappek w Selenium.
dah();
dit();
dah();
dit();
dah();
dah();
encoded += "_._.__";
- Bezsensowne javadocsy - z gatunku "no shit Sherlock":
/**
* Get MorseSettings object.
* @return MorseSettings object
*/
public static MorseSettings getSettings() {
return settings;
}
-
Statyczne metody - statyczne metody na JVM są problematyczne bo generują bytecode z invokestatic, trudno się to testuje (bo chyba nie będziesz w testach sobie pykał tych dźwięków tylko sprawdzał czy dobre znaczki przychodzą do cosia z dźwiękiem). Lepiej poświęcić ten kawałeczek sterty i dostać coś co można później zmockować albo podmienić na stuba.
-
Cała klasa ma globalny mutowalny stan. Nie rób nam tego :(
-
MorseSettinsg ma od ciula redundantnych konstruktorów, wywołuj konstruktor główny z pomocniczych
-
Powyższe ma też dużo magicznych zmiennych z gatunku "1200"
-
Na końcu jest wykomentowany kod - od tego masz gita żeby można wyrzucić kod a później nie płakać tylko go znaleźć.
-
Musiałem myśleć co to jest Pool w settingsach - nadmierne myślenie przy czytaniu kodui może doprowadzić do wypalenia zawodowego.
-
W char pool może ci się wydawać że
public static final char[]jest niemutowalne, ale wcale takie nie jest - elementy tej tablicy można zmieniać. Bo java. -
Powyższe to też globalne zależności od stanu w jednym miejscu - nie lepiej to wczytać z jakiegoś pliku żeby można było wprowadzać nowe języki bez rekompilacji (podobnie tłumaczenia z morse'a do ludzi'a i z ludzi'a do morse'a)
-
W PropertiesController masz linie które przekraczają 140 znaków i więcej bezużytecznych komentarzy, nieużywane importy
-
W MainController (dziwna nazwa) magiczne stałe, puste catch-alle - weź pan przynajmniej zestaw sobie jakieś logowanie (slf4j + logback) i pologuj co tam się wyrąbało, przyszły Ty będzie wdzięczny. Zwłaszcza jak poleci coś ciekawego. Ponadto deklaracje throws nie zgadzają się ze stanem faktycznym.
-
Nie znam JavaFX na tyle żeby powiedzieć coś konkretnego, ale prawie na pewno nie trzeba tworzyć gołych wątków tylko można je zaschedulować.
-
Zmień to cudo na bezwzględną ścieżkę, wywali się przy pierwszym refaktorze.
Parent root = FXMLLoader.load(getClass().getResource("../../../../../view/Properties.fxml"));
-
Zmienne deklarujesz na końcu, nie wiem czemu ale ludzi kręcą różne rzeczy - w stanowych klasach wolałbym to zobaczyć jednak gdzieś u góry
-
Te exceptions które wylecą za maina można by jakoś opakować w obsługę błędów, co by pani Grażynka która jest telegrafistką ogarnęła.
To chyba tyle jak na pobieżny kod. Nie mam tutaj zamiaru cię zniechęcać, bo fajnie że robisz projekty, ale jest jeszcze sporo rzeczy do ogarnięcia. No i taki review dostajesz za darmo, zwykle za bluzganie na kod innych mi płacą. Teraz się zabiorę do UXa.
Projekt całkiem spoko.
Dobrze, że wróciłeś na forum, brakowało nam ciebie :)
Do tworzenia stringu używasz konkatenacji - to ci tworzy nowy string przy każdym +=. Używaj StringBuildera. Komentarze przy tych caseach też są zbędne
To oczywiście bezedura - tam jest **StringBuilder **- tylko ładnie schowany. OOps - nie tak szybko. Mój błąd.
POPRAWKA!
A jednak nie taka bzdura! Jest StringBuilder ale w tym przypadku jako że decoded jest statyczne to jednak jest to niepotrzebnie przez Stringa przeciągane.
Czyli jednak jak by był decoded StringBuilderem to by było lepiej!
Sorry! ( Z rozpędu napisałem - bo ta wiara w += się pokutuje juz od 10 lat - a przeważnie jest to bzdura (mit) ( jeśli String jest obrabiany w pętli to naprawdę kompilator sobie z tym radzi -
tutaj koniecznośc każdorazowo przetrzymania wyniki w Stringu statycznym polu powstrzymała optymalizację -to widać w tym biednym toString wywoływanym na końcu).
2053: invokespecial #72 // Method java/lang/StringBuilder."<init>":()V
2056: getstatic #3 // Field decoded:Ljava/lang/String;
2059: invokevirtual #73 // Method java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
2062: ldc #82 // String h
2064: invokevirtual #73 // Method java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
2067: invokevirtual #75 // Method java/lang/StringBuilder.toString:()Ljava/lang/String;
2070: putstatic #3 // Field decoded:Ljava/lang/String;
to przykładowo kawalek bajtcodu odpowiadający temu:
case "....": // h
decoded += "h";
break;
Ok, odpalam aplikację.
-
Pierwsze co widzę to "Hello World" w belce tytułowej. Nie za dobrze.
-
To jest minimalny rozmiar okna które udało mi się osiągnąć. Daj jakieś minimum:
Ja, p. Grażynka, Klikam sobie po menu.
-
Help -> About nie robi nic
-
Edit -> Delete jest aktywne nawet jak nic nie wpisałam
-
Klikam Procedures -> Train morse i coś mi pika. Nie ogarniam. Nie wiem jak to zastopować/wyłączyć.
-
W akcie desperacji Grażynka klika to samo kilka razy. Odpala się kilka wątków i pikają. W tym momencie p. Grażynka biegnie do helpdesku bo ma wirusa.
-
Informatyk przychodzi do p. Grażynki i wyłącza okienko. Jako że wątki są odpalane jako demony zamknięcie okienka nie powoduje zaprzestania pikania. Ubijamy proces.
-
File -> Close nie bangla
-
File -> Properties - p. grażynka jako że jest zdolną anglistką w polu "Pause before keying" wpisuje "seven". W konsoli leci NumberFormatException. O dziwo jak p. Grażynka wpisze -10 to działa. W sumie wszędzie działa dowolna liczba.
-
Okienko propertiesów co prawda zmienia rozmiar, ale kontroli w nim nie - dzięki czemu labelki ukrywają się pod textboxami.
-
Grażynka zażyła aspirynę i będzie tłumaczyć. Wpisuje "Szanowna Pani,", klika transmit. Niby działa:
ale Grażynka zastanawia się ile tam jest kurczę podkreślników, bo font skleja przyległe i generalnie nie jest monospace.
-
Nadal nie wiem co robi Edit -> Delete
-
Lecimy w drugą stronę: "Szanowna Pani __.._"., receive. Grażynka zastanawia się nad znaczeniem wszechświata i tego czemu przed ź są spacje. I czemu zignorowano złe znaki (jak ktoś wpisze znaczek morse'a który nie istnieje to twój program go olewa).
-
Po tym wszystkim Grażynka klika train morse i musi przesiąść się z Xów na wirtualny terminal żeby wysłać serdeczny SIGKILL do procesu zjadającego 3 rdzenie jej procka. Grażynka ma dość na dzisiaj i idzie ponarzekać na 4programmers.
jarekr000000 napisał(a):
Do tworzenia stringu używasz konkatenacji - to ci tworzy nowy string przy każdym +=. Używaj StringBuildera. Komentarze przy tych caseach też są zbędne
To oczywiście bezedura - tam jest **StringBuilder **- tylko ładnie schowany. - Mój błąd.
POPRAWKA!
A jednak nie taka bzdura! Jest StringBuilder ale w tym przypadku jako że decoded jest statyczne to jednak jest to niepotrzebnie przez Stringa przeciągane.
Czyli jednak jak by był decoded StringBuilderem to by było lepiej!
Sorry! ( Z rozpędu napisałem)2053: invokespecial #72 // Method java/lang/StringBuilder."<init>":()V 2056: getstatic #3 // Field decoded:Ljava/lang/String; 2059: invokevirtual #73 // Method java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder; 2062: ldc #82 // String h 2064: invokevirtual #73 // Method java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder; 2067: invokevirtual #75 // Method java/lang/StringBuilder.toString:()Ljava/lang/String; 2070: putstatic #3 // Field decoded:Ljava/lang/String;to przykładowo kawalek bajtcodu odpowiadający temu:
case "....": // h decoded += "h"; break;
Przyjrzyj się temu bytecodowi. Takie coś ci się generuje przy każdym case, więc jest jeszcze gorzej. Każde dodanie to:
- Tworzenie tego StringBuildera
- Dodawanie
- Budowanie Stringa
Tak więc takiego StringBuildera można sobie o kant JVMki rozbić, bo to się robi dla każdego tokenu w pętli.