Refaktoryzacja kodu

Question

Czy ten kod można jakoś zrefaktoryzować?

#include <iostream>
#include <windows.h>
#include <stdio.h>


using namespace std;

int s;
int m;
int g;

int main()
{
    while(1)
    {
        s++;
        int sec=s;

        Sleep(1000);

        if (s==60)
        {
            m++;
            s=0;

            if (m==60)
            {
                g++;
                m=0;
            }
        }

        system("cls");
        cout<<g<<":"<<m<<":"<<s<<endl;


    }
    return 0;
}

 

Answer 1

Przy tak krótkim programie nie za bardzo jest miejsce na refaktoryzację. Jedyne, co możnaby poprawić, to usunięcie zmiennych globalnych, ale też za wiele sensu w tym nie ma.

Ewentualnie mógłbyś się pokusić o uczynienie tego programiku multiplatformowym, poprzez usunięcie nagłówka "windows.h" i wprowadzenie odpowiedników funkcji z innych systemów operacyjnych. :)

Comments

Ach, jeszcze jedno zauważyłem. W każdym kolejnym obiegu pętli tworzysz nową zmienną
int sec - powinieneś definicję tej zmiennej wyrzucić nad pętlę, a przypisanie zostawić tam, gdzie jest. :)

---

Nope, powinno się je deklarować jak najbliżej miejsca pierwszego użycia (to, że nie jest tu używana, to inna sprawa). Przy złożonych typach można by się martwić o konstrukcję w każdej iteracji, ale w przypadku prostych typów kompilator raz alokuje miejsce na nie na stosie i nic więcej z nim nie robi, więc koszt tego jest dokładnie zerowy.

---

Dobry argument. :)

Answer 2

Usunięcie zmiennych globalnych zawsze ma sens. Radzę też zawsze dodawać spacje między operatorami, znacząco poprawia czytelność.
Przy okazji, kompilator nie narzeka na nieużytą zmienną sec?

Comments

Niekoniecznie zawsze. :) Jaki sens jest w tym programie?

Nie poprawia czytelności, nie powoduje zwiększenia poziomu bezpieczeństwa programu... Tutaj takie zmienne nie powodują też problemów z zasięgiem, bo cały kod programu to tylko krótki main, w dodatku mamy jedną jedyną jednostkę translacji. Nie ma się gdzie zgubić. :)

Oprócz sec, która faktycznie jest nieużyteczna - tego nie dopatrzyłem i za to ode mnie . :)

---

To proste - jako wzmacnianie dobrych praktyk. Nie powinno się myśleć "meh, krótki program, wrzucę zmienne przed main", tylko domyślnie od razu pisać je w funkcji, bo to nic nie kosztuje intelektualnie ani nawet w liczbie wciśniętych klawiszy na klawiaturze, a na dłuższą metę pomaga.

---

Skoro tak podchodzisz, to odruchowo powinno się również eliminować definicje zmiennych z pętli (oprócz jakichś warunkowych ścieżek, co oczywiste).

---

Tego zdania akurat nie zrozumiałem, sorry. Wyjaśnisz?

---

Oczywiście, że wyjaśnię. Eliminowanie definiowania zmiennych z pętli ma wzmocnić dobrą praktykę - zerowym kosztem nihilujemy narzut, związany z wielokrotnym tworzeniem obiektu.

Jak sam napisałeś, przy typach wbudowanych nie ma większej różnicy... Jednak dla obiektów klasy nietrywialnej już ma.

Nie mówię tutaj o okazjonalnym stworzeniu pomocniczej zmiennej, przy pewnym spełnionym warunku (spełnionym w m obiegach pętli, gdzie m << n, przy czym n jest liczbą iteracji) - to jest wygodne, czytelne i nie opóźnia naszego programu.

---

Umieszczenie deklaracji jak najbliżej miejsca faktycznego użycia jest naturalnym następstwem zasady nie używania zmiennych globalnych i ma takie samo uzasadnienie - zmniejsza zagracenie zewnętrznych scope'ów, co ułatwia rozumienie zależności w kodzie i poprawia jakość analizy/sugestii IDE.

