[C] Ocena kodu.

Question

Cześć.

Piszę zadanie. Treść jest taka:

Wprowadzić ciąg liczb, o wielkoś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.

Mój kod wygląda 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]);
    }
}

Jest to poziom szkolny, więc napiszcie, czy kod przejdzie w szkole i napiszcie ogólnie jakiej jest jakości (jako profesjonalni nauczyciele, a nie ludzie który kiedyś czytali coś o ANSI C)

Ogólnie to nie piszę w C, tylko w C++, więc prosiłbym o wszelkie uwagi. Wiele rzeczy mi się np. kiełbasiło z C++.

Z góry dzięki.

Pozdrawiam. 

Comments

A kod?

---

No przecież jest ;)

---

@Hiskiel, zacznij od pozbycia się ostrzeżeń https://godbolt.org/z/nl9usH

Później troszkę podpowiem.

---

Ogólnie, to dostałem parę wskazówek i kod się zmienił.

Warningi zniwelowane z wyjątkiem tych o nieużywanych zmiennych z main, ale zostały, bo podobno niedobrze jest zostawiać main() pusty.

---

Co ciekawe u mnie kompilator nie wyrzuca warningów przy formatowaniu size_t jako %u

(c11, pedantic, wextra, wall, pedantic-errors)

ale ten z godbolta tak. Za to godbolt nie wyświelta mi warningów przy formatowaniu zu, u mnie tych warningów też nie ma, ale string się nie formatuje, wyświetla się zu/zu

---

Oczywiście mogę odpowiedzieć na tym "innym forum". Daj tylko znać gdzie. Jeśli tu, podaj kod do którego mam się odnieść.

---

Edytowałem kod w pytaniu. Tak, możesz odpowiedź tu. (Btw. nie przeszkadza Ci takie pisanie na TY? Trochę to dla mnie krępujące, ale jeden z moderatorów pisał mi, że tutaj zwracamy się do siebie na TY)

Answer 1

Starałem się nie zmieniać logiki kodu. Uwagi w komentarzach.

#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);

/* Jeśli nie używasz argumentów main(...), to podaj void */
int main(void) {
    size_t size = 0;
    /* Lepsze nazwy to np. divider1 i divider2 a nie "anonimowe" a i b */
    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];
    /* Przy sizeof(...), lepiej stosować idiom pobrania wartości z tablicy.
     * Jeśli w przyszłości zmienisz typ danych, nie będzie konieczności
     * dotykania sizeof(...)
     */
    insertToTable_int(numbers, sizeof(numbers) / sizeof(*numbers));

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

    /* Ta pętla kwalifikuje się do oddzielnej funkcji.
     * Jakieś showTable(...) lub podobna nazwa.
     */
    for(size_t i = 0; i < size; ++i){
        printf("%d ", numbers[i]);
    }
    /* Tu chyba brakuje nowej linii. */
    putchar('\n');

    /* Po co zwalniasz jeśli nie alokujesz na stosie? */
    /* free(numbers); */

    /* Jeśli masz nagłówek stdlib.h, możesz zwrócić EXIT_SUCCESS */
    return EXIT_SUCCESS;
}

/* Lepiej było by przesłać tablicę w postaci wskaźnika, rozmiar, wskaźnik na
 * funkcję wartościującą. To ostatnie to funkcja która przyjmowała by dane z tablicy i
 * zwracała wartość. To by było rozwiązanie uniwersalne. Tylko czy w ramach
 * zajęć były wskaźniki na funkcje?
 */
void replaceIfDivisible_int(int* tab, size_t tab_size, int div_a, int div_b, int new_val){
    /* Przy if'ach, zawsze stosuj blok kodu. Nawet jeśli jest 1
     * instrukcja. Jak szkolnie to szkolnie :)
     * Dodatkowy idiom podawania wartości stałej jako pierwszej.
     * Nie popełnisz błędu = czyli przypisania.
     */
    if(NULL == tab) {
        return;
    }

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

}

/* Trochę mi nie pasują koncepcje tych 2 funkcji poniżej.
 * Po co rozdzielać je na typ wprowadzanych danych?
 * Wystarczy przecież przesłać format i void * na wypełniany typ.
 * Nawet można dodać napisy typu:
 *  const char * prompt - prośba wprowadzenia danych
 *  const char * errMsg - komunikat błędu.
 * Przez to funkcja będzie uniwersalna i .. tylko jedna.
 */

void readInput_int(int* variable){
    /* Wydaje się nieco zgrabniejsze */
    while(0 >= scanf("%d", variable)) {
	fflush(stdin);
    }
}

void readInput_size_t(size_t* variable){
    /* W tej funkcji występuje niebezpieczeństwo
     * mieszania arytmetyki ze znakiem i bez.
     * Dlaczego nie wczytujesz przez scanf(...) typu size_t?
     */
    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 %zu/%zu liczbe do tablicy: ", i + 1, tab_size);
        readInput_int(&tab[i]);
    }
}

 

Comments

Nazwy - według zadania ma być a i b, więc jest..

 

