(zakończony)Warsztat 17: etap 06: KONTROLA JAKOŚCI kodów aplikacji treningowej oraz egzaminacyjnej

• niefortunne praktyki w kodzie w tej aplikacji
  
• propozycje na bardziej intuicyjne rozwiązania w UI tej aplikacji
  
• itd.
  

1. "1.Odprawa pasażerów na lotnisku" - zgłaszajcie tu proszę uwagi jakościowe dot. kodu oraz UI
   
2. "2.Inteligentny dom" - zgłaszajcie tu proszę uwagi jakościowe dot. tylko UI
   

1. Nie wiem jakie założenia są do obliczania wieku średniego pasażerów w samolocie, ale jeżeli ma liczyć wiek średni tylko osób odprawionych to na liście samolotów jest źle liczony. Domyślnie dzieli wiek przez 50 pasażerów, a nie przez ilość odprawionych osób.
   
2. Z tego co wiem nie można było używać konstruktorów, a tutaj zostały użyte
   
3. W niektórych przypadkach użyłbym pętli foreach zamiast pętli for np:
   
   Metoda Fight
   Zamiast:
   

   Kod:

         

            for (int i = 0; i < passengers.Length; i++)
            {
            passengers[i].Flight();
            }


   
   Użyłbym:
   

   Kod:

   
           foreach (Passenger passenger in passengers)
            {
            passenger.Flight();
            }


   To samo się tyczy innych przypadków odczytania elementów z tablicy obiektów
   
4. Jest kilka linijek kodu, który nie jest używany a jest zakomentowany to należałoby go usunąć
   

Stravi napisał:Używanie this.zmienna nie jest stosowane w przy każdym użyciu zmiennych klasy. Np. dismissedPassengers, passengers.
Zmienne publiczne powinny być pisane z dużej litery.

Tak jak kolega powyżej wspomniał bardzo często można skorzystać z foreach dzięki czemu kod jest bardziej przejrzysty i łatwiejszy w analizie.

Kod:


            Random r = new Random((int)DateTime.Now.Ticks & 0x0000FFFF);
            this.age = r.Next(6, 80);
            this.passengerWeight = Math.Round(r.NextDouble() * (180 - 30) + 30, 2);
            this.luggageWeight = Math.Round(r.NextDouble() * 30, 2);


Kod:


