Zadanie SPOJ - parzyste, nieparzyste - Code Review

Question

Witam. Zrobiłem ostanio zadanie na SPOJu, które pomyślnie przeszło testy:https://pl.spoj.com/problems/PP0602A/

Oto mój kod:

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

int main() {

	int liczbaTestow,iloscLiczb;
	scanf("%d", &liczbaTestow);
	int** tab = (int**)malloc(sizeof(int*)*liczbaTestow);
	for (int i = 0; i < liczbaTestow; i++) {
		scanf("%d", &iloscLiczb);
		tab[i] = (int*)malloc(sizeof(int) * iloscLiczb);
		for (int j = 0; j < iloscLiczb; j++) {
			scanf("%d", &tab[i][j]);
		}

		/*for (int k = 0; k < iloscLiczb; k++) {
			if (k % 2 != 0) {
				printf("%d ", tab[i][k]);
			}
		}
		for (int k = 0; k < iloscLiczb; k++) {
			if (k % 2 == 0) {
				printf("%d ", tab[i][k]);
			}
		}*/
	
		for (int k = 1; k < iloscLiczb; k += 2) {
			printf("%d ", tab[i][k]);
		}
		for (int k = 0; k < iloscLiczb; k +=2) {
			printf("%d ", tab[i][k]);
		}
		printf("\n");
	}

	for (int i = 0; i < liczbaTestow; i++) {
		free(tab[i]);
	}
	free(tab);
	return 0;
}

Mam 2 pytanie, odnośnie mojego kodu:

1. Jeśli chodzi o samo wypisywanie liczb, to czy lepiej robić, to tak jak w moim kodzie, czy może używać tego co zakomentowałem, czy może jest jeszcze jakiś lepszy sposób?

2.Czy pomysł z dynamiczną alokacją pamięci i jej zwalnianiem, jest optymalnie i czy dobrze to wykonałem?

Mam nadzieję, na Waszą pomoc ! Czy macie może jeszcze jakieś pomysły na zoptymalizowanie tego kodu?

Pozdrawiam.

Answer 1

Na początek CR.

1. W kodzie masz ostrzeżenia o arytmetyce signed/unsigned w 8 i 11 linii. liczbaTestow powinna być unsigned (tak jak iloscLiczb oraz indeksy i, j, k)

2. Dopuszczalna sygnatura main(...) w C to: int main(void).

3. W C jest zbędne rzutowanie wyniku malloc(...). To jest void * i zawsze poddaje się automatycznie konwersji na typ docelowy.

BTW. Zastanów się czy alokacja dynamiczna jest tu potrzebna jeśli masz tylko 100 elementów :)

4. Przy malloc(...) i innych podobnych funkcjach, stosuje się idiom z C który do argumentu sizeof, przyjmuje to co jest po lewej przypisania. Czyli nie tak:

int** tab = malloc(sizeof(int*)*liczbaTestow);

... tylko tak:

int** tab = malloc(sizeof(*tab) * liczbaTestow);

Dzięki temu będziesz mógł zmienić typ danych tab bez poprawiania ciała malloc(...)

5. Jak wyprowadzasz jeden znak, printf(..) w 33 zamień na putchar(...)

6. Preferuj pre (inc/dec) przed post (pre/dec). Szczególnie w pętlach for(..) w których często jest to obojętne.

7. W przypadku kodowania zawodowego, standardy wymagają aby operacje związane z kolejnością ewaluacji operatorów ujmować w nawiasy. Tak więc (linia 13) nie tak:

scanf("%d", tab[i][j]);

tylko tak:

scanf("%d", &(tab[i][j]));

Tak wiem że to uciążliwe, ale narzędzia sprawdzania kodu Ci to wytkną :-/

W efekcie kod może wyglądać tak (usunąłem część zakomentowaną):

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

int main(void) {

    unsigned liczbaTestow,iloscLiczb;
    scanf("%d", &liczbaTestow);
    int** tab = malloc(sizeof(*tab) * liczbaTestow);
    for (unsigned i = 0; i < liczbaTestow; ++i) {
        scanf("%d", &iloscLiczb);
        tab[i] = malloc(sizeof(*tab) * iloscLiczb);
        for (unsigned j = 0; j < iloscLiczb; ++j) {
            scanf("%d", &(tab[i][j]));
        }

        for (unsigned k = 1; k < iloscLiczb; k += 2) {
            printf("%d ", tab[i][k]);
        }
        for (unsigned k = 0; k < iloscLiczb; k += 2) {
            printf("%d ", tab[i][k]);
        }
        putchar('\n');
    }

    for (unsigned i = 0; i < liczbaTestow; ++i) {
        free(tab[i]);
    }
    free(tab);

    return 0;
}

Teraz co do algorytmu.

1. Alokujesz dane ja tyle testów ile podano. Jeśli nie ma konieczności komunikowania się przez dane pomiędzy nimi, po co? Wystarczy alokowanie pamięci na 1 test. Ponowna alokacja możliwa za pomocą realloc(...). Większość systemów da Ci nawet to samo miejsce w pamięci. A dzięki temu unikniesz kłopotliwego czyszczenia tablic.

2. Kod prezentacji danych (parzyste/nie-parzyste pozycje) powtarza się. Ma tylko inny indeks startowy. Warto wyprowadzić go do funkcji. Podobnie wczytanie danych. W rzeczywistym projekcie jeśli pojawi się zarzut "bo to mała funkcja", zrób ją inline i static.

 

