(zakończony)Warsztat 15: etap 06: KONTROLA JAKOŚCI KODÓW źródłowych

Hej

Otwieram etap 06 produkcji warsztatu nr 15.

Tu każdy kto ma ochotę, może zgłaszać uwagi do jakości kodów, utworzonych przez programistów w poprzednim etapie (nr 5) produkcji tej  laborki. Namawiam każdego, żeby aktywnie to robić w tym wątku (zgłaszajcie uwagę nawet wtedy jak macie wątpliwości czy jest sensowna!)

Zanim jednak zaczniecie tu postować, odpowiednio przygotujcie się do tego, tzn przejrzyjcie arkusz przebiegu tego etapu produkcji tej laborki (są tam dalsze instrukcje oraz m.in namiary na utworzone aplikacje):
https://docs.google.com/document/d/1InIu_Zne6vdxqyj6qsXGOAUW6I6CYbF3gaIZREhk1gY/edit?usp=sharing

Arkusz produkcji całego warsztatu można znaleść równoległym poście "etap 00" otwierającym produkcję tej laborki

Życzę powodzenia wykonawcom tego etapu, czekam na pierwsze posty (uwagi jakościowe dot. kodów aplikacji).

Hej. Wydaje mi się że z metodą RecalculateScrollValue jest coś nie tak. Przy przesuwaniu suwaka w aplikacji okienkowej od "Fuel consumption: Gasoline" w dół zaczynają się błędy tzn przeskoki liczb po przecinku - czasami o jeden czasami o 2. Chyba że jest to zamierzone;)

Hej.
Ja jedyne co wyłapałem, to moim zdaniem w
MadagascarTools --> LpgCalculator
niepotrzebna zmienna costSaving. Nie lepiej przerobić CalculateLPG na zwracającą wartość double i ją wywoływać jako parametr metody ShowResult? Nie wiem tylko jak to zadziała w WinFormie, przewija mi się w tle ale docelowo jeszcze się go nie uczyłem.

Potwierdzam też przeskoki, które wyłapał Borixon, aczkolwiek wydają mi się zamierzone żeby nie ciapciać się z większymi przedziałami danych.

Natomiast zmieniłbym (gdybym umiał) żeby suwaki były zintegrowane z textboxami i zmiana położenia suwaka nie resetowała ustawień wprowadzonych z klawiatury tylko je modyfikowała. Tylko nie wiem czy nie zapędzam się tu za bardzo do przodu (w końcu nie tworzymy jeszcze aplikacji na sprzedaż)

Popieram usunięcie pola costSaving (aby oszczędność była zwracana z metody Caclculate)

To ja po uwagach Emielka i Borixona (dot. scrollbarów) proponuję kompletnie wydymić wszystkie suwaki, dzięki temu odpadnie dużo kodu (zrobi się bardzo czytelny):

• odpadnie konieczność rozwiązania problemów dziwnych przeskoków wartości na textboxach
  
• odpadnie dużo kodu dla wszystkich siedmiu eventów które przeliczają zmianę srolla na wartość textboxa (7 wywołań metody RecalculateScrollValue)
  
• odpadnie kod metody RecalculateScrollValue , bo już nie będzie potrzebna
  
• odpadnie konieczność oprogramowania synchronizacji (w drugą stronę) zmiany wartości na textboxie do poziomu przesunięcia suwaka
  
• uzyskamy maksymalnie trywialną aplikację okienkową (tylko labele, textboxy i button) idealną na warsztat z pierwszą aplikacją okienkową (zresztą celem tej aplikacji jest tylko wykazać, że jest ona przykładem kolejnego UI któy podpina bibliotekę kalkulatora lpg)
  

Moje uwagi do kodu:

• formatka kalkulatora okienkowego ma (przez pomyłkę) nazwę ConsoleCalculatorForm.cs trzeba zmienić na oczywiście WinFormsCalculatorForm albo po prostu na CalculatorForm
  
• po uruchomieniu okienkowego kalkulatora w nagłówku formatki pojawia się Form1 (co jest obciachowe ;p), trzeba zmienić ten tekst na np. na ten sam tekst co pojawia się w tytule aplikacji
  

