Ocena kodu aplikacji

Napisałem sobie (dla sportu w zasadzie) program dzięki któremu możemy zaplanować wyłączanie komputera po pewnej godzinie.
Chciałbym prosić o ocenę kodu.
Jest to moja pierwsza aplikacja w Qt więc nie bijcie za mocno ;)
Proszę o ocenę czy dobrze rozplanowałem klasy oraz kodu źródłowego - na tym mi najbardziej zależy.

Sam mam klika wątpliwości co do jakości mojego kodu ale liczę na opinie kogoś bardziej doświadczonego.
Czytałem coś o zasadzie że jedna klasa powinna być odpowiedzialna za jedną rzecz jednak nie rozumiem jeszcze za dobrze Qt więc czy klasa głównego okna powinna posiadać np QTimery czy tylko kontrolki które można kliknąć a kliknięcie wywołuje metodę zawartą w innej klasie?

Link do GitHuba: skasowano

FAQ
Konfiguracje zapisuj do (.ini,.whatever)
*Nie, wolę do .txt

Czemu nie używasz inteligentnych wskaźników
Nie lubię ich, kiedyś zrozumiem że są lepsze ale ten czas nie nadszedł

Na szybko:

Nazwa repo inna niż projektu.
Wywal zakomentowane linijki kodu. Masz githuba od tego lub wzorzec strategia.
Nie baw się w notację węgierską czy inną. Czyli żadne CSystemShutdown
Poszukałbym jakiejś biblioteki do odpalania procesów zamiast korzystać bezpośrednio z winapi
ConfigurationReader też nie wygląda dobrze, pętle while są nieczytelne. Spróbuje przerobić to na XML - Qt ma klasy do parsowania. Dowiedz się o dwóch podejściach SAX (Simple Api for XML czyli parsowanie pętlą "na raz") i DOM (Document Object Model czyli wszystania całego XML do pamięci i łażeniu po nodeach) Pozwoli CI to poznać lepsze api do parsowania plików.
ConfigurationReader zawiera pełno Magiczny znaków a to jakieś # a to jakieś 7 w pętli? Zrób z tego stałe dobrze nazwane

EDIT1:
CSystemShutdown powinien być przerobiony na coś takiego: ShutdownInvoker jako klasa czysto abstrakcyjna (interfejs), oraz WindowsShutdownInvoker i LinuxShutdownInvoker jako konkretne implementacje. Dodatkowo ShutdownFactory które w zalezności od systemu zwraca do kodu odpowiednią impementację.

int error = reboot(LINUX_REBOOT_MAGIC1,LINUX_REBOOT_MAGIC2,LINUX_REBOOT_CMD_POWER_OFF, 0);
if(error == -EINVAL || error == EINVAL)

Tak być nie może. reboot zwraca 0 gdy sukces, lub -1 gdy był błąd ORAZ ustawia zmienną errno na EFAULT, EINVAL albo EPERM. Prawidłowa forma to:

int error = ;
if(reboot(LINUX_REBOOT_MAGIC1,LINUX_REBOOT_MAGIC2,LINUX_REBOOT_CMD_POWER_OFF, NULL) < 0)
{
  switch (errno)
  {
  case EPERM:
    /* permission error */
  case EINVAL:
    /* wrong magic number or cmd */
    /* etc */
  }
}

Lub po prostu zlewasz errno i w środku ifa zgłoś błąd.

Trzecim argumentem dla reboot jest wskaźnik, więc lepiej podać tam NULL zamiast 0.

EDIT1:

Klasa shutdown jest niepotrzebna. Klasa, która ma jedną funkcję i nie posiada żadnego stanu w sobie to nie klasa - to opakowana funkcji. Wywal klasę i na jej miejsce po prostu wrzuć zwykłą funkcję shutdown. Wyjdzie na to samo, a ile mniej kodu, i o ile czytelniej!

EDIT2:

    if(m_handleFile.open(QIODevice::ReadOnly | QIODevice::Text) == false)
    {///CAN NOT OPEN FILE
       #warning error_error
        return;
    }

Zdajesz sobie sprawę, że ten #warning error_error jest dyrektywą preprocesora i te warning wyświetli się ZAWSZE przy kompilacji programu, w runtime'ie nie będzie żadnego błędu?

