Ocena kodu.

0

Dzień dobry.

Dostałem zadanie:

Wprowadzić ciąg liczb, którego weilkości n. Wprowadzić dwie dodatkowe liczby a i b. We wprowadzonym ciągu wszystkie liczby, które są podzielone przez a i b , zamienić na zero. Wyprowadzić nowy ciąg.

Napisałem kod:

#include <stdio.h>
#include <stdlib.h>


void readInput(char*, void*);
void replace_int(int*, size_t, int, int, int);

int main(int argc, char** argv){
    size_t size=0;
    int a=0;
    int b=0;

    printf("Wprowadz ilosc liczb: ");
    readInput("%Iu", &size);

    printf("Wprowadz pierwsza liczbe do sprawdza podzielnosci: ");
    readInput("%d", &a);
    
    printf("Wprowadz druga liczbe do sprawdzania podzielnosci: ");
    readInput("%d", &b);

    int* numbers = (int*)calloc(size, sizeof(int));
    for(size_t i=0;i<size;++i){
        printf("Wprowadz %Iu/%Iu liczbe do tablicy: ", i+1, size);        
        readInput("%d", &numbers[i]);
    }    
   
    replace_int(numbers, size, a, b, 0);

    for(size_t i=0;i<size;++i){
        printf("%d ", numbers[i]);
    }

    free(numbers);
 
    return 0;
}

void replace_int(int* tab, size_t tab_size, int div_a, int div_b, int new_val){
    if(tab==NULL) return;
    
    for(size_t i=0;i<tab_size;++i){
       int mod = tab[i]%div_a + tab[i]%div_b;
       if(!mod)
           tab[i] = new_val;

    }
       
}

void readInput(char* format, void* variable){
     int isInputValid = scanf(format, variable);
     if(!isInputValid)         
        do{             
            fflush(stdin);             
            isInputValid = scanf(format, variable);
         }while(!isInputValid); 
}

Ogólnie to w C nie piszę, wiele rzeczy mi się myliło z C++, więc prosiłbym o wszelkie uwagi.

Dziękuję z góry.
Pozdrawiam.

1

Generalnie ładnie, nie ma za bardzo nadmiarowego kodu, duży plus. Mam jednak uwagi:

  1. Funkcja readInput wpadnie w nieskończoną pętlę jeśli strumień wyjścia zostanie zamknięty. Taka sytuacja zajdzie jeśli przekierujesz niepełny plik na wejście lub zamkniesz strumień w czasie czytania (ctrl+d w linuksowej konsoli). Aby uniknąć tego rozpatrz sytuację scanf()<0. Poza tym nie wiem po co tyle razy fflush(), zwykle nie powinien być potrzebny w ogóle.

  2. Używasz funkcji calloc(), nie jest to błąd, ale sam uzylbym tablicy o rozmiarze zmiennej (VLA) ze standardu C99. Wiem, że w takim programie raczej nie odczujesz kosztu czasowego, fragmentacji pamięci, ani innych problemow związanych z użyciem malloc, ale doświadczenie z C nauczyło mnie nie używać tego jeśli nie ma takiej potrzeby. Stos ma te zalety że automatycznie sprząta, jest szybszy i rzadziej generuje trudne do wykrycia błędy. Jak się przepełni stos to się posypie I debugger of razu napisze dlaczego, jak przekazujesz zmienne lokalne p przez wskaźnik możesz przypadkiem uszkodzić ramkę stosu, ale to łatwiejsze niż problem z napisanymi metadanymi sterty. Niby błąd podobny i do obu spotkałem się z ostrzeżeniami, ale w przypadku stosu przynajmniej wiesz w której funkcji szukać. No ale wiadomo że nie zawsze można użyć stosu...

  3. Nazwa funkcji replace_int nie wyraża w pełni tego co ona robi. Poza tym jej ciało jest mało czytelne.

  4. Brak konsekwencji w nazewnictwie funkcji, raz w stylu C raz wielbłąd.