W dodatku w przypadku obiektów ciężkich zadeklarowanych w jakimś warunku różnica jest pozytywna:

A a; // konstruowany za kazdym razem
if(warunek) {
    B b; // miejsce zarezerwowane na stosie, ale konstrukcja nastepuje tylko jesli kod dojdzie tutaj
}

Co do narzutu wielokrotnego tworzenia obiektu w pętli:

for(...) {
    A a; // konstruowany za kazdym razem
}

To można rozumować że jeśli to w zasadzie jest ten sam obiekt więc prawdziwe miejsce jego użycia jest poza pętlą.

No a jeśli za każdym razem i tak zupełnie zmieniasz wartość obiektu, na przykład:

std::string str;
for(int i = 0; i < 100000; ++i) {
    str = std::to_string(i);
    // zrob cos z str
}

To może się okazać że w zasadzie nie jest to w żaden sposób lepsze od normalnego konstruowania od nowa:

for(int i = 0; i < 100000; ++i) {
    std::string str = std::to_string(i);
    // zrob cos z str
}

 

---

A ja to bym chciał zobaczyć jak kompilator to zamienia na ASM:

std::string str;
for(int i = 0; i < 100000; ++i) {
    str = std::to_string(i);
    // zrob cos z str
}

Oraz to:

for(int i = 0; i < 100000; ++i) {
    std::string str = std::to_string(i);
    // zrob cos z str
}

Żeby mieć rzeczywisty obraz zamiast domysłów. Bo dziś można się spodziewać wszystkiego :) 

---

W ASMa to się nigdy nie bawiłem, chociaż niedługo pewnie się to zmieni (mam to w programie studiów). Ale zrobiłem mały - nieprofesjonalny wręcz - test, który jednak obrazuje to, o co mi chodziło.

Weźmy prosty program, pokazowy i wręcz... debilny. Kompilowany g++ 4.8.

#include <iostream>
using namespace std;

//#define IN_LOOP

struct Foo
{
    int *t;
    Foo()
    {
        t = new int[100];
        for(unsigned i = 0; i < 100; ++i)
            t[i] = 0;
    }
    ~Foo(){ delete[] t; }
    Foo(const Foo & f)
    {
        t = new int[100];
        for(unsigned i = 0; i < 100; ++i) t[i] = f.t[i];
    }

    Foo & operator=(const Foo &f)
    {
        if(this == &f) return *this;
        for(unsigned i = 0; i < 100; ++i) t[i] = f.t[i];
        return *this;
    }
};

int main()
{
    Foo f1;
    // jakieś operacje na f1
    f1.t[30] = 3;
    f1.t[50] = 5;
#ifndef IN_LOOP
    Foo f2;
#endif // IN_LOOP
    for(unsigned i = 0; i < 1000000; ++i)
    {
        #ifdef IN_LOOP
        Foo
        #endif // IN_LOOP
        f2 = f1;

    }
    return 0;
}

Po przedstawieniu średnich czasów wykonania dla 15 uruchomień tego programu (na tej samej maszynie, z porównywalnym obciążeniem systemu [uruchomiony "na czysto" system z otwartym C::B]) ze zdefiniowanym makrem IN_LOOP oraz z niezdefiniowanym, sądzę że nie trzeba się silić na rozbudowany komentarz.

IN_LOOP: 0.791667 s
IN_LOOP: 0.583467 s

Także jeśli nie podasz mi dowodu, że definiowanie obiektów nietrywialnych wewnątrz pętli nie wprowadza dodatkowego narzutu, to Ci po prostu nie uwierzę. :)

---

No tak, tutaj to jak najbardziej ma sens, bo masz już zaalokowany bufor. Nie powiedziałem że deklarowanie poza pętlą nigdy nie ma sensu, tylko że nie zawsze ma sens. Mogę pokazać odwrotny przypadek, w którym deklaracja przy przypisaniu jest lepsza, bo pozwala na RVO:

