Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am a newbie here. I have written a little C program, which is to create a two-dimensional matrix. Here is the code:

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

int **CreatMatrix(int m,int n){
      int **Matrix;
      int i;
      Matrix=(int**)malloc(m*sizeof(int*));
      for(i=0;i<m;i++){
            Matrix[i]=(int*)malloc(n*sizeof(int));
      }
      return Matrix;
}

int main(){
    int m,n;
    int     **A;
    printf("Please input the size of the Matrix: ");
    scanf("%d%d",&m,&n);
    A=CreatMatrix(m,n);

    printf("Please input the entries of the Matrix, which should be integers!\n");
    int i,j;
    for(i=0;i<m;i++){
            for(j=0;j<n;j++){
                    scanf("%d",&A[i][j]);
            }
    }

    printf("The Matrix that you input is:\n");
    for(i=0;i<m;i++){
            for(j=0;j<n;j++){
                    printf("%3d ",A[i][j]);
            }
            printf("\n");
    }

    for(i=0;i<m;i++)
            free(A[i]);
    free(A);
}

I have run it, and it works fine. But I am not sure if it is right? Can anyone help me debug it?

share|improve this question

migrated from programmers.stackexchange.com Nov 11 '12 at 19:54

This question came from our site for professional programmers interested in conceptual questions about software development.

1  
If it works fine why do you need to debug it? –  Walter Nov 11 '12 at 17:19
    
Because I do not know if is needed to free the pointer Matrix in function CreatMatrix? –  azhi Nov 11 '12 at 17:30

2 Answers 2

up vote 1 down vote accepted

Azhi, creating a 2-dimensional array in the way you have done it works fine but has the disadvantage of needing lots of malloc calls.

An alternative is to allocate a single array and construct your 2D array within that. This way you have only one array to free on completion. But note that you must access it by computing the indices yourself - see the code below. Calculation of the indices would be better placed in an inline function so that it is done consistently.

Note also that malloc can fail - see below.

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

static int *CreatMatrix(int m, int n)
{
    void *mem = malloc(m * n * sizeof(int));
    if (mem == 0) {
        perror("malloc");
        exit(EXIT_FAILURE);
    }
    return mem;
}

int main(void)
{
    int m,n;
    printf("Please input the size of the Matrix: ");
    scanf("%d%d",&m, &n);
    int *array = CreatMatrix(m, n);

    printf("Please input the entries of the Matrix, which should be integers!\n");
    for (int i=0; i<m; i++) {
        for (int j=0; j<n; j++) {
            scanf("%d", &array[i*m + j]); // notice the index calculation
        }
    }

    printf("The Matrix that you input is:\n");
    for (int i=0; i<m; i++) {
        for (int j=0; j<n; j++) {
            printf("%3d ",array[i*m + j]);
        }
        printf("\n");
    }
    free(array);
    return EXIT_SUCCESS;
}
share|improve this answer
    
Thanks a lot, William Morris. Your method is very economic. But it is not so easy to indicate the entries of the matrices. –  azhi Nov 14 '12 at 7:36

This code is correct. You absolutely should not free Matrix in CreatMatrix; if you do, CreatMatrix will return a pointer to the region of memory you just freed. Consequently, your program will invoke undefined behavior.

The way you've written your program, CreatMatrix will allocate space for the matrix, which the caller must free. You should in the very least document this with a comment; you might also consider naming the function something more descriptive, like allocIntMatrix or newMatrix. If later, you expand CreatMatrix (or any similar function) to do anything other than allocate the memory, you should consider making the caller responsible for both the allocation and the free; your function signature would then be something like

void CreatMatrix(int m, int n, void **matrix)

Additional nitpicks:

  • Since this is C and not C++, you need not cast the return pointer from malloc. You might also consider using calloc instead – e.g.,

    Matrix = calloc(m, sizeof(int *));
    

    This makes it a bit clearer that you're allocating space for an array.

  • Since you never modify m and n in CreatMatrix, you should consider declaring them const:

    int **CreatMatrix(const int n, const int m);
    
share|improve this answer
    
Thank you, Benjamin Barenblat. Your answer is very helpful to me! –  azhi Nov 12 '12 at 6:44

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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