Oczywiście nie sprawdzałem czy działa. Warto pisać testy...

0

Działać działa. //probably //1st bug to fix - negative numbers at input are passing

  1. fflush jest dlatego, że jak podam błędny input, to scanf nie chce już nic czytać
    Sugerujesz, żeby zamiast !isInputValid dać isInputValid<1 ?

  2. No właśnie. Pomyłka z C++. VLA. Skoro można ich użyć, to użyję, nie ma co.. Wygoda wygodą.

  3. Ok, zmienię na replace_if_divisible_int (trochę długie, ale co..). A ciało zrobiłem najbardziej czytelne jak tylko potrafiłem. Nie wiem co można tam zmienić.
    int mod = ... chyba rozumiesz

readInput - funckja
replace - funkcja _int - typ parametru
gdyby readInput przyjmował tylko int, to nazywałby się readInput_int

0

1. Ten fragment:

     if(!isInputValid)         
        do{             
            fflush(stdin);             
            isInputValid = scanf(format, variable);
         }while(!isInputValid); 

można uprościć, skoro pętla ma się wykonać tylko jeśli !isInputValid:

        while(!isInputValid)
        {
            fflush(stdin);             
            isInputValid = scanf(format, variable);
         }

Owszem, można scanf() umieścić w samym warunku pętli i uniknąć tworzenia zmiennej, ale zmienna przynajmniej fajnie określa co sprawdzamy.

2. Jak jeszcze w przypadku isInputValid zaprzeczenie w warunku nawet pasuje (ponieważ nazwa sugeruje/udaje wartość boolowską), to już stosowanie zaprzeczenia w tym fragmencie:

       int mod = tab[i]%div_a + tab[i]%div_b;
       if(!mod)

wymaga mimo wszystko chwilki by dojść do tego, co tutaj sprawdzasz. Moim zdaniem można pokusić się tutaj o zwyczajne porównanie:

       int mod = tab[i]%div_a + tab[i]%div_b;
       if(mod == 0)

Poza tym, całość wygląda OK (+ uwagi od kolegi wyżej).

0

Ok, dzięki. W takim razie kod wygląda teraz tak:

#include <stdio.h>
#include <stdlib.h>


void readInput_size_t(size_t*);
void readInput_int(int*);
void replaceIfDivisible_int(int*, size_t, int, int, int);
void insertToTable_int(int*, size_t);

int main(int argc, char** argv){
    size_t size=0;
    size_t a=0;
    size_t b=0;

    printf("Wprowadz ilosc liczb: ");
    readInput_size_t(&size);

    printf("Wprowadz pierwsza liczbe do sprawdzania podzielnosci: ");
    readInput_size_t(&a);

    printf("Wprowadz druga liczbe do sprawdzania podzielnosci: ");
    readInput_size_t(&b);

    int numbers[size];
    insertToTable_int(numbers, sizeof(numbers)/sizeof(int));

    replaceIfDivisible_int(numbers, size, a, b, 0);

    for(size_t i=0;i<size;++i){
        printf("%d ", numbers[i]);
    }

    free(numbers);

    return 0;
}

void replaceIfDivisible_int(int* tab, size_t tab_size, int div_a, int div_b, int new_val){
    if(tab==NULL) return;

    for(size_t i=0;i<tab_size;++i){
       int mod = tab[i]%div_a + tab[i]%div_b;
       if(mod==0)
           tab[i] = new_val;

    }

}

void readInput_int(int* variable){
    int isInputValid = scanf("%d", variable);
    while(isInputValid<=0){
        fflush(stdin);
        isInputValid = scanf("%d", variable);
    }
}
void readInput_size_t(size_t* variable){
    int input;
    int isInputValid = scanf("%d", &input);
    while(isInputValid<=0 || input<0){
        fflush(stdin);
        isInputValid = scanf("%d", &input);
    }
    *variable = (size_t)input;
}
void insertToTable_int(int* tab, size_t tab_size){
    for(size_t i=0;i<tab_size;++i){
        printf("Podaj %u/%u liczbe do tablicy: ", i+1, tab_size);
        readInput_int(&tab[i]);
    }
}

