9
\$\begingroup\$

I have written a code for basic array creation and printing it's values in C. I would appreciate comments on how it can be improved, and the industry standards.

One issue I'm encountering is having no way to tell printArr() what the size of the array is.

Some questions:

  1. Can I use arr[i] anywhere in the functions instead of **arr? (Gave error when I tried it)
  2. What is the most common way of dealing with arrays when writing professional code? Is my approach correct?
int * initArr (int ** arr, int n)
{
    *arr=(int *) malloc(sizeof(int*)*n);
    printf("size is %d\n", sizeof(*arr));
    int i;
    for (i=0;i<n;i++)
    {
        *(*arr+i)=i+10;
    }
    return *arr;
}

void ** printArr (int ** arr, int n)
{
    int i;
    for (i=0;i<n;i++)
    {
        printf("Arr element %d is %d\n", i, *(*arr+i));
    }
    return *arr;
}


int main(void) {

    int *arr2;
    arr2=initArr(&arr2,10);
    printArr(&arr2,10);

    return 0;
}

Running the above code (with includes for stdio and stdlib) produces:

size is 8
Arr element 0 is 10
Arr element 1 is 11
Arr element 2 is 12
Arr element 3 is 13
Arr element 4 is 14
Arr element 5 is 15
Arr element 6 is 16
Arr element 7 is 17
Arr element 8 is 18
Arr element 9 is 19
\$\endgroup\$
18
\$\begingroup\$

There are a number of things I would change here, so I'm going to go through the process incrementally, applying changes to the whole file in passes rather than in chunks. I think this should make it more clear why the changes are implemented.


C99 Note

You should have access to, at the very least, a C99 compiler. If that's not true, you can ignore this first point, but if it is, you should change your counter declarations to be inside the first for clause.

for (int i=0;i<n;i++)

Pointers

In initArr, you both return the int array and assign it to the provided out parameter, arr. Why? Out parameters make code confusing and hard to follow! They make sense if you're performing the array allocation somewhere else, but since you're performing the array allocation inside initArr, just drop the arr parameter and return the value.

As a side effect of this, since you no longer need a double pointer, you can replace your dereferences with simple array accesses.

int * initArr (int n)
{
    int * arr = (int *) malloc(sizeof(int*)*n);
    printf("size is %d\n", sizeof(arr));
    for (int i=0;i<n;i++)
    {
        arr[i]=i+10;
    }
    return arr;
}

Next, why are you passing a double pointer (int**) to printArr? More importantly, why does printArr have a return value if it's a purely side-effectful function? Change the parameter to int* and make the function return void.

void printArr (int * arr, int n)
{
    for (int i=0;i<n;i++)
    {
        printf("Arr element %d is %d\n", i, arr[i]);
    }
}

This also simplifies the main function.

int main(void) {
    int * arr2 = initArr(10);
    printArr(arr2,10);

    return 0;
}

Next, let's take a look at the allocation itself (the one in initArr). First of all, you cast to (int *) manually, which is unnecessary and downright discouraged in C. If you're using C++, it's necessary (though you shouldn't need malloc in C++, anyway), but with a C compiler, just drop it.

Second of all, you are not actually allocating the right data! You're allocating n slots for int* values—int pointers. You actually want int data. This might not matter depending on your architecture and compiler, but it's still poor code. Fortunately, you can actually fool-proof this—don't pass an explicit type of sizeof at all! Just deference arr itself, and the compiler will calculate that value's size.

Those changes simplify the allocation to this:

int * arr = malloc(sizeof(*arr)*n);

Finally, the line just after that—the printf line—is useless. It will always print the same value because it's checking the size of a pointer, which is always the same size regardless of type (though it can change on different architectures). That said, the line doesn't make any sense there. A function called initArr shouldn't have side effects, anyway. Just take it out.


Style and Warnings

I'm not sure if you just omitted it from your code, but your code depends on functions declared in external header files in the standard library. Include these at the top of your file.

#include <stdlib.h>
#include <stdio.h>

Next, let's talk about code style. You are writing some very densely-packed expressions in some places. Some whitespace can make code much more readable! For example, change the for loops to this:

for (int i = 0; i < n; i++)

Similarly, change your allocation to this:

int * arr = malloc(sizeof(*arr) * n);

Proper spacing makes code more readable and therefore more maintainable!

Finally, let's talk about pointer declarations. You are declaring your pointers with the asterisks just sort of "floating". This is actually not a bad compromise. Some people prefer them to be grouped with the type (int* foo), others with the name (int *foo). I prefer the former style, so in my final copy of the code, I changed them accordingly, but this is often merely personal preference.


Result

Here is all the code as it stands with the above changes implemented! Not only is it more readable due to style, but it's easier to understand the control flow since it no longer unnecessarily indirects variables via reference.

#include <stdlib.h>
#include <stdio.h>

int* initArr(int n)
{
    int* arr = malloc(sizeof(*arr) * n);
    for (int i = 0; i < n; i++) {
        arr[i] = i + 10;
    }
    return arr;
}

