>Hello guys this is my code.
It's not often that looking at code can leave me speechless.
>Can anyone help me please???
Sure. Let's start by looking at the fatal errors:
>char str[] = { 0 };
As much as many C programmers would love for this to declare a string of unlimited characters, it doesn't. It only allocates one character to the array, effectively giving you either a perpetually empty string (because a null character is required), or a glorified char variable.
>int* cVec = (int*)malloc ( alloc );
You strike me as a C++ programmer who's used to new[] for dynamically allocating arrays. malloc only accepts a byte count, not an item count, which means that on a system with four byte integers you're three bytes short on every item. This causes you to overrun the memory, which corrupts the memory manager, which nearly always results in free choking.
Those are your two fatal errors. They're extremely likely to result in a runtime crash. You also have a portability issues:
>unsigned int alloc = strlen ( value ) + 1;
strlen is declared in , yet you don't include that header.
>//works in windows but not on linux
>//cVec[ i ] = int(value[ i ]);
Then you're compiling as C++ in Windows but not in Linux. This cast style is not supported by C. Also note that comments beginning with // are not supported except in C99 (or C++, or as a non-portable compiler extension) and C99 isn't widely implemented by C vendors yet.
>int index, bit;
This is another indicator that you're compiling as C++. Except in C99, declarations must be at the beginning of a block.
>for ( bit = 8; bit > 0; bit-- ) {
This is the biggie. While 8-bit bytes are a safe assumption on PCs, it's best to write your code without such assumptions. C provides the CHAR_BIT macro for that very reason.
>};
Function definitions don't end with a semicolon.
>int main() {
The correct definition of main is as follows:
int main ( void ) {
/* Your code here */
return 0;
}You can choose to include the two parameter form, and you can also return a different value (though 0, EXIT_SUCCESS, and EXIT_FAILURE are the only portable ones). But you must return a value, end of story.
>printf("Enter a string for convertion : ");
printf isn't required to flush the stream unless the internal buffer fills up or it prints a newline. For maximum portability when you want a user prompt to be visible, always call fflush explicitly. This guarantees that the stream is flushed.
>system ( "pause" );
This assumes the system has a program called pause and that it does what you expect. Not only is this a security risk, it's also not portable.
There's one huge logical error that results in incorrect output:
>for ( bit = 8; bit > 0; bit-- ) {
Much like arrays, bits are calculated with an offset. This means you start at 0 and end at CHAR_BIT - 1 . An immediate fix is this:
for ( bit = CHAR_BIT - 1; bit >= 0; bit-- )
printf ( "%d", ( ( cVec[ index ] & ( 1 << bit ) ) ? 1 : 0 ) );However, there are a few minor nits concerning use of the bitwise operators, and one of them is that it's safest to use unsigned types. This avoids surprises, and while it isn't necessary as long as you're using input characters, it's a good habit to get into:
for ( bit = CHAR_BIT - 1; bit < CHAR_BIT; bit-- )
printf ( "%d", ( ( cVec[index] & ( 1U << bit ) ) ? 1 : 0 ) );The changes are minor. bit was switched to an unsigned type and the condition of the loop adjusted accordingly because an unsigned type will always be greater than or equal to zero. The bit calculation also uses 1U as the base.
There's one severe performance issue in your code. Rather than cache the length of value, you constantly recalculate it using strlen. This is especially damaging in loop conditions.
Now for the quality of implementation issues. These are strictly my opinion, so take them with a grain of salt, but also keep in mind that I've been doing this for a while. ;)
>void charToBinary ( char *value ) {
You never modify value, and I'm a big advocate of const-correctness. Make everything const whenever you can, and you'll have a much easier time with bugs.
>int* cVec = (int*)malloc ( alloc );
In C, conversions to and from void* are implicit. You don't need the cast, and it can hide the legitimate error of forgetting to include . Please avoid casting malloc unless you're trying to write C++ compilable C (not often a requirement).
>int index, bit;
I prefer one variable definition per line. It makes changes easier and doesn't hide variables.
>}//for
Yes, we can see that it's a for loop by looking four lines up. This convention only has a benefit for extremely large blocks, and even then the benefit is debatable. I'd recommend dropping it and keeping your blocks small.
>printf("%d ",cVec[ i ]);printf(" ");printf("\n");
This is awful, both in readability and potential surprises. I'm probably correct in guessing that you thought all of these lines would be performed within the loop, but that's not the case. Only the first statement will be executed as the loop body, and the other two come after the loop terminates.
I stumbled a bit on your code because you create cVec with no particular need to do so. You could have more easily (and more concisely) used value directly and avoided quite a bit of unnecessary work.
Here are a few more potential improvements:
* - Do away with all calls to strlen altogether. One benefit of iterating over strings is you don't need to know the length. You can simply keep going until you find a null character.
* - Avoid printf for printing literals. printf is a very heavy function that does a lot of work. If all you want to do is print a character or a string with no formatting or type conversions, you're better off using puts, fputs, and putchar.
Here's my version of your code with all of the mentioned fixes and improvements:
#include <limits.h>
#include <stdio.h>
#include <string.h>
void charToBinary ( const char *value )
{
size_t bit;
size_t i;
fputs ( "The typed string into Ascii mode: ", stdout );
for ( i = 0; value[i] != '\0'; i++ )
printf ( "%d ", value[i] );
puts ( "\n-----------------------------------------" );
for ( i = 0; value[i] != '\0'; i++ ) {
for ( bit = CHAR_BIT - 1; bit < CHAR_BIT; bit-- )
putchar ( ( value[i] & ( 1U << bit ) ) ? '1' : '0' );
putchar ( ' ' );
}
putchar ( '\n' );
}
int main ( void )
{
char str[BUFSIZ];
fputs ( "Enter a string for conversion: ", stdout );
fflush ( stdout );
if ( fgets ( str, sizeof str, stdin ) != NULL ) {
/* Trim the newline character */
str[strcspn ( str, "\n" )] = '\0';
charToBinary ( str );
}
return 0;
}
