Thread: Improving the following code:

  1. #1
    Registered User
    Join Date
    Nov 2018
    Posts
    21

    Improving the following code:

    Hi,
    I'm new to programming. This is one of the exercises I had: Finding UPC Check Digit, length of UPC code is 11 digit. I was wondering what are the errors I might have in the code and ways to improve the code. The program is running well so far, no exception handling was applied. So, it was assumed that user entered the input in correct format. I used _CRT_SECURE_NO_WARNINGS to use unsafe scanf, strlen fuctions. I'm new to this forum. Any suggestions will be appreciated. Here is the code:

    Code:
    #include <stdio.h>
    #include <stdlib.h>
    #include <string.h>
    
    
    #define GROUP_SIZE 5
    
    
    
    
    char* intToCharArray(size_t d)
    {
        char buf[33];
        char* temp = _itoa(d, buf, 10);
        size_t d_len = strlen(temp);
        char* newChar = (char*)malloc((sizeof(char) * d_len) + 1);
        for (size_t i = 0; i < d_len; i++) newChar[i] = temp[i];
        newChar[d_len] = '\0';
        return newChar;
    }
    
    
    int* splitDigit(size_t d, size_t d_len)
    {
        char* temp = intToCharArray(d);
        size_t temp_len = strlen(temp);
        int* splited = (int*)malloc(sizeof(int) * d_len);
        if (d_len - temp_len == 0)
        {
            for (int i = d_len - 1; i >= 0; i--)
            {
                while (d > 0)
                {
                    int d_split = d % 10;
                    splited[i] = d_split;
                    break;
                }
                d /= 10;
            }
        }
        else
        {        
            size_t difference = d_len - temp_len;
            for (int i = 0; i < difference; i++) splited[i] = 0;
            for (int i = d_len - 1; i > 0; i--)
            {
                while (d > 0)
                {
                    int d_split = d % 10;
                    splited[i] = d_split;
                    break;
                }
                d /= 10;
            }
        }
        free(temp);
        temp = NULL;
        return splited;
    }
    
    
    int oddEvenSum(int* arr, size_t arr_len, size_t start, int increment)
    {
        int sum = 0;
        for (size_t i = start; i < arr_len; i += increment) sum += arr[i];
        return sum;
    }
    
    
    void printUpc()
    {
        int zth, first, second;
        printf("Please enter the 11 digit UPC code in the following format\n"\
            "*** 0 12345 67890 ***\n>>> ");
        scanf("%1d %d %d", &zth, &first, &second);
        int* firstSplit = splitDigit(first, GROUP_SIZE);
        int* secondSplit = splitDigit(second, GROUP_SIZE);
        size_t temp[11];
        temp[0] = zth;
        size_t i = 1;
        while (i < 11)
        {
            if (i < 6) temp[i] = firstSplit[i - 1];
            else temp[i] = secondSplit[i - 6];
            i++;
        }
        printf("Check digit is: %d\n", (9 - ((oddEvenSum(temp, 11, 0, 2) * 3 + oddEvenSum(temp, 11, 1, 2)) - 1) % 10));
        free(firstSplit);
        firstSplit = NULL;
        free(secondSplit);
        secondSplit = NULL;
    }
    
    
    void main()
    {
        printUpc();
        getchar();
        getchar();
    }

  2. #2
    Registered User
    Join Date
    Dec 2017
    Posts
    834
    You haven't actually described your task. Instead, you've left it up to us to look up how it's supposed to be done (very few people have this kind of thing memorized). You also haven't specified whether it's UPC-A or UPC-E.

    So, here's the rules that I looked up. They are for UPC-A.
    Suppose that you want to find the check digit of UPC-A number 72641217542.

    From the right to the left, start with odd position, assign the odd/even position to each digit.
    Sum all digits in odd position and multiply the result by 3: (7+6+1+1+5+2)*3=66
    Sum all digits in even position: (2+4+2+7+4)=19
    Sum the results of step three and four: 66+19=85
    Divide the result of step four by 10. The check digit is the number which adds the remainder to 10. In our case, divide 85 by 10 we get the remainder 5. The check digit then is the result of 10-5=5. If there is no remainder then the check digit equals zero.
    It can be done with less machinery than you've employed.
    Code:
    #include <stdio.h>
    #include <string.h>
    #include <ctype.h>
     
    int calc_upca_check_digit(const char *upc) {
        const int ExpectedCount = 11;
        int count = 0;           // count of digits found
        int odds = 0, evens = 0; // sums of "odd" and "even" positions
     
        for ( ; *upc; ++upc) {
            if (isspace(*upc)) continue;
            if (!isdigit(*upc)) break;
            if (count % 2 == 0)
                odds  += *upc - '0';
            else
                evens += *upc - '0';
            ++count;
        }
     
        if (count != ExpectedCount)
            return -1;
     
        int sum = odds * 3 + evens;
        return (10 - (sum % 10)) % 10;
    }
     
    int main() {
        char line[1000];
        printf("Enter a UPCA code: ");
        fgets(line, sizeof line, stdin);
        int check_digit = calc_upca_check_digit(line);
        printf("Check digit is: %d\n", check_digit);
    }
    Last edited by john.c; 11-20-2018 at 02:36 PM.
    The world hangs on a thin thread, and that is the psyche of man. - Carl Jung

  3. #3
    Registered User
    Join Date
    Nov 2018
    Posts
    21
    Hey, thanks. the task didn't say what kind of UPC code it was. That's why I couldn't include that. But your code is cool! Very precise. By the way, did you see any error or faults in my code. I'm very new to C. And still don't know many library names and functions. Thanks again.

  4. #4
    Registered User
    Join Date
    Dec 2017
    Posts
    834
    The first thing I notice all the warnings that your code generates (such as passing an int* to a size_t* parameter, which is a no-no). I would change all of your size_t's to int's as there is really no point to using size_t here. _itoa is a non-standard function. What compiler are you using?

    This
    Code:
        if (d_len - temp_len == 0)
    is more commonly written as
    Code:
        if (d_len == temp_len)
    Here's something strange:
    Code:
                while (d > 0)
                {
                    int d_split = d % 10;
                    splited[i] = d_split;
                    break;
                }
    That's the same as saying this (i.e., there isn't really a loop because of the break):
    Code:
                splited[i] = d % 10;
    Is splitDigit divided into two parts just to ensure that any unused elements of the split array are 0? Then just use calloc to ensure they are zero from the start.

    The function would be more generally useful if it accepted a string and returned the check digit.

    Also, main returns int, not void (void is non-standard although allowed by some compilers). And you don't need the backslash at the end of line 73. Literal strings separated by only whitespace are concatenated (and a newline is whitespace).

    So, putting some of this into your code yields:
    Code:
    #include <stdio.h>
    #include <stdlib.h>
    #include <string.h>
    
    #define GROUP_SIZE 5
    
    /*  (Not needed anymore.)
    char* intToCharArray(int n) {
        int sz = snprintf(NULL, 0, "%d", n); // how much space is needed?
        char *str = malloc(sz + 1);          // +1 for null char
        sprintf(str, "%d", n);
        return str;
    }
    */
    
    int* splitDigit(int d, int d_len) {
        int* split = calloc(d_len, sizeof *split);
        for (int i = d_len - 1; i >= 0; i--) {
            split[i] = d % 10;
            d /= 10;
        }
        return split;
    }
    
    int oddEvenSum(int* arr, int arr_len, int start, int increment) {
        int sum = 0;
        for (int i = start; i < arr_len; i += increment)
            sum += arr[i];
        return sum;
    }
     
    int calcUpcCheckDigit(const char *upc) {
        int zth, first, second;
        sscanf(upc, "%1d %d %d", &zth, &first, &second);
        int* firstSplit = splitDigit(first, GROUP_SIZE);
        int* secondSplit = splitDigit(second, GROUP_SIZE);
        int temp[11] = {zth};
        for (int i = 1; i < 11; i++) {
            if (i < 6) temp[i] = firstSplit[i - 1];
            else       temp[i] = secondSplit[i - 6];
        }
        free(firstSplit);
        free(secondSplit);
        return 9 - ( (oddEvenSum(temp, 11, 0, 2) * 3
                    + oddEvenSum(temp, 11, 1, 2)    ) - 1) % 10;
    }
    
    int main() {
        char line[100];
        printf("Enter the 11 digit UPC code in the following format\n"\
            "*** 0 12345 67890 ***\n>>> ");
        fgets(line, sizeof line, stdin);
        int check_digit = calcUpcCheckDigit(line);
        printf("Check digit is: %d\n", check_digit);
    }
    The world hangs on a thin thread, and that is the psyche of man. - Carl Jung

  5. #5
    Registered User
    Join Date
    Nov 2018
    Posts
    21
    Thanks man! Have learned a lot from u.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. help improving code
    By jamort in forum C++ Programming
    Replies: 5
    Last Post: 05-28-2009, 05:13 PM
  2. Improving Code Interface
    By helloamuro in forum C Programming
    Replies: 20
    Last Post: 05-02-2008, 04:34 AM
  3. Improving my code
    By rwmarsh in forum C++ Programming
    Replies: 14
    Last Post: 07-08-2006, 11:18 AM
  4. Code improving
    By Ideswa in forum Game Programming
    Replies: 6
    Last Post: 04-06-2006, 09:49 AM
  5. Improving code
    By Munkey01 in forum C++ Programming
    Replies: 21
    Last Post: 01-15-2003, 01:36 AM

Tags for this Thread