Can't find the bug

Code:

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

int bin2dec(const char *);
char* dec2bin(const char *);

int main(void)
{
	char dec[33];
	char bin[33];

	printf("Type a binary number: ");
	fgets(bin, sizeof(bin), stdin);
	printf("%s is %d in decimal\n", bin, bin2dec(bin));

	printf("\nType a decimal number: ");
	fgets(dec, sizeof(dec), stdin);
	printf("\n%s is %s in binary\n", dec, dec2bin(dec));

	return 0;
}

int bin2dec(const char *binario)
{
	int i = 0;          
	char *ptr;   
	int dec = 0;

	if((ptr = strchr(binario, '\n')) != NULL)
		*ptr = '\0';

	while(binario[i] == '0' || binario[i] == '1' && i <= 32)
	{
		if(binario[i] == '0')
			dec <<= 1;

		else
		{
			dec ^= 1;
			dec <<= 1;
		}

		++i;
	}

	dec >>= 1;

	return dec;
}

char* dec2bin(const char *decimal)
{
	int i;        
	int dec;      
	int length;   
	char *ptr;   
	char bin[33];

	if((ptr = strchr(decimal, '\n')) != NULL)
		*ptr = '\0';

	dec = atoi(decimal);

	i = 0;
	while(dec)
	{
		if((dec & 1) == 1)
			bin[i] = '1';

		else
			bin[i] = '0';

		dec >>= 1;
		++i;
	}

	bin[i] = '\0';

	length = strlen(bin) - 1;

	/* reverse array in place */
	for(i = 0; i < length; i++, length--)
	{
		   /* I use dec as a temporary varible so
		      that I don't have to declare another one */
		dec = bin[i];
		bin[i] = bin[length];
		bin[length] = dec;
	}

	return bin;
}

Here's what happens when I run it:

Code:

Type a binary number: 1010
1010 is 10 in decimal

Type a decimal number: 10

10 is hÉ@ in binary

Can someone help me find what is wrong? Thank you.

Code:

char* dec2bin(const char *decimal)
{
	<snip>
	char bin[33];
	<snip>
	return bin;
}

Don't return pointers to local variables. bin is already out of scope by the time you try to use it.

I declared bin as static and it worked. Is it okay to do such a thing? Thanks.

definately not a good idea. just add an extra parameter to the function for a destination buffer.

Originally Posted by caduardo21

I declared bin as static and it worked. Is it okay to do such a thing? Thanks.

That's one way to go about it. There's nothing wrong with doing it that way.

well, you just have to be sure to do something with the data between calls.

Originally Posted by legend

well, you just have to be sure to do something with the data between calls.

like strncpy() to a local variable. Though I believe it would be easier just to send a char * and just edit the string directly.

Originally Posted by itsme86

That's one way to go about it. There's nothing wrong with doing it that way.

I wouldn't say nothing. If that same function suddenly has to be executed in a threaded environment, you will get problems.

I would. There's nothign wrong with using the static keyword. You just have to understand how it works. If you understand how it works, then you're fine in using it.

There's no reason to use a sledge hammer to drive a nail when a simple claw hammer will work better. Use the tool that fits the task.

Quzah.

Originally Posted by quzah

I would. There's nothign wrong with using the static keyword. You just have to understand how it works. If you understand how it works, then you're fine in using it.

well, when code at the time of writing is not multithreaded you wont have any problems. But when a few years later the same function has to run in a multithreaded environment (I have seen it happening) this will be one extra thing to change. So it's more a practical maintenance issue as there is nothing wrong with using local static return values from a C standard point of view.

Does this homework problem really look like it's going to be multithreaded to you? Ever?

Quzah.

Originally Posted by quzah

Does this homework problem really look like it's going to be multithreaded to you? Ever?

Quzah.

Saying there's nothing wrong with using static doesn't help either.

No need for fighting guys, the problem's solved. I changed the function and both are returning ints now

Not every function ever written has to be made for a multi-threaded enviornment.

It comes down to one simple question: Do you know how to correctly use the function you've written? If no, you'd better rewrite it. If yes, then where's the problem?

This function isn't the end all function of all functions. It does one task, and the person that wrote it knows how to use it. Who gives a ........ if it doesn't work in some other case?

You just apparently have some need to prove that you're "right", so you come up with a scenario where this function won't work. Who cares if it doesn't work in a multi-threaded application. Is this program multi-threaded? No. Does the function work here? Yes. Good. Stop your whining. No one cares if it works in a multi-threaded enviornment except you. In which case, write you own and shut up about it.

It is a good idea, if they know how to use it, which they do. Just because it's not your idea, doesn't mean it's not a good one. It works. It's perfectly valid C. It conforms to the standard, doesn't invoke undefined behavior, and does the job. That's considered good. That's also considered way better than most of the people posting here.

There is nothing wrong with using static. You're an idiot if you think there is. I guess you've never heard of strtok eh?


Quzah.

Originally Posted by quzah

There is nothing wrong with using static. You're an idiot if you think there is. I guess you've never heard of strtok eh?

Quzah.

yeah, strtok is one of the reasons that compilers like VC and BorlandC had to invent stuff like including libcmt.lib or if you did normal C libc.lib. With strtok C started to really show its age. Many people have had crashes in their programs because they used strtok correctly but failed to include the proper crt lib.

No need to get so defensive