Whats wrong with my C code?

Nulty

Writing a c program to create a random array

	
	#define ASIZE 10

	int array[ASIZE];
	int temp=0, i=0;
	
	// Initialize array of random numbers
	srand ( time(NULL) );
	
	while((temp = (rand() % 100)!=0) && (i < ASIZE)){
	
		array[i] = temp;
		i++;

	}

	printf("This is the new array [");
	for(i=0; i<ASIZE; i++)
	{
		printf("%d, ", array[i]);
		
	}
	printf("]\n\t");

my output is:

This is the new array [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, ]

dazberry

Nulty wrote: »

Writing a c program to create a random array

	while((temp = (rand() % 100)!=0) && (i < ASIZE)){

The problem is in this line, hint: look at the brackets and what is actually being assigned...

D.

Nulty

Thanks daz

while(((temp = rand() % 100)!=0) && i < ASIZE)

Nulty

The code gives me a nasty runtime error. Sometimes I get:

[38, 95, 96, 98, -1217152283, 0, 134520820, -1074201848, 134513432, -1215653024, ]

[45, 44, -1218021499, -1077712648, -1218123035, 0, 134520820, -1077712632, 134513432, -1216623776, ]

[1, 62, 14, 85, 1, 72, 4, 30, 23, -1215521952, ]

The repeated 1 isn't a problem in the last but I'm still getting 0's and numbers outside the range of 1 to 99.

Anyone got any debugging tips?

Nulty

Could it be that when the while loop hits a 0, it breaks out of the while - stopping the random gen and assignment - and returning the uninitialised memory from the array?

EDIT
Thats got to be it. "While temp !=0" - when it is 0, it stops the loop.

Nulty

This is my solution. Can anyone do a better one?

	
	#define ASIZE 10

	int array[ASIZE];
	int temp=0, i=0;
	
	// Initialize array of random numbers
	srand ( time(NULL) );
	
	[COLOR="Red"]for(i=0; i<ASIZE; i++){
		while((temp = (rand() % 100)) ==0);
		
		array[i] = temp;
		
	}[/COLOR]

	printf("This is the new array [");
	for(i=0; i<ASIZE; i++)
	{
		printf("%d, ", array[i]);
		
	}
	printf("]\n\t");

moycullen14

Nulty wrote: »

This is my solution. Can anyone do a better one?

Replace

	
	[COLOR="Red"]for(i=0; i<ASIZE; i++){
		while((temp = (rand() % 100)) ==0);
		
		array[i] = temp;
		
	}[/COLOR]

with

	
	[COLOR="green"]for(i=0; i<ASIZE; i++){		
		array[i] = (rand() % 99) + 1;
	}[/COLOR]

Nulty

Can you explain whats going on there please?

Nulty

On looking back at it This would work wouldn't it?

	
	[COLOR="Red"]for(i=0; i<ASIZE; i++){
		while((array[i] = (rand() % 100)) ==0);
		
	}[/COLOR]

spoonface

Nulty wrote: »

Can you explain whats going on there please?

In the below you are assigning to temp the value of 'Is rand()%100 equal to 0 ?' i.e. a 0 or a 1:

while((temp = (rand() % 100)) ==0);

In the below the assignment is being done correctly:

array = (rand() % 99) + 1;

Basically you were trying to do too much in your while statement. I don't know why you even needed a while statement or why you were doing a ==0. All that is unnecessary here.

spoonface

Nulty wrote: »

On looking back at it This would work wouldn't it?

    
    [COLOR=Red]for(i=0; i<ASIZE; i++){
        while((array[i] = (rand() % 100)) ==0);
        
    }[/COLOR]

No, you're mixed up in your concepts. Your while is completely unnecessary here. A while is a loop. What is your while loop trying to do?
The ==0 is a test - what are you trying to test the value of and what for?

moycullen14

Nulty wrote: »

Can you explain whats going on there please?

Looking at the code, you are trying to assign a 10 element integer array with random values from 0 to 99 - except that the code is stopping the assignment of 0 - that's what the while loop is doing (while the random number is 0, loop).

So you are trying to set the values of the array to 1 - 99. That's 99 possible numbers. If, instead of modding by 100 (100 possible), you mod by 99, you get 99 possible values - 0 thru 98. Add 1 to that and hey presto, you have random values from 1 thru 99. The while loop is redundant. When you think of mod & random, think of the number of possible values, rather than their values - that helps.

There's some interesting coding points here.

1: Try and make it as efficient as possible.
2: Empty loops are very hard to read and debug. You should get into the habit of trying to make code as tight as possible. It usually means less bugs and easier maintenance
3: The vast majority of issues with code are to do with boundary and error conditions - the first or last possible values, values outside a range, etc. Always be thinking of them. Try and not have special code (your while loop is special code for the 0 case, for example) for these conditions - it leads to bloat, maintenance issues and bugs. Imagine your code if the conditions changed so that the values 1-5 and 95-99 were disallowed!

Nulty

spoonface wrote: »

No, you're mixed up in your concepts. Your while is completely unnecessary here. A while is a loop. What is your while loop trying to do?
The ==0 is a test - what are you trying to test the value of and what for?

It does work. The while loop keeps setting the i'th value in the array to (rand() % 100) while the i'th value is equal to 0. It stops the while loop if the (rand % 100) evaluates to any thing but 0, which will be in the range of 1..99.

All I've done is swap out the temp variable

To answer your question, I'm testing to see is rand returns 100. If it does then (100 % 100 == 0). if that happens then one of my array values will be 0 and I don't want any of them to be 0.

Nulty

moycullen14 wrote: »

Looking at the code, you are trying to assign a 10 element integer array with random values from 0 to 99 - except that the code is stopping the assignment of 0 - that's what the while loop is doing (while the random number is 0, loop).

So you are trying to set the values of the array to 1 - 99. That's 99 possible numbers. If, instead of modding by 100 (100 possible), you mod by 99, you get 99 possible values - 0 thru 98. Add 1 to that and hey presto, you have random values from 1 thru 99. The while loop is redundant. When you think of mod & random, think of the number of possible values, rather than their values - that helps.

There's some interesting coding points here.

1: Try and make it as efficient as possible.
2: Empty loops are very hard to read and debug. You should get into the habit of trying to make code as tight as possible. It usually means less bugs and easier maintenance
3: The vast majority of issues with code are to do with boundary and error conditions - the first or last possible values, values outside a range, etc. Always be thinking of them. Try and not have special code (your while loop is special code for the 0 case, for example) for these conditions - it leads to bloat, maintenance issues and bugs. Imagine your code if the conditions changed so that the values 1-5 and 95-99 were disallowed!

Great post, thanks for that!

Trojan

It's also good practise to write tests like this:

while((array[i] = (rand() % 100)) ==0);

the other way around like this:

while (0 == (array[i] = (rand() % 100)));

It's good practise as you avoid certain types of hard to detect bug - typically assignment instead of comparison still testing as true.

Also this code is a perfect example of when you could use a step-through debugger to find out what values are getting set. Learn to love the debugger

Nulty

By debugger do you mean those found in IDEs like Eclipse?

yawha

Trojan wrote: »

It's also good practise to write tests like this:

while((array[i] = (rand() % 100)) ==0);

the other way around like this:

while (0 == (array[i] = (rand() % 100)));

It's good practise as you avoid certain types of hard to detect bug - typically assignment instead of comparison still testing as true.

Also this code is a perfect example of when you could use a step-through debugger to find out what values are getting set. Learn to love the debugger

I disagree wholeheartedly.

Firstly, I don't think having the comparison in the other way round is an accepted good practice. Personally, I think it decreases readability. And it's pretty hard to do in any modern strongly typed language. Furthermore, pass "-Wall" to gcc/g++ and it'll warn you about it. I'd imagine the same can be done in MS compilers.

Secondly, there's no way a debugger should be used here. I'm of the opinion that if you want to learn to code right, you should avoid relying on debuggers for such simple stuff.

Colonel Panic

Meanwhile in the real world, I see assignment instead of equality bugs in large code bases all the time and getting into the habit Trojan suggested does help.

Nulty

More C code.

How can I keep the buffer from overflowing into the next scanf's if the user inputs 35154 into the first (or any) of the scanfs?

History : 35
Math: 15
English: 4

printf("Enter Math result: ");
	scanf("%2d", &student->math);
	
	printf("Enter History result: ");
	scanf("%2d", &student->hist);
	
	printf("Enter English result: ");
	scanf("%2d", &student->eng);

Nulty

Hows this?

	printf("Enter Math result: ");
	scanf("%2d", &student->math);
	[COLOR="Red"]while(getchar()!='\n');[/COLOR]
	
	printf("Enter History result: ");
	scanf("%2d", &student->hist);
	[COLOR="Red"]while(getchar()!='\n');[/COLOR]
	
	printf("Enter English result: ");
	scanf("%2d", &student->eng);
	[COLOR="Red"]while(getchar()!='\n');[/COLOR]
	
	display(student);

Trojan

yawha wrote: »

I disagree wholeheartedly.

Firstly, I don't think having the comparison in the other way round is an accepted good practice. Personally, I think it decreases readability. And it's pretty hard to do in any modern strongly typed language. Furthermore, pass "-Wall" to gcc/g++ and it'll warn you about it. I'd imagine the same can be done in MS compilers.

We'll agree to disagree. For me, it's a good coding habit, and the particular environment, compiler or typing of the language are all irrelevant - you won't always be coding in that language or with that compiler.

http://faisalferoz.wordpress.com/2010/09/16/why-boolean-true-type-of-comparisions-should-be-avoided/

Why wrote:

This technique is safe as you cannot assign a variable or anything to a constant. My little secret when I used to interview people for C/C++ jobs I always gave them some type of code involving such comparisons and would keep an eye on how do they write their conditions to get an idea bout how much they have coded and how much pain they have suffered from having such bugs in the code. Anyways this technique is same as the if ( bool ) check.

It is pretty easy for a bug like this to slip in during maintenance. So we should keep an eye out for less careful future programmers who will be maintaining the code that we once wrote.

WordPress Coding Standards
http://codex.wordpress.org/WordPress_Coding_Standards#Yoda_Conditions

WordPress Coding Standards wrote:

Yoda Conditions

When doing logical comparisons, always put the variable on the right side, like above. If you forget an equal sign it'll throw a parse error instead of just evaluating true and executing the statement. It really takes no extra time to do, so if this saves one bug it's worth it.

Example bugs (just the first few off the search results, there are thousands)

FBDialog initializer (init) has comparison instead of assignment to self
https://developers.facebook.com/bugs/278289512191769/

Bug 387884 - Use of assignment instead of comparison in assertion
https://bugzilla.mozilla.org/show_bug.cgi?id=387884

[gs-bugs] [Bug 692647] Mistaken comparison-instead-of-assignment in contrib/japanese/gdevmjc.c
http://ghostscript.com/pipermail/gs-bugs/2011-November/022978.html

yawha wrote: »

Secondly, there's no way a debugger should be used here. I'm of the opinion that if you want to learn to code right, you should avoid relying on debuggers for such simple stuff.

Again we'll have to agree to disagree. Stepping through a debugger can be a great aid in helping people learn to code, particularly around loop boundary conditions where most beginners (and pros) can have difficulty. It's also good for beginners to learn what the IDE can provide so that when they're stepping through production code later they know how to use the tools at hand.

Giblet

I agree with Trojan, it's not a big issue to read it either, and it solves valid assignments ****ing up tests. The best coding practices are the ones that cause less errors.

yawha

Trojan wrote: »

FBDialog initializer (init) has comparison instead of assignment to self
https://developers.facebook.com/bugs/278289512191769/

That's the opposite problem, and wouldn't have been solved but having the operands reversed

I do see the point of it, and understand that it's a common mistake (I've made it myself many times), I just think reversing the operands looks kind of ugly, and it's a solved problem in languages that don't allow ints to be implicitly casted to bools, although we aren't going to move away from C/C++ any time soon...

Trojan wrote: »

Again we'll have to agree to disagree. Stepping through a debugger can be a great aid in helping people learn to code, particularly around loop boundary conditions where most beginners (and pros) can have difficulty. It's also good for beginners to learn what the IDE can provide so that when they're stepping through production code later they know how to use the tools at hand.

Here I tend to err on the side of the philosophy of avoiding creating reliance on IDEs when initially teaching one to code. Large, collaborative code bases most definitely almost require the use of debuggers to work on effectively, but initially, I strongly feel beginners should learn to read code, understand what it does, and master the art of printf debugging.