Code review - Cs50 Pset 1

0

Cześć,
Rozpocząłem naukę programowania z Cs50. Udało mi się zrobić zadania z pset1 i chciałbym prosić o mały code review. Nie ma dużo kodu, ale już od początku chciałbym wiedzieć gdzie popełniam błędy i je niwelować. Pozdrawiam!
Rysuje drzewko - ala przeszkoda w Mario

#include <cs50.h>
#include <stdio.h>

int main(void)
{
int height;
    do
    {
        height = get_int("Height: ");
    }
    while (height < 1 || height > 8);
    for(int i = 0; i < height; i++) // Creates "height" number of verses
    {
        for(int a = 1; a <= height; a++) //Creates first part of columns
        {
            if (a >= height - i)
            {
                printf("#");
            }
            else
            {
                printf(" ");
            }
        }
        printf("  "); // Two spaces dividing columns
        for(int b = height; b >= 1; b--) //Creates second part of columns
        {
            if (b >= height - i)
            {
                printf("#");
            }
        }
        printf("\n");
    }
    
}

Sprawdza poprawność numeru karty kredytowej i wypisuje jej typ:

/* Asks user for a Credit card number, validaes if the number is correct according to
 Luhn's algorithm, and prints if this is a American Express card, MasterCard or VISA */

#include <stdio.h>
#include <cs50.h>

int validate(long number);
long get_number(void);
void exist(long number);

int valid = 0;

int main(void)
{
    long number = get_number();
    exist(number);
}
long get_number(void)
{
// Get credit card number from user
    long number = get_long("Number: ");
    return number;
}
void exist(long number)
    // Checks the type of a card and calls validate function
{
    if((number >= 340000000000000 && number < 350000000000000) || (number >= 370000000000000 && number < 380000000000000))
    {
        string type = "AMEX\n";
        if (validate(number))
        {
            printf("%s",type);
        }
        else 
        {
            printf("INVALID\n");
        }
    }
    else if (number >= 5100000000000000 && number < 5600000000000000)
    {
        string type = "MASTERCARD\n";
        if (validate(number))
        {
            printf("%s",type);
        } 
         else 
        {
            printf("INVALID\n");
        }
    }
    else if ((number >= 4000000000000 && number < 5000000000000) || (number >= 4000000000000000 && number < 5000000000000000))
    {
        string type = "VISA\n";
        if (validate(number))
        {
            printf("%s",type);
        }
         else 
        {
            printf("INVALID\n");
        }
    }
    else
    {
        string type = "INVALID\n";
        printf("%s",type);
    }
}
int validate(long number)
    // Validates if the credit card number is correct according to Luhn's algorithm
{
    int sum = 0;
    int sum_2 =0;
      for (long divide = 10; divide <= number * 10; divide *= 100)
// Multiply every other digit by 2, starting with the number’s second-to-last digit, and then add those products’ digits together.
    {
        long num_1 = number / divide;
        int other = num_1 % 10;         // Geting the digit
        int other_x2 = other * 2;        // Digit * 2 
        sum = sum + other_x2 / 10 + other_x2 % 10; //Sum of digits in number
    }
    for (long divide = 1; divide <= number * 10; divide *= 100)
// Add the sum to the sum of the digits that weren’t multiplied by 2.
    {
        long num_2 = number / divide;
        int otherNotDivided = num_2 % 10; 
        sum_2 += otherNotDivided;
    }
    int validation_sum = sum + sum_2;
    if (validation_sum % 10 == 0)
    {
       valid = 1;
    }
    return valid;
}
4

jak widzę funkcję, która ma więcej niż dwie instrukcje sterujące (if/for/while/switch) to już wiem, że nie jest to dobry kod.
Więcej niż 4 oznacza wielki ból głowy od samego patrzenia.
Inna forma tej zasady: każda funkcja która nie mieści się w ~10 linijkach jest trudna w zrozumieniu.
Liczby magiczne.

To są proste podstawowe zasady., łatwe w zrozumieniu i przestrzeganiu, a mimo to ludzi mają problemy z ich przestrzeganiem, a wręcz czasem bronią się przed nimi rękami i nogami.

Czyli podziel to na mniejsze funkcje.
Pozbądź się magicznych liczb (np 5100000000000000), 10 powinno zostać na swoim miejscu.