Foo func() {
    Foo f1;
    // jakieś operacje na f1
    f1.t[30] = 3;
    f1.t[50] = 5;
    return f1;
}
 
int main()
{
#ifndef IN_LOOP
    Foo f2;
#endif // IN_LOOP
    for(unsigned i = 0; i < 1000000; ++i)
    {
        #ifdef IN_LOOP
        Foo
        #endif // IN_LOOP
        f2 = func();
    }
}

Tutaj wersja IN_LOOP była u mnie mniej więcej dwa razy szybsza.

A ja to bym chciał zobaczyć jak kompilator to zamienia na ASM:

Dokładnie ASM nie pokażę bo to burdel, ale zrobiłem analogiczny kod z std::vector w którym dodałem do jego implementacji parę printf'ów (do std::string nie mogłem, pewnie używa implementacji z libstdc++) żeby pokazać różnicę w wywoływanych konstruktorach: http://hastebin.com/inolerufaj.cpp

Answer 3

1. zmianna sec nie jest używana

2. nazwy s,m,g nie dają nam zadnych informacji co robią i są do siebie podobne co sprawia ze nie dopatrzenie jakiejś literówki może zrobić błąd. Powinieneć zmienić je na pełne nazwy.

3. zawsze staraj się eliminować zagłebienia if (m==60) można wyciągnąć na zewnącz if(s==60)

4. po wyciągnieciu dochodzimy do wniosku ze nie mamy potrzeby za każdym razem sprawdzania if(m==60) więc robimy funkcje i stosujemy return aby nie wykonywać nie potrzebnie tego porównania