Wszystkie aktualne uwagi wstawiłem do arkusza uwag uczestników (listy kontrolnej dla programstów wprowadzających poprawki), link do tego arkusza znajdziecie w otwierającym poście, w punkcie 3 (rezultat etapu), lub tu jest ten link: 
https://docs.google.com/spreadsheet/ccc?key=0AiEDbuEU88x4dHdsY293WVlOR05TRkhDMkktUnFyOFE&usp=sharing

Co do głównego okna formatki.
Właściwość maximizeBox i MinimizeBox ustawiłbym na false aby użytkownik
nie miał możliwości powiększania aplikacji gdyż nieefektownie wtedy ona wygląda.
Pozostaje jeszcze ręczne powiększanie formy. Metodą prób i błędów doszedłem że ustawienie właściwości AutoSizeMode na GrowAndShrink wyłącza tę możliwość.
StartPosition można by ustawić na CenterScreen aby za każdym razem aplikacja nie uruchamiała się w innym miejscu.
Można by zmienić ikonkę zamiast domyślnej np. na jakiś samochodzik. oraz pomyśleć nad kolorystyką.

Jeszcze malusieńka uwaga.
Jeśli użytkownik pozostawi choć jedno miejsce niewypełnione tj. 0 to przy polu Cost Saving mogłaby się pojawiać informacja: proszę wypełnić wszystkie pola
lub
ikonka Calculate powinna się uaktywnić po wprowadzeniu wszystkich danych.

Super
W takim razie pojawi się jeszcze jeden mało wymagający element WinForms, okienko "alert" jeśli dobrze pamiętam, które wyświetli komunikat o konieczności wprowadzenia wszystkich parametrów kalkulacji.

Pewnie zbędny bajer ale co tam

Dodajemy do naszej aplikacji 2 lub 3 malutkie buttony(a jeszcze lepiej jakby pojawiał się tu suwak) na dole formatki zmieniające kolorystykę naszej aplikacji. Domyślnie by była ona taka jak teraz szara ale jakby komuś wydawała się nudna zmienia ją w bardziej żywą np. odcienie koloru niebieskiego.

Sorki że nie wszystko w jednym poście ale jeszcze jedna rzecz mi się przypomniała.
WYJĄTKI.

Wypadało by aby kompilator w textboxach interpretował w liczbach zmiennoprzecinkowych "." jako ",". W przypadku wprowadzania liter też trzeba by wyświetlić jakiś komunikat
aby nie wywalało błędu.

Borixon, nie ma problemu można zgłaszać uwagi na raty. Ten topik ma być chaotycznym workiem na uwagi dot. kodu. Część z Twoich nowych uwag zaplanowałem do wdrożenia (te które nie kosztują wiele dodatkowego dewelopmentu), a część odrzuciłem. 
https://docs.google.com/spreadsheet/ccc?key=0AiEDbuEU88x4dHdsY293WVlOR05TRkhDMkktUnFyOFE&usp=sharing
Te odrzucone uwagi są również są sensowne. Aplikacja celowo nie będzie zabezpieczona przed błędami użytkownika (nie będzie mechanizmów analogicznych do metody GetNumber w bibliotece konsolowej), bo w tym okienkowym kalkulatorze LpG będzie chodziło tylko o pokazanie mechanizmu podpięcia biblioteki LpgCalculator do formy, a nie utworzenie maksymalnie dopieszczonej aplikacji (o tym można pomyśleć w kolejnych laborkach). Tak na razie to widzę, ale może zmienię zdanie, zobaczymy co inni napiszą.

Upewniam Was aby uwagi zgłaszać nie tylko do okienkowego klienta kalkulatora LpG, ale również do pozostałych projektów z tej samej solucji (klasy Console, klasy LpgCalculator, konsolowego klienta kalkulatora Lpg itd.) ... no chyba, że te moduły zostały oprogramowane bezbłędnie w co nie wierzę  :p

WinFormsLpgCalculator
- To co mi się rzuciła to brak blokady wpisania liczby ujemnej z klawiatury i walidacji tego.
- Po naciśnięciu przycisku licz możliwość manipulowania wynikiem (przesuwając suwaki) w czasie rzeczywistym zmieniajać parametry.
- zablokowania maksymalizowania i rozciągania formy