Normalnie to zrobiłbym to z tym void*, tak też wyglądała pierwsza funkcja, ale dla size_t przepuszczało liczby ujemne i był overflow, a potem byłyby problemy, bo tablicę na 4 miliardy ciężko zaalokować.

 

free zostało z poprzedniego kodu, w którym używałem dynamicznej alokacji, ale dowiedziałem się, że mogę użyć VLA (co nie wydawało mi się dobrym rozwiązaniem, ale co ja tam wiem), a o free zapomniałem.

 

Po co rozdzielać jednego for'a do funkcji? Rozumiem, że porządek etc, ale naprawdę nie widzę sensu czegoś co możnaby zapisać w jednej linijce do funkcji. Dalej zaś nie wiem po co po wypisaniu outputu przed końcem należy wypisać line feed.

 

Dalej - replaceIfDivisible

W ramach zajęć nie było nic. Tak naprawdę piszę ten kod dla kogoś innego, dlatego też chcę, żeby jego wartość była jak największa, a przy tym sam się dużo nauczę.

Wskaźniki na funkcję potrafię używać.. sugerujesz mi napisanie czegoś w stylu transform, for_each ..

W jednym z moich problemów (czyt. wątków) napisałeś, żeby robić wszystko jak najprościej.

Więc myślę, że póki nie robię jakiejś biblioteki, która będzie musiała pracować uniwersalnie, a tylko projekt objęty w wąskie ramy wymagań, to myślę, że mogę to zrobić tak, jak jest.

Może gdyby zastosować się do Twojej rady, to nauczyciel tej osoby by się zaskoczył, pochwalił ją etc.

Jak pisałem - zadanie piszę dla kogoś, a kontakt z tą osobą jest bardzo ciężko, więc nie wiem co potrafi i co mieli na zajęciach, a po treści zadania wnioskuję, że są gdzieś na początku... Czyli na szkolnym końcu.

 

A dlaczego nie zmieniałeś logiki kodu? Myślę, że dzięki nie kod może stać się bardziej lub też mniej czysty.

 

 

Gdybyś mógł naprostować wszystkie moje wątpliwości i odpowiedzieć na pytania byłbym bardzo wdzięczny

---

Ok, to co będziesz chciał, "pożyczysz", to co nie będzie Ci pasowało, nie użyjesz. Trudno spekulować co wie a czego nie wie ktoś początkujący. Jeśli masz pytania do kodu odpowiem. Oczywiście starałem się zmaksymalizować ponowne użycie kodu nawet poza granice "produkcyjne".

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

#define BUFFER_SIZE 40

struct table_type {
    void * data;
    size_t size;
    size_t offset;
};

struct transform_args_type {
    int divider1;
    int divider2;
};

void inputValue(const char * format, const char * prompt, const char * errMsg, void * value) {
    int inputStatus;
    do {
        printf("%s", prompt);
        inputStatus = scanf(format, value);
        if(0 == inputStatus) {
            fprintf(stderr, "%s", errMsg);
            char c;
            while((c = getchar()) != '\n' && c != EOF) {}
        } else if(EOF == inputStatus) {
            fprintf(stderr, "Osiągnięto koniec strumienia danych!\n");
            exit(EXIT_FAILURE);
        }
    } while(0 >= inputStatus);
}

void inputTable(struct table_type * table, const char * format) {
    char buff[BUFFER_SIZE];
    char * internalTable = table->data;
    for(size_t i = 0; i < table->size; ++i) {
        snprintf(buff, BUFFER_SIZE, "Podaj %zu/%zu liczbę do tablicy: ", i + 1, table->size);
        inputValue(format, buff, "Zły format danych!\n", (internalTable + table->offset * i));
    }
}

void showTable(struct table_type * table, void (*printer)(void *)) {
    for(size_t i = 0; i < table->size; ++i) {
        printer(((char *)table->data) + i * table->offset);
    }
    putchar('\n');
}

void valuePrinter(void * value) {
    printf("%d ", *((int *)value));
}

void transformTable(struct table_type * table, void (*func)(void *, void *), void * arg) {
    char * internalTable = table->data;
    for(size_t i = 0; i < table->size; ++i) {
        func(internalTable + i * table->offset, arg);
    }
}

void divideCheck(void * value, void * arguments) {
    struct transform_args_type * arg = arguments;
    int * val = (int *)value;
    int mod = *val % arg->divider1 + *val % arg->divider2;
    if(0 == mod) {
        *val = 0;
    }
}

int main(void) {
    struct table_type table = {.data = NULL, .size = 0, .offset = sizeof(int)};
    struct transform_args_type divideArgs = {.divider1 = 0, .divider2 = 0};

    inputValue("%zu", "Wprowadź ilość liczb: ", "Zły format danych!\n", &table.size);
    inputValue("%d", "Wprowadź pierwsza liczbę do sprawdzania podzielności: ", "Zły format danych!\n", &divideArgs.divider1);
    inputValue("%d", "Wprowadź druga liczbę do sprawdzania podzielności: ", "Zły format danych!\n", &divideArgs.divider2);

    table.data = malloc(table.size * sizeof(int));
    if(NULL == table.data) {
        fprintf(stderr, "Błąd alokacji tablicy!\n");
        goto FAIL;
    }

    inputTable(&table, "%d");

    transformTable(&table, divideCheck, &divideArgs);
    showTable(&table, valuePrinter);

    free(table.data);

    return EXIT_SUCCESS;

FAIL:
    return EXIT_FAILURE;
}