Comments

Witam, bardzo dziękuję za uwagi!

Czytając je i wdrażając w tym zadaniu, kilka rzeczy mnie zaciekawiło i chciałbym się jeszcze o coś zapytać.

1. Piszę w visual studio code 2019 i jeśli usuwam rzutowanie, to kompilator pokazuje mi błąd.

2.Cy mógłby mi pan rozpisać ten przykład z reallociem, ponieważ nie umiem jeszcze wdrażać tego rozwiązania w moich projektach.

3. Dlaczego powinienem preferować  pre (inc/dec) przed post (pre/dec)? Znam teoretyczną różnicę między tymi technikami, ale niestety nie wiem czemu praktycznie tego powinno się używać.

4. Czemu putchar, jest bardziej zalecany niż printf? Oraz czy void, pisany w nawiasie przy main jest ciągle zalecany, ponieważ zauważyłem, że wiele osób unika tej praktyki.

To mój kod po kilku zalecanych zmianach:

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

static void wypisz(int start, int iteracja, int iloscLiczb, int **tab);

int main(void) {

	unsigned int liczbaTestow, iloscLiczb;
	scanf("%d", &(liczbaTestow));
	int** tab = (int**)malloc(sizeof(*tab) * liczbaTestow);
	for (unsigned int i = 0; i < liczbaTestow; ++i) {
		scanf("%d", &(iloscLiczb));
		tab[i] = (int*)malloc(sizeof(tab) * iloscLiczb);
		for (unsigned int j = 0; j < iloscLiczb; ++j) {
			scanf("%d", &(tab[i][j]));
		}

		wypisz(1, i, iloscLiczb, tab);
		wypisz(0, i, iloscLiczb, tab);

		putchar('\n');
	}

	for (int i = 0; i < liczbaTestow; ++i) {
		free(tab[i]);
	}
	free(tab);
	return 0;
}

static void wypisz(int start, int iteracja, int iloscLiczb, int **tab) {
	for (unsigned int k = start; k < iloscLiczb; k += 2) {
		printf("%d ", tab[iteracja][k]);
	}
}

 

---

Ad. 1

Ustaw kompilację w trybie C a nie C++. W C++ należy robić rzutowanie a w C nie. Poza tym, co do języka C, kompilator MS nie jest obecnie sensownym wyborem. W przeciwieństwie do C++, firma nie rozwija go i ciągle nie jest w pełni kompatybilny c C90.

Ad. 3

W przypadku post, nakazujesz kompilatorowi przetrzymywać starą wartość. Sugerujesz że tego potrzebujesz kiedy... tak naprawdę nie potrzebujesz. Usłyszysz argumenty "bo dziś kompilatory umieją to wykryć", ale to nie jest prawda bo nie każdy kompilator to wykrywa.

Ad. 4

putchar(...), wygeneruje mniejszy kod oraz jasno informuje że chcesz wyprowadzić znak a nie formatowany string. Znów, kompilator głównych platform zapewne sam to zoptymalizuje, ale kod czytają ludzie.

Co do main(void), standard wyraźnie mówi że to jest jedna z sygnatur funkcji w C. Brak argumentu w funkcji w C, powoduje że przyjmuje ona nieoznaczoną ilość argumentów (w przeciwieństwie do C++).

Ad. 2.

Proszę bardzo. Na Twoim kodzie z minimalnymi poprawkami:

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

static void wypisz(int start, int iloscLiczb, const int *tab);

int main(void) {

    unsigned int liczbaTestow, iloscLiczb;
    scanf("%d", &(liczbaTestow));
    int * tab = NULL;
    for (unsigned int i = 0; i < liczbaTestow; ++i) {
        scanf("%d", &(iloscLiczb));
        tab = realloc(tab, sizeof(*tab) * iloscLiczb);
        for (unsigned int j = 0; j < iloscLiczb; ++j) {
            scanf("%d", &(tab[j]));
        }

        wypisz(1, iloscLiczb, tab);
        wypisz(0, iloscLiczb, tab);

        putchar('\n');
    }

    free(tab);
    return 0;
}

static void wypisz(int start, int iloscLiczb, const int *tab) {
    for (unsigned int k = start; k < iloscLiczb; k += 2) {
        printf("%d ", tab[k]);
    }
}

Osobiście, napisał bym to tak:

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

#define MAX_VALUES_COUNTER 100

static inline void show_by2_table(const int * table, unsigned start_index, const unsigned size);
static inline void show_even_odd_table(const int * table, const unsigned size);
static inline void input_table(int * table, const unsigned size);

int main(void) {

    unsigned testCounter = 0;
    unsigned valuesCounter = 0;
    scanf("%d", &testCounter);

    int table[MAX_VALUES_COUNTER + 1];

    for (unsigned i = 0; i < testCounter; ++i) {
        scanf("%d", &valuesCounter);
        input_table(table, valuesCounter);
        show_even_odd_table(table, valuesCounter);
    }

    return 0;
}

static inline void show_by2_table(const int * table, unsigned start_index, const unsigned size) {
    for (unsigned i = start_index; i < size; i += 2) {
        printf("%d ", table[i]);
    }
}

static inline void show_even_odd_table(const int * table, const unsigned size) {
    show_by2_table(table, 1, size);
    show_by2_table(table, 0, size);
    putchar('\n');
}

static inline void input_table(int * table, const unsigned size) {
    for (unsigned j = 0; j < size; ++j) {
        scanf("%d", &(table[j]));
    }
}