Klasa LpgCalculator

metoda  FuelCostSavingPer1Km()

Zastanawiam sie nad nazwą  zmiennych : petrolConsumptionPer100Km  oraz lpgConsumptionPer100Km
                                   "zużycie(konsumpcja) benzyny na 100 km  "
petrolConsumptionPer100Km= gasolineConsumption100kmLitres *  litreOfGasolinePrice
można by dać coś w stylu:  koszt paliwa na 100 km    lub    cena przejazdu za 100 km

metoda  TotalMileageKm()

Z nawiasem może lepiej by to wyglądało????  Niech specjaliści się wypowiedzą
return monthlyMileageKm * 12 * carUsageYearsPlan;
return (monthlyMileageKm * 12) * carUsageYearsPlan;

co do aplikacji konsolowej.


Nagłówek CALCULATOR FOR LPG INSTALLATION PROFITALBILITY mógłby się bardziej wyróżniać od reszty aplikacji - proponuje zmienić kolor

komunikaty przy wprowadzeniu błędnych danych typu: "A-non numeric value" mogły by być wyświetlane np. na czerwono.

Zakończeniem aplikacji może być komunikat: Czy chcesz kontynuować czy wyjść z aplikacji y/n.

Aplikacja do Video
1. W konsoli można by pozwoliś na wpisywanie liczb z "." jak i "," i w kodzie można by to obsłużyć.
2. Błędy w konsoli mogłyby być kolorem czerwonym, żeby się wyróżniały.

Alfabe Morsa
1. Formatka ma nazwę "Form1". Można by ustawić np  "Translate of Morse alphabet".
2. Usunąłbym nazwę Englisz i zamienił na "Text" gdyż jest to tłumaczenie znaków alfanumerycznych (czyli [a-zA-z0-9]) na alfabet morsa i odwrotnie.
3. Okno można by zablokować gdyż można je dowolnie rozciągać

W projekcie MadagaskarUserInterface w metodzie Write Zmianę koloru tekstu na szary można wcisnąć do ifa. Nie widzę sensu aby zmieniał się za każdym wywołaniem metody.

BYć może będzie to mało istotne i z punktu widzenia programisty poprawka ta będzie sprawiać, że kod będzie mniej wydajny, ale powiem. Warto być może byłoby gdyby w aplikacji konsolowej w momencie wpisania błędnej wartości, aby pojawił się komunikat o konieczności ponowienia wpisania wymaganej tresci i obok informacji o errorze powinien znaleźć się tekst typu: "wpisz pożądaną wartość raz jeszcze" , albo cos w tym stylu.

W projekcie MadagaskarUserInterface można by dodać metodę np. getColor - ustawiającą jedynie kolor czcionki a w pozostałych metodach wykasować wszystkie parametry związane z ustawianiem koloru.
Zmniejszyła by nam się wtedy znacznie liczba parametrów.

Borixon911 napisał:W projekcie MadagaskarUserInterface można by dodać metodę np. getColor - ustawiającą jedynie kolor czcionki a w pozostałych metodach wykasować wszystkie parametry związane z ustawianiem koloru.
Zmniejszyła by nam się wtedy znacznie liczba parametrów.

