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-07-08 07:25
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.


"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-08 07:30

Pozostało 580 znaków

2019-07-08 11:58
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. :)

edytowany 2x, ostatnio: Ikszad, 2019-07-08 12:01
To pyknij ptaszka pod tym moim postem co żeś odklikał, dzięki czemu inni użytkownicy forum będą widzieli, że problem rozwiązany. No i zapraszamy serdecznie z kolejnymi pytaniami, nie żałujemy tu pomocy ludziom, którzy własnymi siłami próbują coś zrobić, tylko leni chcących odwalenia roboty za nich nie lubimy. - MasterBLB 2019-07-08 12:31
Tak na przyszłość - tylko jedną odpowiedź w wątku można oznaczyć ptaszkiem, reszta co najwyżej łapkę w górę może dostać. No i miło, że pomogłem^^ - MasterBLB 2019-07-08 13:08
Właśnie wszystko zacząłem oznaczać, a po chwili się skapnąłem, że tylko jedna może być oznaczona. No cóż, trzeba się dostosować. :D - Ikszad 2019-07-08 13:11

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