void printArr(int* arr, int n)
{
    for (int i = 0; i < n; i++)
    {
        printf("Arr element %d is %d\n", i, arr[i]);
    }
}


int main(void) {
    int* arr2 = initArr(10);
    printArr(arr2, 10);

    return 0;
}

Extra Notes

Can I use arr[i] anywhere in the functions instead of **arr? (Gave error when I tried it)

You can! But * has lower precedence than array subscript ([]), so you'd need to use grouping operators

(*arr)[i]

This became unnecessary with the other changes, though.

What is the most common way of dealing with arrays when writing professional code? Is my approach correct?

Depends on the situation. Honestly, the best away to handle arrays is to not use them—if you're using a language with any more power than C, you should have higher-level language constructs (std::vector, ArrayList, etc.). However, if you have to use C, abstracting them behind an API is a good start.

One issue I'm encountering is having no way to tell printArr() what the size of the array is.

Yes, in C, arrays are just blocks of memory, so you can't get their length at runtime because it's not stored anywhere! If you wanted to do this, though, you could encapsulate your array in a struct.

typedef struct my_array {
    unsigned int length;
    int* data;
} my_array_t;

You could then return a my_array_t from initArr and pass a my_array_t to printArr, which could use arr.length to get the length.

\$\endgroup\$
  • 1
    \$\begingroup\$ An answer like this makes me wonder why I haven't seen you review more C questions. +1 \$\endgroup\$ – syb0rg Jan 28 '15 at 18:13
  • \$\begingroup\$ regarding this line: 'int* arr = malloc(sizeof(*arr) * n);' Code should always check the returned value from malloc (and family) to assure the operation was successful. Also, why the '*arr' a much clearer parameter would be 'int'. \$\endgroup\$ – user3629249 Jan 28 '15 at 23:10
  • \$\begingroup\$ @user3629249 I dispute that first point. In plenty of cases, if malloc fails, then what? How do you recover from an out of memory error in a simple C utility? In regards to your second point, *arr is much better because if the type of arr changes to long*, it will work correctly. \$\endgroup\$ – Alexis King Jan 28 '15 at 23:12
  • \$\begingroup\$ A few points. 1) you CAN do "for (int i = 0;", but IMHO there's no good reason to do so. 2) Why would you not cast the pointer returned by alloc functions to the desired type? 3) It's clearer to write int arr" rather than "int * arr" (which at a quick glance can be taken for multiplication); 4) "malloc(sizeof(int)*n);" is allocating space for n POINTERS, which may not be the same size as ints. Use (n * sizeof (int)) instead. 5) Put your main at the top. \$\endgroup\$ – jamesqf Jan 29 '15 at 0:31
  • 1
    \$\begingroup\$ @jamesqf I addressed points 3 and 4 by the end. Did you actually read my whole review? Also, I completely disagree with your points 1, 2, and 5. \$\endgroup\$ – Alexis King Jan 29 '15 at 0:38
6
\$\begingroup\$
  • Return values

    • initArr communicates the start address via both the return value and a pass-by-reference parameter. It surely is redundant. Normally you'd pick one of two possible signatures:

      int * initArr(int size);
      void initArr(int ** arr, int size);
      

    and stick to it.

    • printArr, as the name suggests, only produces a side effect of values being printed. It has nothing to report back except possibly errors encountered while printing. Make it void printArr(int *, int).
  • (Ab)use of sizeof

    initArr allocates an array of n units of a pointer size. This works by coincidence: in pretty much any platform space taken by a pointer is enough to accomodate an integer. However it is not guaranteed.

    printf("size is %d\n", sizeof(*arr)) is totally misleading. It is equivalent to printf("size is %d\n", sizeof(int *)), and doesn't depend on how many bytes are actually allocated. Try to pass size 0 to initArr.

\$\endgroup\$
5
\$\begingroup\$

On the whole, this needed to be simplified. There are some things going on that don't make a lot of sense. You don't need to pass a pointer-to-a-pointer into initArr, just return the initialized array.

int *initArr (int n) // you don't need to pass in a pointer-to-a-pointer here
{
    int *arr = (int *) malloc(sizeof(int)*n); 
      // ^ sizeof(int), not sizeof(int *)
    printf("size is %d\n", sizeof(arr)); 
     // ^ sizeof() here returns 8, the size of the pointer (64-bit machine), 
     // not the size of the array
    int i;
    for (i=0;i<n;i++)
    {
        arr[i]=i+10; // use array notation where possible 
                     // until you're more familiar with C 
    }
    return arr;
}

void printArr (int *arr, int n) 
  // this ^ was declared to return a pointer to a void pointer, 
  // actually returned a pointer to an int pointer.  You didn't 
  // need either of those
{
    int i;
    for (i=0;i<n;i++)
    {
        printf("Arr element %d is %d\n", i, arr[i]);
    }
}


int main(void) {

    int *arr2 = initArr(10);
    printArr(arr2,10);
    free(arr2); // you need to free anything you malloc

    return 0;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I was wondering when free() was going to be mentioned :) \$\endgroup\$ – user52380 Jan 30 '15 at 6:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.