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.

Wow, I didn't expect such a fast answer. Thanks. Yeah, I think'll hand it in like you said. And the teacher is pretty young, so he probably wants to show that he can do it "properly". Anyway, thanks again for the quick answer.

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.

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);

On systems where sizeof(int) is the same as sizeof(int *) your code works, but it's still not correct, because there's no guarantee that the sizes are the same with all compilers.

As for the second vs. the first, the second is a lot less clear. If you want to do something after your loops are done, do it after the loops. That is to say, the first example makes more sense than the second one.

It's true that you don't need a temporary in order to multiply each value by two when printing out, and so that should be done as in the second example. Also, when you're printing out newlines for each row, it makes a lot more sense to do it like this:

Code:

for(i = 0; i < n; i++)
{
  for(j = 0; j < n; j++)
  {
    printf("%3d", p[i][j]);
  }
  putchar('\n');
}

No need to test j; just print out a newline each time a row is completed.

Expressions can also be used in places where you put variables. What I mean is this:

Code:

k=i+1;
l=j+1;
printf("%d. row %d. number: ",k,l);

/* That is cumbersome compared to: */
printf("%d. row %d. number: ", i + 1, j + 1);

And, in fact, you can remove your temp1 variable using the same idea:

Code:

scanf("%d", &p[i][j]);
p[i][j] *= 2;

Whether you should multiply by two when reading in or printing out depends on the requirements. For this program it really doesn't matter, unless your professor cares.

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.

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.

You're right, I tried running it like I posted it and Visual Studio didn't like it very much, but swapping those lines works.

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);

Yes, that makes sense, now that I think about it. I felt there was something strange about that, just couldn't put my finger on it.

All your other suggestions are helpful too. It's funny I didn't spot that I could do away with k,l and temp1 as well. Hopefully this will satisfy my prof., though even if it doesn't I think I've at least learned some handy things here. I guess these are rather basic for a more experienced programmer, but I'm only a newbie yet.

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.

Thank you for all your responses. I think this will be the final version:

Code:

#include <stdio.h>
#include <stdlib.h>
int main()
{
	int n,i,j;	
	int **p=NULL;	
	printf("n="); scanf("%d",&n);	
	p=malloc(n*sizeof(*p));	
	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++)
		{
			printf("%d. row %d. number: ",i+1,j+1);	
			scanf("%d",&p[i][j]);	
			p[i][j]*=2;		
		}
	}
	for(i=0;i<n;i++)
	{
		for(j=0;j<n;j++)
		{
			printf("%d ",p[i][j]);	
		}
		putchar('\n');	
	}
	for(i=0;i<n;i++)
	{
		free(p[i]);		
	}
	free(p);
	return 0;
}

I changed the memory allocation (nice one spotting that, by the way), left out the unnecessary variables and I'm using free at the end too. Left the for cycles separate in the end.

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

You mean like this:

Code:

p=malloc(n*sizeof(*p));
	for(i=0;i<n;i++)
	{p[i]=(int*)malloc(n*sizeof(int));	
		for(j=0;j<n;j++)
		{
			printf("%d. row %d. number: ",i+1,j+1);
			scanf("%d",&p[i][j]);
			p[i][j]*=2;	
		}
	}

Heh, that's true. Another thing I didn't notice. Though I think I'm fine with more for cycles, if only to better see what I'm doing in each one.

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

Yeah, I know that's how it's properly done, but the task was writing a program that works right, so it's not supposed to give an error message at all.

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));

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.

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;		
}

Ah! yes I forgot to square the array rank while malloc()'ing memory so here's the one w/o the typo.

Code:

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

The rest stays as is because this is a 2D array so p[i] == &p[i][j] except if it were a 3D.

Originally Posted by itCbitC

Ah! yes I forgot to square the array rank while malloc()'ing memory so here's the one w/o the typo.

Code:

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

The rest stays as is because this is a 2D array so p[i] == &p[i][j] except if it were a 3D.

I think you mean that p[i][j] (for normal people) here is represented as p[n*i + j]. (Because naturally p[i] cannot possibly equal p[i][j] for all j.)