EDIT3:

// If the stream has read to the end of the file, readLine() will return a null QString.
        if( textLine.isNull() == true )break;</cpp>

Jesteś pewien, że przy odczytywaniu ostatniej linijki, która nie ma znaku nowej linii, program nie zakończy parsowania w tym miejscu, bez parsowania ostatniej linii? Nie znam QT więc to takie do sprawdzenia.

@mlyszczek - poprawa w toku :)

Zapomniałem że podobnie jak w WinAPI zmienne są przechowywane w specjalnych zmiennych .

int error = reboot(LINUX_REBOOT_MAGIC1,LINUX_REBOOT_MAGIC2,LINUX_REBOOT_CMD_POWER_OFF, 0);
if(error == -EINVAL || error == EINVAL)

A co sądzisz o fragmencie:

if(error == -EINVAL || error == EINVAL)

chodzi o to czy należy sprawdzać te dwie możliwe wartości (jak już wiem zmiennej errno)?
Pytam gdyż w źródłach Linuksa znalazłem że syscall reboot( magic1, int, magic2, unsigned int, cmd,void __user *, arg)
może zwrócić

-EINVAL

Linijka 297 - https://github.com/torvalds/linux/blob/master/kernel/reboot.c

Ten kod może być przejściowy, tj po drodze może być jakaś funkcja, która łapie to -EINVAL, zapisuje EINVAL do errno i zwraca -1. Wywołanie systemowe może nie móc pisać do errno, bo errno jest osobne dla każdego wątku.

Według dokumentacji nie może. reboot zwraca tylko i wyłącznie 0 albo -1, nigdy nie zwróci -EINVAL. EINVAL zostanie zapisane do errno. errno jest ZAWSZE dodatnie i nie ma potrzeby sprawdzać dwóch wartości. To co masz jest na pewno złe.

Nie musisz sprawdzać errno, wystarczy że sprawdzisz return code. sprawdzanie errno jest opcjonalne. To jest po prostu dodatkowa informacja, możesz w if na przykład zrobić printf("reboot error(%d), %s\n", errno, strerror(errno)); strerror(errno) zwróci błąd jako opisowy string.

OK,dzięki wielkie bałem sie że errno może miec wartości również ujemne.
To że errno jest osobne dla wątku to wiem ale dziękuje za potwierdzenie.
W sumie racja mimo że to wywołanie systemowe to logiczne że musi być jakaś warstwa pośrednicząca która własnie np zapisuje do zmiennej errno.

Co do ostatniego - twierdzisz że mi wystarczy sprawdzić czy np reboot zwraca sukces czy nie a errno jest dodatkową informacją mówiącą co poszło nie tak ?

Tak, jak dostaniesz 0 od reboot, to w sumie nie musisz się już niczym martwić, nawet szkoda zasobów zwalniać;-)

W Linuksie mamy standard. -1 mówi, że COŚ poszło nie tak, errno mówi CO poszło nie tak.

Witam

Zmienił... zrefaktoryzowałem kod zgodnie z waszymi wskazówkami :)

Nazwa repozytorium już jest taka sama jak projektu.
Zombie code wywalony.
ConfigurationReader zmieniłem trochę - wydaje mi się że jest bardziej czytelne. Tym razem zależy mi na napisaniu własnego parsera więc nie skorzystam z XML ani klas udostępnianych przez Qt - następnym razem.
Wywaliłem magiczne liczby - dodałem komentarz informujący iż znak "#" nakazuje parserowi pominąć linie tekstu która go zawiera.
Zdecydowałem się na zniszczenie klasy CShutdownSystsem i zrobienie z niej funkcji. Implementacja funkcji zależy od zdefiniowanych makr.
Uznałem że stosowanie Fabryki itd w tym przypadku jest niepotrzebne.
Usunąłem kilka mniejszych błędów oraz niepotrzebne ostrzeżenia - zastąpiłem te ostrzeżenia komentarzami TODO.

Znacznie zmodyfikowawszy funkcję main - proszę o ocenę tych zmian.
Mam zamiar dodać okno informujące o ewentualnych błędach - patrz komentarze TODO

link skasowany

Będę wdzięczny za wszelkie sugestie.