Borixon jak zrobię metodę getColor to tylko przykryję jednolinijkową konstrukcję z Console.ForegroundColor = ... . Więc tu nie widzę zysku. Jeżeli z kolei usunę parametry określające kolor z metod WriteLine, to rozrośnie mi się kod przy naprzemiennym prezentowaniu tekstów w kolorze białym i zielonym w konsolowym kalkulatorze lpg (bo każde WriteLine będę musiał poprzedzać Console.ForegroundColor = ... ustalające naprzemiennie kolor biały i zielony
... chyba, że po prostu nie zrozumiałem Twojej sugestii. Można natomiast pomyśleć o tym, aby nie było powrotu do koloru szarego przy każdym uruchomieniu metody Write/WriteLine, oraz usunięciu domyślnej wartości koloru szarego dla parametru koloru ... w rezultacie kolor w parametrze przy wywołaniu metody WriteLine będzie się podawało tylko wtedy gdy zaistnieje konieczność zmiany koloru na inny.

Pewnie Koszmarek masz racje.

Swój poziom na razie uważam za początkujący także czasami mogę coś palnąć mało sensownego.

O co mi chodziło:
Myślałem żeby do metod Write, WriteLine, GetNumber również dodać tę metodę.

Zamiast System.Console.ForegroundColor = textColor;
napisać getcolor(color = ConsoleColor.White)
"color - zmienna widoczna w całek klasie"

a parametry typu ConsoleColor textColor = ConsoleColor.Gray wyciąć

jednak w takim przypadku nie moglibyśmy zmieniać koloru za pomocą argumentów - było by to raczej zrobione na sztywno.

Projekt MadagascarUserInterface
Co do samej klasy Console… szalu bym dostal, gdybym do każdego wywolania metody ‘Write’ miał podawac kolor i opóźnienie tekstu (zwłaszcza gdy zawsze chce mieć to samo). Tutaj bym utworzyl kilka publicznych pol (arghhh) przechowujące informacje o kolorze i delay. Oczywiście te pola powinny być zainicjalizowane domyslna wartoscia, tak jak aktualnie poprzez parametry imienne. Poza tym uważam, ze metoda taka, jak SetWindowSize nie powinna mieć parametrow opcjonalnych. Metoda jest od ustawienia wielkości okna, wiec użytkownik powinien je zawsze podac.

Projekt MadagascarTools:
Dlaczego klasa Console jest statyczna to rozumiem, ale nie potrafie zrozumieć dlaczego tez taka jest LpgCalculator. To nie jest na tyle uniwersalna klasa, aby musiala być takiego typu. Rozumiem, ze jest to wygodne, ale nie funkcjonalne. Nie będę się w tym temacie zaglebial, po prostu jest to zle i nalezy to zmienic.

Projekt Console.LpgCalculator:
Uwazam, ze jest on do przebudowania z powodu niepotrzebnego uzycia klasy statycznej.

Projekt Winforms.LpgCalculator:
Co do uwagi koszmarka – scrolle bym zostawil, ponieważ fajnie wygladaja :)Można ladnie metody  od scroll barow umiescic w jednej metodzie, ale nie wiem czy rozwiązanie tego problemu, jak na ten etap nie bedzie zbyt zaawansowane. Mam tu na myśli bindowanie danych lub wykorzystanie property 'Tag' przechowujaca nazwe textboxa ktoremu odpowiada scrollbar i po tym wyszukiwac kontrolke nadajac jej odpowiednia wartosc. Ta druga opcja jest dosc prosta i czesto przydatna.

Do tego bym poszalał i w momencie zmiany jakiekolwiek wartości z automatu przeliczalbym oszczednosc paliwa (aby nie trzebalo za każdym razem klikac button Calculate) – taka opcje uruchomiłbym po pierwszym kliknieciu button, aby na starcie nie pojawialy się dziwne wartości.

Borixon911 napisał:
Jeszcze malusieńka uwaga.
Jeśli użytkownik pozostawi choć jedno miejsce niewypełnione tj. 0   to przy polu Cost Saving mogłaby się pojawiać informacja: proszę wypełnić wszystkie pola
lub
ikonka Calculate powinna się uaktywnić po wprowadzeniu wszystkich danych.

Ewentualnie mozna wykorzystac kontrolke errorprovider. Mi osobscie bardziej podoba sie takie zglaszanie bledow formularza, niz masa wyskakujacych okienek.


Co do aplikacji na egzamin:
Dlaczego 'English Text'? Czy to z angielskiego czy z polskiego to algorytm tlumaczenia zawsze jest taki sam, wiec czemu mamy dyskryminowac inne jezyki?

Fores napisał:Projekt MadagascarUserInterface
Co do samej klasy Console… szalu bym dostal, gdybym do każdego wywolania metody ‘Write’ miał podawac kolor i opóźnienie tekstu (zwłaszcza gdy zawsze chce mieć to samo). Tutaj bym utworzyl kilka publicznych pol (arghhh) przechowujące informacje o kolorze i delay. Oczywiście te pola powinny być zainicjalizowane domyslna wartoscia, tak jak aktualnie poprzez parametry imienne.

Wygląda na to, że masz rację