PS. Odnosząc się do pytań.

Normalnie to zrobiłbym to z tym void*, tak też wyglądała pierwsza funkcja, ale dla size_t przepuszczało liczby ujemne i był overflow, a potem byłyby problemy, bo tablicę na 4 miliardy ciężko zaalokować.

No to jest to problem wczytywanego formatu a nie typu. Jeśli masz duże tablice, nie ma wyjścia, trzeba alokować na stercie (przypadki dla embedded z alokacją statyczną pominę bo to nie ten kontekst).

free zostało z poprzedniego kodu, w którym używałem dynamicznej alokacji, ale dowiedziałem się, że mogę użyć VLA (co nie wydawało mi się dobrym rozwiązaniem, ale co ja tam wiem), a o free zapomniałem.

j.w. Jeśli tablice duże, alokacja na stercie. BTW, nowy standard C, wręcz określa VLA jako opcjonalne, a w wielu projektach np. dla embedded jest wręcz zabronione. Powody są ale nie tu o tym...

Po co rozdzielać jednego for'a do funkcji? Rozumiem, że porządek etc, ale naprawdę nie widzę sensu czegoś co możnaby zapisać w jednej linijce do funkcji. Dalej zaś nie wiem po co po wypisaniu outputu przed końcem należy wypisać line feed.

Po to masz kod by go czytać i był czytelny. To czy w funkcji jest 1 czy 2 fory, nie ma znaczenia. Znaczenie ma poziom abstrakcji w danym miejscu kodu. Później przyjdą optymalizacje z potencjalnym inline lub zmianą na makro.

W jednym z moich problemów (czyt. wątków) napisałeś, żeby robić wszystko jak najprościej.

I dalej podtrzymuję. Dostałeś zaś kod z kilkoma pomysłami byś sam mógł stwierdzić co ma sens a co nie. Jeszcze raz, nie jestem jasnowidzem co odbiorca kodu zna a czego nie.

A dlaczego nie zmieniałeś logiki kodu? Myślę, że dzięki nie kod może stać się bardziej lub też mniej czysty.

Proszę bardzo... jak widzisz w przykładzie logikę kodu zmieniłem :)

---

Okej super, dzięki bardzo, ale dalej pozostaje mi jedno pytanie - po co to wypisywanie nowej linii?

 

// No i jednak mam pytania do kodu.

1. 

void inputValue(const char * format, const char * prompt, const char * errMsg, void * value) {
    int inputStatus;
    do {
        printf("%s", prompt);
        inputStatus = scanf(format, value);
        if(0 == inputStatus) {
            fprintf(stderr, "%s", errMsg);
            char c;
            while((c = getchar()) != '\n' && c != EOF) {}
        } else if(EOF == inputStatus) {
            fprintf(stderr, "Osiągnięto koniec strumienia danych!\n");
            exit(EXIT_FAILURE);
        }
    } while(0 >= inputStatus);
}

while((c = getchar()) != '\n' && c != EOF) {}

Dlaczego tak, a nie po prostu getchar()? Dlaczego nie może być enter?

} else if(EOF == inputStatus) {
            fprintf(stderr, "Osiągnięto koniec strumienia danych!\n");
            exit(EXIT_FAILURE);
        }

O co z tym chodzi? 

---

Co do nowej linii. Jeśli wypiszesz zawartość tablicy, następne dane będziesz chciał mieć w nowej linii. Stąd najprawdopodobniej chcesz do niej przejść. Wyświetl coś po wyprowadzeniu danych z tablicy a wszystko Ci się wyjaśni.

Czytanie scanf(...), może się nie powieść z wielu powodów. Jednym z nich jest wprowadzenie innego typu danych niż oczekiwany (np. prosisz o wartość int a dostajesz napis "ala ma chomika"). Wtedy scanf(...) zwróci 0 jako informację ile bajtów wczytał. scanf więc nie przeczytał danych ale pozostały one w buforze. Trzeba je konsumować aż do znaku nowej linii lub końca danych. To jest oficjalnie polecany sposób opróżnienia bufora.

Drugim powodem załamania scanf(...) jest zakończenie strumienia. Dla GNU/Linux to będzie <ctrl + d> na konsoli a dla MS Windows <ctrl + z> na konsoli cmd. Taki znak dostajesz jeśli czytasz z pliku i oznacza on koniec tego pliku. Ten błąd także należy wykryć.

Inne błędy scanf(...) to wartości ujemne. Stąd do {... } while(...); wykonuje się aż status będzie dodatni.

To prosty program więc nie wiadomo co zrobić jak się wywróci scanf(...). Stąd wykonuję exit(...) i załamuję program.

W kodzie jest jeszcze użycie goto. W przypadku alokacji zasobu lub wieloetapowego inicjowania, często używa się tego idiomu. C (bez plusów) nie ma RAII. Stąd taki idiom.