public void Flight()
        {
            if ((float)dismissedPassengers / (float)passengers.Length >= 0.75)


Kod:

public float CalculateAverageAge()
...
return sum / passengers;


• Używanie this. nie jest zbędne przy większych projektach może odbić się to sporą czkawką, jeżeli ktoś stworzy zmienna o takiej samej nazwie w swojej klasie. Wystarczy poszperać w sieci o powodach dlaczego warto używać this. Decyzja czy powinno być zastosowane należy do wszystkich w projekcie, ale już na pewno trzeba się stosować do tego konsekwentnie, a nie raz z this. a raz bez.
  
• W klasie Plane nadal jest magiczna liczba 0.75
  
• Jeżeli nie stosujemy konstruktorów to ten jest do wyrzucenia public Plane(int id, int passengers)
  
• Klasa Passenger składa się z wielu magicznych liczb
  
• Thread.Sleep(20); - Zło które nie ma komentarza dlaczego taki sleep jest wykonany
  
• foreach(Passenger passenger in plane.Passengers)
  {
  if (passenger.Age > 0) passengers += 1;
  }
  Proszę o dwie dodatkowe klamry. Dwie dodatkowe linie nikogo nie zbawią, a czytającym kod ułatwi analizę.
  

• nazewnictwo modułów aplikacji: ponieważ (jak wszyscy już wiemy) wszystkie elementy aplikacji powinny nazywać się zgodnie z ich przeznaczeniem to uważam, że nazwa rozwiązania (Solution) nie powinna nazywać się "Warsztat 17" a powinna się nazywać np. AirportControl
  
• analogicznie nazwa projektu nie powinna nazywać się "Warsztat 17" a powinna się nazywać np. WinFormsAirportControl (aby informowała czym jest aplikacja oraz że jest aplikację Windows Forms)
  
• na etapie tworzenia opracowania (pdf) warsztatu zacznę od omawiania tworzenia klasy Passenger oraz jej obiektów na konsoli (bo konsola idealnie nadaje się do prezentowania uwydatniania podstawowych cech OOP). Potem chcę się przenieść tą samą (w pełni oprogramowaną) klasą Passenger (może nawet też klasą Plane) do aplikacji WinForms. Będzie bez sensu robić copy and paste kodu tych klas z aplikacji konsolowej do formatkowej (przerabialiśmy to już w warsztacie 15 i 16). Dlatego uważam, że klasy obsługujące (nazwijmy to górnolotnie) model obsługi lotniska nie powinny być wstawione bezpośrednio (osobno) do aplikacji Winforms (aplikacja WinForms na ile się da ma zajmować się tylko prezentacją tego modelu). Na razie bez zaawansowanych technik / wzorców projektowych (wspieranych użyciem np. interfejsów czy klas abstrakcyjnych) musimy (na ile potrafimy na tym etapie szkolenia) oddzielać model od prezentacji., czyli uważam , że klasy Passenger, Plane oraz Airport powinny być wydzielone do dedykowanej biblioteki (o nazwie np. "LibAirportConstrol") po to aby można było się po prostu podpinać do tej biblioteki (przy generowaniu obiektów tych klas) zarówno z aplikacji konsolowej, okienkowej, webowej, unity 3d whatever. ... będziemy tym samy kontynuować podejście z warsztatów 15 oraz 16
  
• klasa Passenger: wiem że sugerowałem w specyfikacji założyć zmienną PlaneId, ale teraz z niej jednak bym zrezygnował bo w aktualnej (okrojonej) aplikacji nie jest nigdzie używana (o ile się nie mylę)
  
• wszystkie klasy: zrezygnować z konstruktorów ! - temat poza dyskusją (ograniczenie wynikające z planu szkolenia) - ja wiem polkom że Ci to Ci trochę teraz zbrzydzi :p kod, ale nie mam wyboru. Niemal każde opracowanie wprowadzające w OOP nie omawia od razu konstruktorów (bo wiąże się z z tym np. omawianie np. przeciążania konstruktorów i materiał takiej laborki się mocno rozrasta). Jak nie wiesz jak to ugryźć (uwstecznienie technologiczne Twojej aplikacji) do pogadaj z Cygnusem jak to zrobił u siebie. Ja bym zrobił (na poziomie być może na razie aplikacji WinForms) metodę (poza klasami) która najpierw założy obiekt klasy Airport. A potem (linijkę niżej) w zagnieżdżonej pętli pozakłada 5 obiektów klasy Plane a dla każdego samolotu (w pętli wewnętrznej) pozakłada po 50 obiektów klasy Passenger
  
• Passenger.CheckIn() - po co ten Thread.Sleep(20); ??? Zrezygnowałbym z tego .
  
• klasa Passenger: mnie jestem pewien tej uwagi ! - zastanawiam się czy metoda Flight akurat w tej klasie ma odzwierciedlenie biznesowe (zgodne z realem). Staramy się, żeby OOP odzwierciedlało (na ile to możliwe) rzeczywistość. Stwierdzenie, że pasażer odlatuje (jakby samodzielnie) trochę mi nie pasuje do realu. Ja bym jednak metodę Flight() pozostawił tylko na poziomie klasy Plane i zrezygnowałbym z metody Passenger.Flight() bo  właśnie z poziomu samolotu mają być wg mnie zmieniane (na razie) zmienne (a w kolejnej laborce właściwości) klasy Passenger: zerowana ma być waga każdego pasażeta, oraz dismissed = true itd.) ...ale może tu nie mam racji, przemyśl to !
  
• klasa Passenger: brakuje metody Passenger.CalculateWeight . W przeciwieństwie do metody Passenger.Flight() to właśnie metoda o nazwie np. Passenger.CalculateSumWeight wg mnie ma sens ... bo jest wg mnie zgodne z logiką biznesową wyliczenie całego obciążenia w kilogramach jakie generuje pojedynczy pasażer
  
• wszystkie klasy: notacja: przypominam, że zmienne publiczne wg mnie powinny nazywać się z małej a nie z dużej litery.  (ustalenie moje, Foresa oraz bodajże Cygnusa vs ustalenie Straviego) 3:1 :p
  
• nadal są "magic numbers" o których wspomina Fores
  
• klasa Plane.CalculateWeight() - tu bym w pętli foreach uruchomił po prostu metodę Passenger.CalculateSumWeight, ktorej na razie nie ma a której wspominałem powyżej
  