Fores napisał:Poza tym uważam, ze metoda taka, jak SetWindowSize nie powinna mieć parametrow opcjonalnych. Metoda jest od ustawienia wielkości okna, wiec użytkownik powinien je zawsze podac.

Możesz mieć rację.

Fores napisał:Projekt MadagascarTools: Dlaczego klasa Console jest statyczna to rozumiem, ale nie potrafie zrozumieć dlaczego tez taka jest LpgCalculator. To nie jest na tyle uniwersalna klasa, aby musiala być takiego typu. Rozumiem, ze jest to wygodne, ale nie funkcjonalne. Nie będę się w tym temacie zaglebial, po prostu jest to zle i nalezy to zmienic.

W takim razie musisz się w to zagłębić i wyjaśnić szerzej, dlaczego powinno się zrobić z tego klasę standardową. My (ze Zdankiem) zrobiliśmy z tego klasę statyczną z dwóch powodów:

1. Nie chcemy w tej laborce jeszcze wyjaśniać czym jest obiekt względem klasy (używać konstrukcji new ... bo zwyczajnie brak czasu na wideo żeby zmieścić do w w15. Dopiero od w16 chcę przejść do obiektówki (tworzenia instancji klas)
   
2. Wg mnie jest uzasadnione użycie klasy statycznej, bo opieramy się tu na założeniu, że jest bardzo mało prawdopodobne, aby programista wykorzystał bibliotekę do zarządzania więcej niż jednym kalkulatorem LpG w swojej aplikacji (utworzenia wielu obiektów klasy kalkulator i zarządzania wartościami jego pól-->właściwości dla np. kilku obiektów tego kalkulatora). Raczej jest pewne że będzie takiemu programiście (nieważne czy klienta oprogramuje na konsoli, WinForms, asp, unity ... whatever) raczej będzie się odwoływał do tylko jednego kalkulatora ... więc nie ma potrzeby tworzyć jego instancji, od razu odwoła się do klasy statycznej kalkulatora. Co o tym myślisz, ma wg Ciebie sens taka argumentacja? Pisz jak nie (ale argumentuj) bo może rzeczywiście właśnie forsuję rozwiązanie (klasa statyczna) które spowoduje, że internauci oglądając w15 na youtoube nas wygwizdają :p
   

Fores napisał:Projekt Winforms.LpgCalculator:
Co do uwagi koszmarka – scrolle bym zostawil, ponieważ fajnie wygladaja :)Można ladnie metody  od scroll barow umiescic w jednej metodzie, ale nie wiem czy rozwiązanie tego problemu, jak na ten etap nie bedzie zbyt zaawansowane. Mam tu na myśli bindowanie danych lub wykorzystanie property 'Tag' przechowujaca nazwe textboxa ktoremu odpowiada scrollbar i po tym wyszukiwac kontrolke nadajac jej odpowiednia wartosc. Ta druga opcja jest dosc prosta i czesto przydatna.