Zmienne globalne to samo zło i są zupełnie zbędne dla tego problemu.

Cytując klasyków (na dowód, że to nie moje wymysły):

Linus Torvalds:

Linus Torvalds
Jeśli potrzebujesz więcej niż trzech poziomów wcięcia, twój kod i tak jest schrzaniony i powinieneś go poprawić.
If you need more than 3 levels of indentation, you’re screwed anyway, and should fix your program."

Robert C Martin (AKA Unclue Bob)

Czysty kod czyta się jak dobrze napisaną powieść
Clean code reads like well-written prose.

Na "Clean code" jeszcze dla ciebie na pewno za wcześnie (trzeba dużo powodować, żeby zrozumieć rzeczowość tej książki)

A jeszcze jak zaczniesz pisać małe funkcje to zrozumiesz, że w takim przypadku komentarze są w zasadzie zbędne.
Każda nazwa funkcji będzie małym krótkim opisem co się dzieje.
Wady komentarzy:

  • mało kto je poprawia jak zmieni kod, więc często kłamią!
  • często są mniej zrozumiałe niż kod
  • język potoczny nie jest jednoznaczny - a kod jest jednoznaczny, więc czytelny kod jest wart 100 razy więcej niż komentarz.

Nazwy funkcji są zbyt ogólne.
W C nie ma przestrzeni nazw, więc trzeba stosować prefixy oraz nadawać bardziej szczegółowe nazwy.

Nie mieszaj UI z logiką.
Przykładowo exist (po nazwie nie wiem co ma to robić i czym się różni od validate), powinno zwracać wartość zamiast drukować na standardowe wyjście.

1
MarekR22 napisał(a):

jak widzę funkcję, która ma więcej niż dwie instrukcje sterujące (if/for/while/switch) to już wiem, że nie jest to dobry kod.

static int
acpi_video_get_next_level(struct acpi_video_device *device,
			  u32 level_current, u32 event)
{
	int min, max, min_above, max_below, i, l, delta = 255;
	max = max_below = 0;
	min = min_above = 255;
	/* Find closest level to level_current */
	for (i = ACPI_VIDEO_FIRST_LEVEL; i < device->brightness->count; i++) {
		l = device->brightness->levels[i];
		if (abs(l - level_current) < abs(delta)) {
			delta = l - level_current;
			if (!delta)
				break;
		}
	}
	/* Ajust level_current to closest available level */
	level_current += delta;
	for (i = ACPI_VIDEO_FIRST_LEVEL; i < device->brightness->count; i++) {
		l = device->brightness->levels[i];
		if (l < min)
			min = l;
		if (l > max)
			max = l;
		if (l < min_above && l > level_current)
			min_above = l;
		if (l > max_below && l < level_current)
			max_below = l;
	}

	switch (event) {
	case ACPI_VIDEO_NOTIFY_CYCLE_BRIGHTNESS:
		return (level_current < max) ? min_above : min;
	case ACPI_VIDEO_NOTIFY_INC_BRIGHTNESS:
		return (level_current < max) ? min_above : max;
	case ACPI_VIDEO_NOTIFY_DEC_BRIGHTNESS:
		return (level_current > min) ? max_below : min;
	case ACPI_VIDEO_NOTIFY_ZERO_BRIGHTNESS:
	case ACPI_VIDEO_NOTIFY_DISPLAY_OFF:
		return 0;
	default:
		return level_current;
	}
}

Torvalds mówi swoje, a robi też swoje

4