I w sumie wszystko działa, ale dalej nie jestem pewien czystości kodu. Doradzicie coś?.. Heh.
// Jeśli chodzi o readInput, to myślę, żeby zrobić enum INT/SIZE_T i w zależności od tego jakoś tą funkcję zmienić, ale może to wyjść pokracznie

0

Z punktu widzenia wydajności konstrukcja:

int mod = tab[i]%div_a + tab[i]%div_b;
if(!mod)
   tab[i] = new_val;

(choć wydaje się elegancka) nie jest optymalna.
Jeśli tab[i]%div_a nie jest spełniony, to i tak zostanie policzone tab[i]%div_b

0

To jak inaczej można by to zapisać?

0

Możliwości jest wiele

	if(!(tab[i]%div_a))
		if(!(tab[i]%div_b))
	        	   tab[i] = new_val;

	if(!(tab[i]%div_a) && !(tab[i]%div_b))
       	   tab[i] = new_val;

w powyższej wersji kompilator to zoptymalizuje i drugiego warunku nie policzy.
Ty użyłeś "+". To spowoduje, że kompilator policzy oba składniki i je doda. I dopiero wynik sprawdzi.
Musisz spowodować, że do pierwszego sprawdzenia doszło już po wyliczeniu pierwszego składnika.

0

Jak już optymalizujemy - to optymalizujmy dalej ;-).

Czemu wynik działania np. tej funkcji

void readInput_size_t(size_t* variable);

odbierasz przez wskaźnik?

Czemu nie zrobisz tak? (może są jakieś powody - uzasadnij)

size_t readInput_size_t(){
...
	return input;
}

A jeśli już masz przez wskaźnik, to po co tworzysz dodatkową zmienną input, którą na koniec przypisujesz do variable? W tym rozwiązaniu jedno tworzenie zmiennej i jedno przypisanie (dwie operacja) są nadmiarowe.

Dalej - cała ta konstrukcja while

while(isInputValid<=0 || input<0){
        fflush(stdin);
        isInputValid = scanf("%d", &input);
 }

jest taka sobie :-)

Ja już dawno nie programowałem dla konsoli, musiałem zajrzeć do dokumentacji :-), ale generalnie - w tym while sprawdzasz dwa warunki. Pytanie - po co? Czy nie da się jednym sprawdzeniem tego "załatwić". Kiedy input może być mniejsze od 0?
I w dwóch wierszach wykonujesz scanf. Czy nie możesz tego przerobić na do{}while, żeby scanf tylko raz występowało w tym kodzie?

Poza tym - w tej wersja jak masz teraz - to się nie skompiluje (masz int numbers[size];)
To nie przejdzie, w takim zapisie size musi być stałą.

2

optymalizacja optymalizacją ale
po

if(!(tab[i]%div_a))
        if(!(tab[i]%div_b))
                   tab[i] = new_val;

przed

int mod = tab[i]%div_a + tab[i]%div_b;
if(!mod)
   tab[i] = new_val;
  1. ile zyskuje sie na tej jednej linii kodu? Czy na pewno ma to takie znaczenie? Czy ten fragment kodu jest bottle neck? Cytujac Donalda Knuth'a

Programmers waste enormous amounts of time thinking about, or worrying about, the speed of noncritical parts of their programs, and these attempts at efficiency actually have a strong negative impact when debugging and maintenance are considered. We should forget about small efficiencies, say about 97% of the time: premature optimization is the root of all evil. Yet we should not pass up our opportunities in that critical 3%.

http://wiki.c2.com/?PrematureOptimization

  1. z punktu widzenia czytelnosci, kod zaczyna byc nieczytelny

  2. debugowanie tego to jest tragedia

1 użytkowników online, w tym zalogowanych: 0, gości: 1