• Plane.CalculateAverageAge niekonsekwencja w typach danych. Ta metoda na poziomie klasy Airport zwraca float a ta sama metoda w Plane zwraca double.
  
• Plane.DismissedPassengers - wystarczy wg mnie tu w pętli (dla każdego samolotu) zsumować wartości zmiennych Plane.dismissedPassengers
  
• klasa Airport - dorobiłbym metodę sumarycznej wagi wszystkich pasażerów i ich bagażu dla całego lotniska (wywołującej tą samą metodą dla każdego samolotu w pętli) po to aby można było ją bardzo łatwo zaprezentować na formatce (w statystykach całego lotniska) ... tym bardziej bym to zrobił, żeby zachować analogię względem istniejącej już metody Airport.CalculateAverageAge(). Poza tym ułoży nam się fajna piramidka :p (po kolei wywoływanych 3 metod) CalculateWeight dla wszystkich 3 klas : Passenger, Plane oraz Airport ... na szkolenie fajne.
  
• formatka: zmieniliśmy koncept (zrezygnowaliśmy z drzewa) - dlatego nazwa metody createTree() jest niefortunna
  
• formatka: również jej notacja metod używasz javowej nie c#, + metody mają się nazywać z dużej litery
  
• do statystyk lotniska dodałbym info o łącznej wadze pasażerów i bagażu
  
• ciągle uważam że odpalanie konkretnych funkcjonalności z menu górnego jest nieintuicyjne (menu górne służy do nawigowania po aplikacji, odpalania takich opcji jak "settings", "help" , "about" itd.) Tu użyłbym uniwersalnego klawisza (kontrolki Button) (który zmienia swoją treść (a może nawet ikonkę - będzie bardziej cool ;p) na "Flight" , "CheckIn" w zależności od zaznaczonego elementu). Ale dobra nie upieram się (zawsze jest argument, że nauczymy uczestnika kontrolki tego menu) ... przemyśl to jeszcze !
  

koszmarek napisał:
wszystkie klasy: notacja: przypominam, że zmienne publiczne wg mnie powinny nazywać się z małej a nie z dużej litery. (ustalenie moje, Foresa oraz bodajże Cygnusa vs ustalenie Straviego) 3:1 :p

Stravi napisał:

koszmarek napisał:
wszystkie klasy: notacja: przypominam, że zmienne publiczne wg mnie powinny nazywać się z małej a nie z dużej litery.  (ustalenie moje, Foresa oraz bodajże Cygnusa vs ustalenie Straviego) 3:1 :p

Fores napisał tak jak powinno być, czyli właściwości. Jeżeli nie chcemy wdrażać na tym etapie właściwości to zapisał, że powinno być z dużej litery - czyli 2:2


Ewentualnie, a może i ładniej (jeżeli nie możemy używać właściwości) powinniśmy napisać publiczne metody GetSize, SetSize, itp.

Kod:


public Plane Plane {get; set;}


Kod:


private Plane _plane;

public Plane GetPlane()
{
    return _plane;
}

public void SetPlane(Plane plane)
{
    _plane = plane;
}


Stravi napisał:Jaro pracujesz w walidacji?

Kod:

Age = r.Next(6, 80);
PassengerWeight = Math.Round(r.NextDouble() * (180 - 30) + 30, 2);
LuggageWeight = Math.Round(r.NextDouble() * 30, 2);

Kod:

return (double)Math.Round(PassengerWeight + LuggageWeight, 2);

Kod:

if (passengers > 0)
            {
                return (double)sum / passengers;
            }
            else
            {
                return 0;
            }

Kod:

if (passengers > 0)
            {
                return (double)sum / passengers;
            }

            return 0;

1. W programie w bloku Airport stats jest pole Average age, w którym można ograniczyć ilość cyfr wyświetlanych po przecinku do dwóch tak jak to jest w innych miejscach.
   
2. Wszędzie w programie jest rzutowanie na double lub jest zwracany double, a w jednym miejscu jest rzutowanie na float
   

   Kod:

   public bool Flight()
        {
            if ((float)DismissedPassengers() / Passengers.Length >= 0.75) // Sprawdzenie czy odprawione zostało minimum 75% pasażerów
            {
                foreach(Passenger passenger in Passengers)
                {
                    passenger.Flight();
                }
                return true;
            }
            else
            {
                return false;
            }
        }