Thread: multidimensional array help

Hi!

I'm trying to write a program using a two-dimensional array (for school), and I think I've got it, but my teacher sent back the code I sent to him, saying there is a "grave mistake" in it. I've been trying to figure it out for the past two hours, but I don't get what he's getting at, so I thought I'd ask around here.

What I want to accomplish is this:
I give the program an 'n' number, which is the number of both the rows and the colums of a matrix, after which I'm asked to input further n*n numbers for the matrix. After this, the program is supposed to multiply these numbers by two then write the matrix to the screen.

What I've come up with for this is:

Code:

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

int main()
{
	int n,i,j;	//n is the number of rows and columns in the matrix, i and j are for the for cycles
	int k,l;	//k and l are used so that the program starts asking for 1st row 1st number and not 0r 0n
	int temp1,temp2;
	int **p=NULL; 
	printf("n="); scanf("%d", &n);	//inputting n
	p=(int**)malloc(n*sizeof(int));		//allocating memory for the matrix
	for(i=0;i<n;i++)
	{
		p[i]=(int*)malloc(n*sizeof(int));	
	}
	for(i=0;i<n;i++)
	{
		for(j=0;j<n;j++)
		{
			k=i+1;
			l=j+1;
			printf("%d. row %d. number: ",k,l);	//asking for the numbers in the matrix
			scanf("%d",&temp1);		//inputting the numbers into the first temporary variable
			p[i][j]=temp1;	//putting it into the matrix
		}
	}
	for(i=0;i<n;i++)
	{
		for(j=0;j<n;j++)
		{
			temp2=p[i][j];	//putting each number in the matrix into a second temporary variable
			temp2=2*temp2;	//multiplying by two
			p[i][j]=temp2;	//putting it back into the matrix
			printf("%3d",p[i][j]);	//printing out the matix 
			if(j==(n-1)) {printf("\n");}	//if the end of a row is reached, start a new row
		}
	}
}

As you can see, I'm not exactly experienced here. After I thought about it a bit, I've figured out that using temp2 is unnecessary and that I could put the second for cycle inside the first (though I don't think this makes the program any better). Also, I figured that I'd set every variable I'm using to 0, so there can be no problem there.

So this is my second version:

Code:

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

int main()
{
	int n=0,i,j;	
	int k=0,l=0;	
	int temp1=0;
	printf("n="); scanf("%d", &n);
	int **p=(int**)malloc(n*sizeof(int)); 
	for(i=0;i<n;i++)
	{
		p[i]=(int*)malloc(n*sizeof(int));	
	}
	for(i=0;i<n;i++)
	{
		for(j=0;j<n;j++)
		{
			k=i+1;
			l=j+1;
			printf("%d. row %d. number: ",k,l);	
			scanf("%d",&temp1);
			temp1=2*temp1;
			p[i][j]=temp1;
			if((i==(n-1))&(j==(n-1)))	//only start writing the matrix once every number has been read and multiplied 
			{
				for(i=0;i<n;i++)
				{
					for(j=0;j<n;j++)
					{
						printf("%3d",p[i][j]);
						if(j==(n-1)) {printf("\n");}
					}
				}
			}
		}
	}
}

Is this any better than the first? What was the "grave mistake" in the first one, and did I accidentally solve it with the second one? Also, the teacher mentioned that there was a simpler way of doing this. If anyone could help, I would much appreciate it.

Thank you very much, in advance.

Originally Posted by dg862

I'm trying to write a program using a two-dimensional array (for school), and I think I've got it, but my teacher sent back the code I sent to him, saying there is a "grave mistake" in it.

I have a university degree and your prof is 1) a jerk 2) a jerk, and 3) a jerk, and I would bet money if you ask other students or even other professors they will confirm that. Don't take a class with him/her again unless you have to, and don't waste your brain trying to satisfy this person when you almost certainly have other and better opportunities to actually learn.

Your second version works fine, hand it in -- except I would add "return 0" at the very end.

Well, I don't want to mislead you -- my degree is not in computing. But if I walked into a prof's office during his/her consultation hours with a question or whatever and received that kind of non-advice, I wouldn't bother asking for any more advice. If he believes this is the extent of his job, he must be aware that he himself would easily be replaced by:

- some textbooks
- a C compiler
- an assignment submission site

I think you are paying for more than that, tho.

w/r/t your code you might also want to free() what you malloc at the end. It doesn't really matter here, but at least this will demonstrate that you understand the need.

Also, gcc -pendantic advises:

test2.c:10: warning: ISO C90 forbids mixed declarations and code

which means you should swap line 9 and 10 around. It is much nicer to put all your declarations first, since then you know exactly where they are when your code gets longer. I believe the compiler prefers this too, someone in the know here may explain.

Yay cas!

Originally Posted by cas

Code:

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

This is the only "real" problem I see with the code. You're allocating pointers here, not ints, so you should multiply by sizeof(int *); or, perhaps somewhat nicer:

Code:

p = malloc(n * sizeof *p);

I had not noticed this, but it certainly would constitute a "grave mistake" in the sense that it will work (since an int is at least the size of a pointer) but if these had been chars, you would have created an overflow.

It only applies to the array declaration. You are not using a "pure" 2D array which would be this:

Code:

int matrix[10][10];

You are using a 1D array of pointers to more 1D arrays. That's why these two lines should be different:

Code:

	int **p=(int**)malloc(n*sizeof(*int)); 
	for(i=0;i<n;i++)
	{
		p[i]=(int*)malloc(n*sizeof(int));	
	}

I would still stick with the second version but separate the loops as cas says.

Two more issues for completeness.

1. You should check each malloc call for success. If malloc returns NULL, you need to take care of that.

2. You should call free as well, first a loop for each p[i], then free p

imo you need only two loops; one for malloc()'ing memory / initializing array elements etc. and another loop for printing out the elements of the 2D array; and make use of the dereferencing operator "*" instead of using the p[i][j] syntax; and i see no "grave mistake" other than the one cas has already pointed out.

Re: error-checking malloc, you can do this:

Code:

void *ec_malloc (size_t bytes) {
	void *assign;
	assign = malloc(bytes);		
	if (assign == NULL) {
             puts("OUT OF MEMORY!??!");
             /* maybe you want to do other different stuff here now */
             return NULL;
        }
	return assign;
}

ec_malloc() can then be substituted for malloc():

Code:

int *ray=ec_malloc(12*sizeof(int));

Originally Posted by itCbitC

Note that p[i] is the same as &p[i][j] and p[i][j] *=2 can be written as *p[i] *= 2. This is based on the fact that x[n][n] is equivalent to y[n*n] as in.

Code:

p=malloc(n*sizeof(int*));	
for(i=0;i<n*n;i++)
{
    p[i]=(int*)malloc(sizeof(int));	
    scanf("%d",p[i]);	
    *p[i] *=2;		
}

Based on this the two inner for loops can be eliminated. That's just my 2c, though you have to decide if code conciseness is to be preferred over clarity.

The above advice and code is completely wrong! p[i] is definitely not the same as &p[i][j]

There are two obvious ways of representing a 3D array. One is the way dg862 has used, the other way is the bitmap scanline type of approach where you store each row in consecutive memory, all in one big array. Some code for this way would actually be:

Code:

int *p=malloc(<n*n*sizeof(int));	
for (i=0; i<n*n; i++)
{
    scanf("%d", &p[i]);	
    p[i] *=2;		
}