Code review - Cs50 Pset 1

Odpowiedz Nowy wątek
2019-06-29 15:16
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;
}
za mało się dzieje żeby mówić o code review, same ify i garstka obliczeń - au7h 2019-06-30 00:19

Pozostało 580 znaków

2019-06-30 00:49
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.


Jeśli chcesz pomocy, NIE pisz na priva, ale zadaj dobre pytanie na forum.
edytowany 4x, ostatnio: MarekR22, 2019-06-30 18:25
często są mniej zrozumiałe niż kod WtF? to pisz je dobrze i faktycznie tam, gdzie wypadałoby. Komentarz ma również za zadanie wyjaśnić logikę biznesową stojącą za linijką kodu, która może być bardzo niejasna przy różnych edge caseach lub po prostu rozjaśnia triki/hacki itd. - WeiXiao 2019-06-30 01:20
to nie jest moje stwierdzenie, ale coś co jest opisane w książce "Czysty kod" R.C. Martin, gościa który zaczyna karierę w 1970 roku więc ma troszkę doświadczenia i troszkę jest znany w branży. - MarekR22 2019-06-30 13:48
język potoczny nie jest jednoznaczny , nazwa funkcji będzie opisem - czyli ten opis też nie będzie jednoznaczny, a argument "mój guru tak mówi więc to prawda" słaby jest - Miang 2019-06-30 15:58
Języki programowania są sformalizowane przez co są jednoznaczne i nie chodzi tu o nazwy funkcji. Zamiast wysuwać puste argumenty polecam książkę lub filmiki od Uncle Bob (w internecie jest trochę darmowych, ale te płatne są dużo bardziej obszerne: ~26h). Ja książki nie zamierzam tu przepisywać, ani nie mam ochoty na kolejną g..oburzę. - MarekR22 2019-06-30 18:24

Pozostało 580 znaków

2019-06-30 01:16
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

edytowany 1x, ostatnio: au7h, 2019-06-30 01:17
...i jeszcze ta zmienna l :D - Mc_Hammer 2019-06-30 01:31
teoria ciekawa, szkoda, że niezgodna z faktami. Pasuje do innego cytatu: I’m basically a very lazy person who likes to get credit for things other people actually do.. - MarekR22 2019-06-30 13:53
nie zmienia to faktu, że ludzie którzy nad tym pracowali to także doświadczeni programiści, którzy przecież tworzą kod dla milionów ludzi, a takich kawałków jest zapewne więcej. Pojęcie "czystego kodu" istnieje tylko w teorii, w praktyce to czego nie umie się napisać czysto, pisze się tylko tak, byleby działało - au7h 2019-06-30 14:33
Nawet jeśli to prawda (nie jest to pewne). No i co z tego? Teraz pracuję z gościem, który ma 10 lat doświadczania więcej od mnie, chwali się, że pracował w MS, a pisze straszny crap code (za to menadżerem projektu jest świetnym). Są osoby odporne na rzeczowe argumenty jak powinien wyglądać dobry kod i nieważne że pracują +30 lat skoro z powodu swego uporu nic nie zmieni ich podejścia. Zły specjalista nie chcący się doskonalić pozostanie złym specjalistą nawet jeśli zdobędzie 40 lat doświadczenia.. - MarekR22 2019-06-30 14:40
Wniosek z tego taki, że człowiek nie robot, brakłoby ci życia na poznanie wszystkich technologii + frameworków łącznie z zasadami clean code. :) - au7h 2019-06-30 15:46

Pozostało 580 znaków

2019-07-01 14:06
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)

"Sugeruję wyobrazić sobie Słońce widziane z orbity Merkurego, a następnie dupę tej wielkości. W takiej właśnie dupie specjalista ma teksty o wspaniałej atmosferze, pracy pełnej wyzwań i tworzeniu innowacyjnych rozwiązań. Pracuje się po to, żeby zarabiać, a z resztą specjalista sobie poradzi we własnym zakresie, nawet jeśli firma mieści się w okopie na granicy obu Korei."
-somekind,
konkretny człowiek-konkretny przekaz :]
edytowany 6x, ostatnio: MasterBLB, 2019-07-02 07:15
Jeśli WrongLength to argumentem nie może być long ale raczej const char * - MarekR22 2019-07-01 14:20
A tak, zapomniałem dodać sugestię, że numer karty to lepiej jako łańcuch podawać - MasterBLB 2019-07-01 14:34

Pozostało 580 znaków

2019-07-01 15:33
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

edytowany 3x, ostatnio: Ikszad, 2019-07-01 15:35
Nawiasowanie to akurat chwaliłem, że jest zrobione porządnie. - MasterBLB 2019-07-02 07:00

Pozostało 580 znaków

2019-07-02 14:46
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.


"Sugeruję wyobrazić sobie Słońce widziane z orbity Merkurego, a następnie dupę tej wielkości. W takiej właśnie dupie specjalista ma teksty o wspaniałej atmosferze, pracy pełnej wyzwań i tworzeniu innowacyjnych rozwiązań. Pracuje się po to, żeby zarabiać, a z resztą specjalista sobie poradzi we własnym zakresie, nawet jeśli firma mieści się w okopie na granicy obu Korei."
-somekind,
konkretny człowiek-konkretny przekaz :]
edytowany 1x, ostatnio: MasterBLB, 2019-07-02 14:46

Pozostało 580 znaków

2019-07-02 15:13
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.

edytowany 1x, ostatnio: Ikszad, 2019-07-02 15:14

Pozostało 580 znaków

2019-07-02 17:46
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;
}

"Sugeruję wyobrazić sobie Słońce widziane z orbity Merkurego, a następnie dupę tej wielkości. W takiej właśnie dupie specjalista ma teksty o wspaniałej atmosferze, pracy pełnej wyzwań i tworzeniu innowacyjnych rozwiązań. Pracuje się po to, żeby zarabiać, a z resztą specjalista sobie poradzi we własnym zakresie, nawet jeśli firma mieści się w okopie na granicy obu Korei."
-somekind,
konkretny człowiek-konkretny przekaz :]
edytowany 3x, ostatnio: MasterBLB, 2019-07-03 10:04

Pozostało 580 znaków

2019-07-02 19:14
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?

Pozostało 580 znaków

2019-07-02 19:36
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';

"Sugeruję wyobrazić sobie Słońce widziane z orbity Merkurego, a następnie dupę tej wielkości. W takiej właśnie dupie specjalista ma teksty o wspaniałej atmosferze, pracy pełnej wyzwań i tworzeniu innowacyjnych rozwiązań. Pracuje się po to, żeby zarabiać, a z resztą specjalista sobie poradzi we własnym zakresie, nawet jeśli firma mieści się w okopie na granicy obu Korei."
-somekind,
konkretny człowiek-konkretny przekaz :]
edytowany 1x, ostatnio: MasterBLB, 2019-07-02 19:54

Pozostało 580 znaków

2019-07-07 14:24
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.

edytowany 3x, ostatnio: Ikszad, 2019-07-07 14:39

Pozostało 580 znaków

Odpowiedz
Liczba odpowiedzi na stronę

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