Zrezygnuję ze scroli (ewentualnie pokażę je na chwilę na wideo (i wspomnę o możliwości rekalkulacji dla każdego przesunięcia scrolla), ale na końcu wyjaśnię, że aplikacja okienkowa w W15 ja celowo maksymalnie trywialna (jako wstęp do Winforms) czyli tylko labele , tekstboxy i button (również celowo bez walidacji i ochrony przed błędami użytkownika) ... aby na razie (tylko w tym warsztacie) pokazać mechanizm podpięcia tej samej biblioteki (MadagascarTools --> LpgCalculator) przez co najmniej 2 UI.

Co do aplikacji na egzamin:
Dlaczego 'English Text'? Czy to z angielskiego czy z polskiego to algorytm tlumaczenia zawsze jest taki sam, wiec czemu mamy dyskryminowac inne jezyki?

Słusznie, ktoś już zgłosił taką uwagę (choć nie jestem pewien) jutro zajrzę to arkusza i sprawdzę, jak będę dopisywał Twoje uwagi

Pozostałe uwagi Twoje słuszne Fores, ale tu znowu ta sama sytuacja: ominę te ficzery celowo, aby zrobić na razie aplikację okienkową prostą jak budowa cepa (+ zaznaczę odbiorcy na wideo , że z pełną premedytacją na razie utworzyłem aplikację która wielu rzeczy (od strony GUI WinFormsów) nie pilnuje, co będzie uzupełniane w kolejnych laborkach, wraz z przekazywaniem wiedzy dot. kolejnych zgadnień) .

Co do winformsow - fakt, lepiej po malu wprowadzac nowe rzeczy. Teraz sobie przypomnialem, ze sam w zalozeniach proponowalem okrojony material na ich temat

@edit
W pracy dość często korzystam z zewnętrznych dllek, które również często kupujemy i jeszcze nigdy nie zdarzylo mi się, aby glowna jej funkcjonalność była statyczna. Rozumiem, ze w tym przypadku nie potrzebujemy więcej instancji tej klasy, ale to nie jest żaden argument sklaniajacy do używania klasy statycznej. Rownie dobrze możemy uzyc singletona, zachowując wszystkie przywileje, jakie dostarczają nam klasy instancyjne. I tak wiem, ze jest troche za wczesnie na pewne wzorce projektowe, ale w tym przypadku nie ma potrzeby, aby blokowac liczbe instancji klasy mozliwych do utworzenia, a wiec i nie musimy go uzywac.
Nie lubie pisac takich tekstow, z reguly nie potrafie przekazac to co mam na mysli, wiec w razie czego moge porozmawiac na ten temat na skypie. Bedzie mi latwiej

Proponuje dokładnie przesledzic ten watek - http://stackoverflow.com/questions/241339/when-to-use-static-classes-in-c-sharp

Fores, masz rację.

Mam teraz problem, bo nie chcę wprowadzać w studenta w jednym warsztacie w 2 tematy na raz:

1. w biblioteki: główny temat warsztatu
   
2. klasy standardowe: co najmniej dodatkowe 10-20 min na wideo zajmie mi wyjaśnianie studentowi znaczenia/sensu słowa new, np. poprzez tworzenie więcej niż jednego obiektu tej samej klasy, dalszego operowania na tych obiektach i ich różnych stanach  itd.
   

Pomyślałem sobie więc, że wybrnę w ten sposób, że wdrożę Twoją uwagę (klasę LpgCalculator uczynię standardową), ale na samym końcu wideo warsztatu. Bibliotekę oraz konsolowy i okienkowy klient kalkulatora LpG oprę najpierw (przez większość ostatniego rozdziału wideo) na statycznej klasie LpGCalculator, po czym na samym końcu (dając w komentarzu argumentację zbliżoną do tej w Twoim ostatnim poście) zmienię klasę LpGCalculator na standardową (co pociągnie za sobą minimalne poprawki w obu klientach kalkulatora: okienkowym i konsolowym). Tą końcówkę na wideo (m.in użycie "new" do założenia obiektu kalkulatora) wykonam bez wyjaśniania (w komentarzu pod filmem) znaczenia tej operacji tworzenia obiektu, ale pokażę to jako tylko krótką zapowiedź tego, co będzie w kolejnym warsztacie (z komentarzem dla studenta, że nie musi rozumieć tego co widzi, bo zostanie mu to szczegółowo wyjaśnione w kolejnej laborce ... choć to już uwaga dla nas: niekoniecznie będzie to wyjaśniane na kalkulatorze Lpg, być może już na innej aplikacji).

Fores, takie rozwiązanie Ci pasuje ?

Szczerze powiedziawszy to nie jestem w stanie powiedziec czy taki sposob bedzie dobry czy tez nie. Moim zdaniem nie ma zlotego srodka, gdyz temat jest dosc obszerny i ciezko jest napisac 'dobry' kod bedac ograniczony materialem, jaki mozna przekazac. Mysle, ze dopoki bedziemy w pelni przekazywac wiedze na jeden dany temat (w tym przypakdu tworzenie wlasnych bibliotek) to bedzie dobrze. W kazdym warsztacie beda pojawiac sie nowe elementy jezyka, to jest nie uniknione. Wazne tylko, aby nie egzaminowac z nich zdajacego, a traktowac taki dodatkowy content, jako uzupelnienie warsztatu. Tworzenie takich warsztatow nie bedzie latwe, lecz z pewnoscia beda one bardziej atrakcyjne w porownaniu z innymi polskimi kursami C#, jakie mozna znalezc w googlach.