C program to print Pascal's triangle

Question

Worked 2 hours on this. It print Pascal's triangle. Change MAX_LEN to any desired triangle size. What do you think about the quality of the code?

#include "stdafx.h"
#include <stdio.h>
#include <malloc.h>


int main() {
        const unsigned int MAX_LEN = 20;
        unsigned char rowNum, rowWidth, rowOffset, i;
        unsigned int *numPointer, *num;
        unsigned int maxNum = 0;

        for (char len = 1; len <= MAX_LEN; len++)
            maxNum += len;
        num = (unsigned int*)malloc(sizeof(unsigned int)*maxNum);
        if (num == NULL)
            return 1;

        for (rowNum = 0; rowNum < MAX_LEN; rowNum++) {
            rowOffset = 0;
            for (i = 0; i <= rowNum; i++) {
                rowOffset += i;
            }
            num[rowOffset] = 1;
            for (i = 1; i < rowNum; i++) {
                num[rowOffset + i] = num[rowOffset + i - rowNum] + num[rowOffset + i - rowNum - 1];
            }
            num[rowOffset + rowNum] = 1;
        }

    numPointer = num;
    for (rowNum = 0; rowNum < MAX_LEN; rowNum++) {
        for (rowWidth = 0; rowWidth <= rowNum; rowWidth++) {
            printf_s("  %d  ", *numPointer);
            numPointer++;
        }
        printf_s("\n");
    }
        return 0;
}

Answer 1

Some things to consider:

1. for (char len = 1; len <= MAX_LEN; len++) is extremely dangerous as you could easily overflow len here since MAX_LEN has type unsigned. Currently, the value of MAX_LEN is 20, and your code is safe; however, if you decide to change this value at any point, you will likely overflow len here, leading to undefined behavior either because of overflow of a signed type if char is signed or because of the fact that infinite loops (into which this one will turn) that do not do IO are ub in the case of an unsigned char type.

2. In C, you do not need to cast the return value of malloc. Simple assignment will do. In fact, don't actively cast the return value of malloc because it clutters your code with casts, which is a symptom of bad code in general, and can hide a bug related to implicit functions (only before C11).

3. Actually, that loop from point 1 I mentioned can be removed altogether. This follows from recognizing that you are adding all integer values starting from one up to MAX_LEN, for which there is an easy to compute formula: S = n(n + 1)/2. Translated into code, this would be

   maxNum = MAX_LEN * (MAX_LEN + 1) / 2;
   

   which is not only shorter, but also more efficient to compute.

4. Why do you define almost all your variables at the top of your function, although they are not used until much later? This is a bad practice, because it increases the mental load everyone reading your code has to deal with. I don't want to remember the types of eight or nine different variables while focusing on code that doesn't even use them. Just define them right were they need to be defined.

5. Overall, you use char too much. Only use char when you are working with characters or single bytes, otherwise use int, unsigned and everything larger or equal in bit size. You are not gaining anything through your use of char except a high chance to trigger ub several times over in your code (You are likely not even saving space because of alignment concerns on most common modern platforms.)

6. Don't printf a single character; that's wasteful. Use putchar for that.

7. Don't include stdafx.h. Unless you have a concrete compilation performance concern, it won't do anything for you. And even then, it is not standard, and most people who use compiler other than MSVC will likely complain about it.

8. Split your code up into functions. main() is not the right place to write problem-solving code, only to put things together. Do yourself a favor and organize your code a little more, it will help you keep a good overview over which part does what and make other readers not turn away on sight of that huge main god-function.