5. teraz można zauważyć ze oba if`y są bardzo podobne i można dojśc do wniosku ze można je wykonywać w pętli gdybyśmy zmienne zamienili na 3 elementową tablice

6 pewnie jeszcze coś by się znalazło.

Comments

A ja nadal nie widzę sensu w refactoringu takiego banału. :)

---

@draghan Nie wiem w jakim wieku jesteś ale jak zaczniesz pracować docenisz refraktoryzowanie kodu. Wystarczy pierwsze spotkanie z czyimś kodem gdzie są jednoliterowe zmienne i już bedzie ci się chciało klnąć i zmienić prace bo nic nie mozesz znaleźć. Pisanie przejszystego kodu to też szanowanie współpracowników. Chodzio o szybkośc analizowania kodu ile zajeło ci przeanalizoweanie kodu z przykładu ? Pewnie z minute lub dwie i dowiedziałeś się ze dodaje jeden do zmiennej s zatrzymuje program na sekunde i pokazuje dane. A teraz ile zajmie ci przeanalizowanie tego ?

int main()
{
    while(1)
    {
		Sleep(1000);
		addOneSecond();
		showTime();
    }
    return 0;
}

O wiele szybciej dowiedziałeś się ze program jest zatrzymywany ze jest dodawana sekunda i ze czas jest potem pokazywany już wiesz co robi program! Teraz zmienne masz zmienne s,m,g możesz tylko zgadywać co to jest. Każdy programista lubi zagadki więc każdy się zatrzymuje i myśli hmmm może chodzić o czas. Taki zapis spowodował by brak zastanawiania się.

int hour, minute, second;

Dodatkowo łatwiej znaleźć błąd w kodzie dzięki opisowym nazwą.

if (s==60)
{
	m++;
	s=0;
	if (m==60)
	{
		s++;
		m=0;
	}
}

Prawdopodobnie troche ci zajmie gdzie jest błąd lecz po dodaniu nazw znajdziesz go szybciej 

if (second==60)
{
	hour++;
	second=0;
	if (minute==60)
	{
		hour++;
		minute=0;
	}
}

a tak moim zadniem powinien wyglądać kod całości 

#include <iostream>
#include <windows.h>
#include <stdio.h>

using namespace std;

int hour, minute, second;
void addOneSecond();
void showTime();

int main()
{
	while(1)
	{
		Sleep(1000);
		addOneSecond();
		showTime();
	}
	return 0;
}

void addOneSecond()
{
	second++;

	if (second<60)
	{
		return;
	}

	minute++;
	second=0;

	if (minute<60)
	{
		return;
	}

	hour++;
	minute=0;
}

void showTime()
{
	system("cls");
	cout<<hour<<":"<<minute<<":"<<second<<endl;
}

---

Nawet nie śmiem zaprzeczyć - byłbym skończonym głupcem. Zgadzam się ze wszystkim - tak to powinno wyglądać. :) Ale proszę, powiedz mi: jaki jest sens refactoringu konkretnie tego programu?

Przecież to jest HelloWorld++, a refactoring przeprowadza się w określonym celu, którym jest ułatwienie pielęgnacji programu.

---

draghan aby ćwiczyć umięjętności pisania czystego kodu. Poza tym gdyby ktos wkleił swój kod na 500 linijek nikt by nie odpowiedział a tak ma już sporo podstaw które może zastosować w dłuższym kodzie. Pamiętajmy ze każdy najmniejszy kod się rozrasta ja tu widze czas gry wyświetlany w roku ekranu teraz czas na gre! Zawsze zaczyna się od małych kodów :)

---

@draghan, czytając Twoją wypowiedź śmiem twierdzić, że nie masz w sobie duszy programisty. Nie jesteś zakochany w pięknym kodzie i nie jest to dla Ciebie ważne. W każdym programie, nawet najbanalniejszym i najmniejszym powinno się stosować takie zasady, jakie podał @rafal612b. Wyobraź sobie, że ten program wykorzystuje tylko proste QuickSort. QuickSorty czy inne algorytmy wyszukiwania czy sortowania to przykłady programów o ekstremalnie niskiej czytelności.

Ale wyobraź sobie, że zamiast różnych zmiennych typu t[100] m1 a t b itp. to jest to tak czytelny algorytm, który sam tłumaczy siebie. Nie dość że programista wie z nazwy funkcji że to quicksort to jeszcze może sobie na szybko przeanalizować czy robi to dobrze. A uwierz mi, że często się zdarza przy refaktoringu że kilka rzeczy w takich algorytmach można spokojnie usunąć :) CHociażby na cele czytelności jak i optymalizacji.

Roy Osherove w swojej książce podaje, że refaktoryzacja jest takim procesem zmieniania kodu, aby nie zmienić jego zachowania a uprościć jego użycie i uelastycznić na zmiany.

Teraz jest funkcja addOneSecond, ale w przyszłości chłopak będzie chciał to zmienić i zrobi funkcję addSecond(int seconds) i wtedy będzie dodawać określoną ilość sekund. Będzie mógł ten kod wykorzystać w jakimkolwiek innym programie, nie będzie musiał tego przepisywać, tworzyć na nowo. Zaincluduje odpowiedni plik i ma.

---

@draghan, czytając Twoją wypowiedź śmiem twierdzić, że nie masz w sobie duszy programisty

Dzięki... ;__; Wychodzi więc na to, że jestem bezduszny. >.-

---

Odniosłeś się akurat do najmniej znaczącego zdania w mojej wypowiedzi

---

Bo resztę akceptuję i nie zamierzam polemizować. :)

Answer 4

Tak swoją drogą co masz na myśli używając tego słowa? Chodzi o optymalizację aby program szybciej działał? Czy może, eliminację kroków?

Bo refaktoryzacja to także wydzielenie takich fragmentów kodu na funkcje, tworzenie fabryk, nadawanie zmiennym znaczących nazw a nie "a, s, m" które nic Ci nie powiedzą za 2 miesiące kiedy będziesz czytać kod. Poza tym g m s to już bardziej H m s ewentualnie h m s (w zależności jaki format by się przyjęło. W pętli while np zamiast 1 to ładniej byłoby "true". TO jest refaktoryzacja, a optymalizacja moim zdaniem to część refaktoryzacji.

Answer 5

Od siebie mogę dać radę taką, aby nie skracać zmiennych gdyż to utrudnia czytanie takiego kodu.