Cóż Bracie Ikszard, to tak:

  • kolorowanie składni na forum typowe dla danego języka uzyskasz poprzez sufiks c albo cpp po tych trzech `
  • porządne nawiasowanie, nie jak zakamuflowane, dżawowskie degeneraty
  • wskazane byłoby oddzielać nową linią fragmenty kodu mające inną logikę. W przypadku pierwszego programu są to pętla pobierająca height oraz część rysująca choinkę
  • przy interakcji z użytkownikiem wskazane jest informowanie go, jakich akcji/danych od niego się spodziewasz, oraz z jakiego zakresu. I tak, dla programu nr1 prawidłowy sposób pobrania wielkości wyglądałby tak:
    do
    {
        height = get_int("Podaj wysokość choinki (od 1 do 8): ");
    }
  • walidacja kart płatniczych całkiem źle jest zrobiona, a akurat w tym temacie siedzę bo pracuję przy oprogramowaniu i testowaniu terminali płatniczych :] Prawilnie karty rozpoznaje się po czymś, co się nazywa BIN, a tutaj masz ich listę. Generalnie, jest to lista prefiksów od jakich się zaczyna numer karty płatniczej danego wydawcy. Poza tym łatwiej będzie to sprawdzić na łańcuchu znaków nie na liczbie.
  • używanie zmiennej globalnej do przekazywania stanu walidacji to proszenie się o kłopoty. Na razie to bez problemu ogarniasz, ale poczujesz ten ból jak funkcji mających dostęp do takiej danej i ją modyfikujących będzie kilkanaście/kilkadziesiąt :]
  • nie do końca poprawny prototyp funkcji int validate(long number). Zastanów się, jaki rodzaj informacji zwrotnej interesowałby użytkownika - w końcu odkryjesz, że są w zasadzie tylko dwa przypadki:
    a) użytkownik oczekuje prostej odpowiedzi "jest poprawny" albo "nie jest poprawny", wtedy wystarczy bool validate(const char *cardNumber)
    b) użytkownik oczekuje informacji o tym, co poszło nie tak. Wtedy prototyp mógłby wyglądać następująco:
enum PANValidationResult
{
    Ok = 0,
    WrongLength = 1,
    NonExistingBIN = 2,
    WrongLuhn = 4
};
//i prototyp
PANValidationResult validate(const char *cardNumber)
0

Dzięki za rady, program poprawię według wskazówek i wrzucę jeszcze raz.
Wyjaśnię tylko czym kierowałem się w tych "magicznych liczbach" i walidacji.

walidacja kart płatniczych całkiem źle jest zrobiona, a akurat w tym temacie siedzę bo pracuję przy oprogramowaniu i testowaniu terminali płatniczych :] Prawilnie karty rozpoznaje się po czymś, co się nazywa BIN, a tutaj masz ich listę. Generalnie, jest to lista prefiksów od jakich się zaczyna numer karty płatniczej danego wydawcy. Poza tym łatwiej będzie to sprawdzić na łańcuchu znaków nie na liczbie.

Robiąc zadanie nie miałem jeszcze wprowadzonych list więc nie mogłem (lub nie znałem odpowiedniej metody) analizować nr karty jako ciąg znaków stąd traktowanie ich jako liczby. A rozpoznawanie prefiksów o których mówisz jeśli dobrze rozumiem mam zaimplementowane właśnie przez te "magiczne liczby" z tym, że listę prefiksów i długości numerów poszczególnych rodzajów kart miałem zdefiniowaną w treści zadania więc może być niepełna.

porządne nawiasowanie, nie jak zakamuflowane, dżawowskie degeneraty

Mógłbyś sprecyzować?

Podsumowując:
-Podział na małe konkretne funkcje
-Ogarnięcie nazw funkcji
-Wywalenie zmiennych globalnych
-Poprawienie logiki
-Postaram się zmodyfikować program by używał list i ciągu znaków

2
Ikszad napisał(a):

Dzięki za rady, program poprawię według wskazówek i wrzucę jeszcze raz.
Wyjaśnię tylko czym kierowałem się w tych "magicznych liczbach" i walidacji.

walidacja kart płatniczych całkiem źle jest zrobiona, a akurat w tym temacie siedzę bo pracuję przy oprogramowaniu i testowaniu terminali płatniczych :] Prawilnie karty rozpoznaje się po czymś, co się nazywa BIN, a tutaj masz ich listę. Generalnie, jest to lista prefiksów od jakich się zaczyna numer karty płatniczej danego wydawcy. Poza tym łatwiej będzie to sprawdzić na łańcuchu znaków nie na liczbie.

Robiąc zadanie nie miałem jeszcze wprowadzonych list więc nie mogłem (lub nie znałem odpowiedniej metody) analizować nr karty jako ciąg znaków stąd traktowanie ich jako liczby. A rozpoznawanie prefiksów o których mówisz jeśli dobrze rozumiem mam zaimplementowane właśnie przez te "magiczne liczby" z tym, że listę prefiksów i długości numerów poszczególnych rodzajów kart miałem zdefiniowaną w treści zadania więc może być niepełna.

Mając taki kod jak zaprezentowałeś polegniesz z kretesem:

    else if (number >= 5100000000000000 && number < 5600000000000000)
    {
        string type = "MASTERCARD\n";

a teraz zajrzyjmy do linka, co tam mamy:

  • BINy dla Mastercarda to 2221-2720, 51-55
  • ALE zarazem BINy dla Diners Club United States & Canada to 54, 55 więc się załapią w ten warunek co podałeś, tym bardziej, że i długość w obu wypadkach to 16 znaków

Sprawa jest tym poważniejsza, że w przypadku płatności kartą jest od groma organizacji płatniczych które ciągle przeprowadzają jakieś kontrole czy audyty, i w tej branży w razie pomyłki skutkującej np. defraudacją z powodu złego rozpoznania karty można trafić do ciupy. Więc sugeruję naprawdę się do implementacji przyłożyć.
Pamiętaj poza tym, że to jest tabelka pierwsza z brzegu jaką mi gógle pokazały, upewnij się, że korzystasz z aktualnej.

0

BINy dla Mastercarda to 2221-2720, 51-55
ALE zarazem BINy dla Diners Club United States & Canada to 54, 55 więc się załapią w ten warunek co podałeś, tym bardziej, że i długość w obu wypadkach to 16 znaków

Rozumiem Cię całkowicie, ale muszę się wytłumaczyć. W zadaniu nie miałem rozróżniać BINów Diners Club United States & Canada itd. ani korzystać z żadnych tabel prezentujących aktualne BINy. Po prostu w uproszczeniu miałem określone:
American Express - nr zaczyna się od 34 lub 37, Ilość cyfr - 15
Master Card - zaczyna się od 51,52,53,54,55, ilość cyfr - 16
Visa zaczyna się od 4, ilość cyfr - 13 lub 16
Więc nie musiałem zakładać, że wystąpi sytuacja zaprezentowana przez Ciebie, bo moje warunki to wykluczały.
Pod wymagania zadania (oraz aktualną na tamtą chwile wiedzą) tworzyłem więc rozwiązanie problemu i jestem świadom, że jeśli miałoby to funkcjonować poważnie to wszystko jest do rozbudowy i przebudowy uwzględniając te wszystkie wspomniane BINy. W sumie to jest fajny pomysł, żeby popracować w własnym zakresie nad dopracowaniem aplikacji.

1

Ok, skoro tak to działaj według wytycznych z zadania.
Natomiast jak porządnie przeprowadzić walidację karty, oto parę wskazówek

//żeby używać łańcucha nie longa to już wiesz. Ale w kwestii rozbicia walidacji na funkcje to proponuję tak:
enum Issuer
{
    Unknown = 0,
    Visa = 1,
    MasterCard = 2,
    AmericanExpress = 3
};

//to ma za zadanie zwrócić odpowiednią wartość enuma Issuer, albo Unknown jak nie dopasuje żadnego
Issuer recognizeIssuer(const char *cardNumber)
{
     return Issuer::coś tam;
}

//to ma sprawdzić długość w zależności od wykrytego wydawcy kart
PANValidationResult checkLength(Issuer issuer, const char *cardNumber)
{
      return PANValidationResult::Ok;
      //albo
      return PANValidationResult::WrongLength;
}

//tego parametru issuer nie jestem pewien czy potrzebujesz w twojej uproszczonej wersji. Jeśli okaże się, że nie to wystarczy sam numer karty
PANValidationResult checkLuhn(Issuer issuer, const char *cardNumber)
{
       return PANValidationResult::Ok;
       // albo
       return PANValidationResult::WrongLuhn;
}

//i teraz twoja główna funkcja walidująca (w wersji bogatszej, informującej na czym walidacja poległa) będzie wyglądać tak:
PANValidationResult validateCardNumber(const char *cardNumber)
{
    Issuer issuer = recognizeIssuer(cardNumber);
    if (issuer == Issuer::Unknown)
    {
        return PANValidationResult::NonExistingBIN ;
    }

    PANValidationResult result = checkLength(issuer, cardNumber)l
    if (result == PANValidationResult::Ok)
    {
         result = checkLuhn(issuer, cardNumber);
    }

    return result;
}
0

Przeanalizowałem Twój kod i myślę, że mniej więcej kumam Twoje rozwiązanie, ale mam pewną wątpliwość.
Skoro używam numeru karty poprzez

const char *cardNumber

To jak wykonać te wszystkie matematyczne obliczenia w algorytmie Luhna? Konwertować jakoś poszczególne cyfry na int?

1
Ikszad napisał(a):

Przeanalizowałem Twój kod i myślę, że mniej więcej kumam Twoje rozwiązanie, ale mam pewną wątpliwość.
Skoro używam numeru karty poprzez

const char *cardNumber

To jak wykonać te wszystkie matematyczne obliczenia w algorytmie Luhna? Konwertować jakoś poszczególne cyfry na int?

Tak, przy czym nawet niekoniecznie na int - wystarczy taka prosta sztuczka aby znak cyfry zmienić na wartość liczbową:

char digit = characterFromCardNumber - '0';
0

Cześć, popracowałem nad kodem więc prosiłbym o ocenę jeszcze raz. Mam wrażenie, że teraz dużo więcej się tam dzieje.

/* Asks user for a Credit card number, validaes if the number is corrrct
   and prints if this is a American Express card, MasterCard or VISA.
   If the number incorrect prints for what reason.*/

#include <stdio.h>
#include <cs50.h>                              
#include <string.h>
#include <stdint.h>

enum cardTypes
{
    Unknown = 0,
    Amex = 1,
    MasterCard = 2,
    Visa = 3    
};
enum validationResult
{
    OK = 0,
    wrongPrefix = 1,
    wrongLength = 2,
    wrongLuhn = 3

};

char *getCardNumber(void);
enum validationResult validateCard(char *cardNumber, enum cardTypes *cardType);
enum cardTypes checkCardPrefix(char *cardNumber);
enum validationResult checkLength(char *cardNumber,enum cardTypes *cardType);
enum validationResult checkLuhn(char *cardNumber);
int calculateLuhnSum(char *cardNumber);
void printValidationError(enum validationResult result);
void printCardType(enum cardTypes *cardType);

int main(void)
{
    enum cardTypes cardType;
    
    char *cardNumber = getCardNumber();
    enum validationResult result = validateCard(cardNumber, &cardType);
    if(result == OK)
    {
        printCardType(&cardType);
    }
    else
    {
        printValidationError(result);
    }
  
}

char *getCardNumber(void)
{
    char *cardNumber = get_string("Type credit card number: \n");
    return cardNumber;
}

// Check card BIN, length, and Luhn sum, return result
enum validationResult validateCard(char *cardNumber, enum cardTypes *cardType)
{
    *cardType = checkCardPrefix(cardNumber); //Defined in main
    if (*cardType == Unknown)
    {
        return wrongPrefix;
    }
    
    enum validationResult result = checkLength(cardNumber, cardType);
    if (result == OK)
    {
        result = checkLuhn(cardNumber);
    }
    
    return result;    
}

enum cardTypes checkCardPrefix(char *cardNumber)
{
    char *amexPrefixes[2] = {"34","37"};
    char *masterCardPrefixes[5] = {"51", "52", "53","54", "55"};
    char *visaPrefix = "4";
        
    enum cardTypes type;
    
    for (int i = 0; i < 2; i++)
    {
        if(strncmp(amexPrefixes[i], cardNumber, 2) == 0)
        {
            type = Amex;
            return type;
        }
    }        
    for(int i = 0; i < 5; i++)
    {
        if(strncmp(masterCardPrefixes[i], cardNumber, 2) == 0)
        {
            type = MasterCard;       
            return type;  
        }
    }
   
    if(strncmp(visaPrefix, cardNumber, 1) == 0)
    {
        type = Visa;
        return type;
    }
    else
    {
        type = Unknown;
        return type;
    }
}

enum validationResult checkLength(char *cardNumber, enum cardTypes *cardType)
{
    uintptr_t type = *cardType;
    switch(type)
    {
        case 0: 
        break; 
        
        case 1:
        if (strlen(cardNumber) == 15)
        {
            return OK;
        }
        else break;
        
        case 2:
        if(strlen(cardNumber) == 16)
        {
            return OK;
        }
        else break;
        
        case 3:
        if(strlen(cardNumber) == 13 || strlen(cardNumber) == 16)
        {
            return OK;
        }
        else break;
    }
    return wrongLength;
}

enum validationResult checkLuhn(char *cardNumber)
{
    
    int luhnSum = calculateLuhnSum(cardNumber);
    if(luhnSum % 10 == 0)
    {
        return OK;
    }
    else
    {
        return wrongLuhn;
    }
}

int calculateLuhnSum(char *cardNumber)
{
    int sum = 0;
    bool flag = true;
    
    for(long i = strlen(cardNumber) - 1; i >= 0; i-- ) 
    {
        char digit = cardNumber[i] - '0';
        
        if(flag)
        {
            sum += digit; 
            flag = !flag;
        }
        else
        {
            int digit_x2 = digit * 2;
            sum += digit_x2 / 10 + digit_x2 % 10;
            flag = !flag;
        }
          
    }  
    return sum;
}

void printCardType(enum cardTypes *cardType)
{
    uintptr_t type = *cardType;
    switch(type)
    {
        case 0:
        case 1:
        printf("American Express\n");
        break;
        case 2:
        printf("MasterCard\n");
        break;
        case 3:
        printf("Visa\n");
        break;
        
    }
}

void printValidationError(enum validationResult result)
{
    switch(result)
    {
        case 0:
        break;
        case 1:
        printf("Wrong BIN\n");
        break;
        case 2:
        printf("Wrong number length\n");
        break;
        case 3:
        printf("Wrong Luhn sum\n");
        break;
    }
}

=Tak, przy czym nawet niekoniecznie na int - wystarczy taka prosta sztuczka aby znak cyfry zmienić na wartość liczbową:
char digit = characterFromCardNumber - '0';

Mógłbyś wyjaśnić w jaki sposób to działa?

Co do samego kodu to problem sprawiło mi ogarnięcie jak do zaproponowanej logiki wpleść wypisanie typu karty, bo zmienna cardType była definiowana w funkcji walidującej i po powrocie do main nie miałem już do niej dostępu, a nie chciałem używać odradzanych przez was zmiennych globalnych. Wpadłem na rozwiązanie przekazania cardType do funkcji przez wskaźnik, ale nie wiem czy to dobre rozwiązanie.

2
Ikszad napisał(a):

=Tak, przy czym nawet niekoniecznie na int - wystarczy taka prosta sztuczka aby znak cyfry zmienić na wartość liczbową:
char digit = characterFromCardNumber - '0';

Mógłbyś wyjaśnić w jaki sposób to działa?

Według tablicy ASCII widoczne dla człowieka znaki cyfr mają kody od 0x30 (taki kod ma 0) do 0x39(kod dla 9). Zatem jeśli dany char przechowuje cyfrę, to odejmując od niego 0x30 otrzyma się w rezultacie liczbę z przedziału 0-9. Jeśli otrzymana wartość jest mniejsza, lub większa to oznacza, że ten znak nie był cyfrą.
A to można zapisać prościej w sposób, jaki podałem powyżej.

Ikszad napisał(a):

Co do samego kodu to problem sprawiło mi ogarnięcie jak do zaproponowanej logiki wpleść wypisanie typu karty, bo zmienna cardType była definiowana w funkcji walidującej i po powrocie do main nie miałem już do niej dostępu, a nie chciałem używać odradzanych przez was zmiennych globalnych. Wpadłem na rozwiązanie przekazania cardType do funkcji przez wskaźnik, ale nie wiem czy to dobre rozwiązanie.

Takie rozwiązanie jest standardem w C, więc dobrze wykoncypowałeś. Jedyne o czym nie wiedziałeś, to to iż zmienną cardType wypada zainicjalizować na Unknown przed wysłaniem do funkcji - może się zdarzyć, iż wewnątrz funkcji do której przekazujesz tą zmienną nie zostanie ona zmodyfikowana, i pozostanie w takim stanie niezdefiniowanym, na czym można się grubo przejechać. A tak, jak na dzień dobry będzie Unknown, to jeśli nic ją w funkcji nie zmodyfikuje to nadal pozostanie Unknown, a Ty Bracie będziesz miał pewność co do jej stanu.

A teraz, co do kodu

Ikszad napisał(a):
enum validationResult checkLength(char *cardNumber, enum cardTypes *cardType)
{
   uintptr_t type = *cardType;
   switch(type)
   {
       case 0: 
       break; 

       case 1:
       if (strlen(cardNumber) == 15)
       {
           return OK;
       }
       else break;

       case 2:
       if(strlen(cardNumber) == 16)
       {
           return OK;
       }
       else break;

       case 3:
       if(strlen(cardNumber) == 13 || strlen(cardNumber) == 16)
       {
           return OK;
       }
       else break;
   }
   return wrongLength;
}

Tutaj zamiast case 0, 1, itd możesz, a nawet powinieneś użyć wartości enuma. Poza tym nie ma sensu wywoływać X razy strlen, skoro można raz przed switchem. Kolejna uwaga to else break jest nadmiarowe, starczy sam break. Poza tym, czemu *cardType jako wskaźnik? Można wprawdzie, ale to jest komplikowanie sobie życia. I ostatnia uwaga, w enumach warto dawać sekcję default, dzięki czemu na przyszłość, jak przybędzie w enumie wartości to nadal kod da radę to obsłużyć. Jako przykład wziąłem funkcję checkLenght(), ale uwagi dotyczą wszystkich funkcji gdzie używasz switcha.

enum validationResult checkLength(char *cardNumber, enum cardTypes cardType)
{
    int length = strlen(cardNumber);    
    switch(cardType)
    {
        case Amex:
        if (length == 15)
        {
            return OK;
        }        
        break;

        case MasterCard:
        if (length == 16)
        {
            return OK;
        }
        break;

        case Visa:
        if (length == 13 || length == 16)
        {
            return OK;
        }
        break;

        default:
        ;
    }
    return wrongLength;
}

W checkCardPrefix zamiast

        if(strncmp(amexPrefixes[i], cardNumber, 2) == 0)
        {
            type = Amex;
            return type;
        }

wystarczyłoby po prostu

        if(strncmp(amexPrefixes[i], cardNumber, 2) == 0)
        {
            return Amex;
        }

Ale generalnie widać znaczną poprawę i fakt, iż zaczynasz czaić o co w tym całym programowaniu chodzi.

0

Takie rozwiązanie jest standardem w C, więc dobrze wykoncypowałeś. Jedyne o czym nie wiedziałeś, to to iż zmienną cardType wypada zainicjalizować na Unknown przed wysłaniem do funkcji - może się zdarzyć, iż wewnątrz funkcji do której przekazujesz tą zmienną nie zostanie ona zmodyfikowana, i pozostanie w takim stanie niezdefiniowanym, na czym można się grubo przejechać. A tak, jak na dzień dobry będzie Unknown, to jeśli nic ją w funkcji nie zmodyfikuje to nadal pozostanie Unknown, a Ty Bracie będziesz miał pewność co do jej stanu.

Początkowo tak miałem zdefiniowaną, ale uznałem, że skoro funkcja i tak nada wartość Unknown to nie ma takiej potrzeby, ale już w kodzie poprawione!

Poza tym, czemu *cardType jako wskaźnik? Można wprawdzie, ale to jest komplikowanie sobie życia.

Wynikało to z czystej niewiedzy. To moje pierwsze spotkanie ze wskaźnikami i pewne kwestie się mieszają, ale sytuację ogarnąłem następująco:


*cardType = checkCardPrefix(cardNumber);
enum cardTypes type = *cardType;

enum validationResult checkLength(char *cardNumber, enum cardTypes type)
{
    int numberLength = strlen(cardNumber);
    switch(type)
    {
        case Unknown: 
        break; 
        
        case Amex:
        if (numberLength == 15)
        {
            return OK;
        }
        break;
        
        case MasterCard:
        if(numberLength == 16)
        {
            return OK;
        }
        break;
        
        case Visa:
        if(numberLength == 13 || numberLength == 16)
        {
            return OK;
        }
        break;
        
        default:
        ;
    }
    return wrongLength; 
}

Generalnie ogromne dzięki za poświęcony czas. Samo zadanie sporo mnie nauczyło, ale to głównie dzięki waszym radom czuję się lepszy niż byłem. Będę wpadał pewnie jeszcze z kolejnymi zadaniami. :)

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