Hello guys this is my code.

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

using namespace std;

void charToBinary( char *value ) {
    unsigned int i;
    unsigned int alloc = (strlen(value)+1);
    int* cVec = (int*)malloc(alloc);

    for( i = 0; i < strlen(value); i++ ) {
            //works in windows but not on linux 
            //cVec[ i ] = int(value[ i ]);
            cVec[ i ] = (int)(value[ i ]);
    }//for
    printf("\n");
    printf("The typed string into Ascii mode: ");
   for( i = 0; i < strlen(value); i++ )
       printf("%d  ",cVec[ i ]);printf(" ");printf("\n");
   printf("-----------------------------------------\n");
   int index,
        bit;
   for(index = 0 ; index < strlen(value); index++ ) {
       for(bit = 8; bit >0; bit-- ) {
           
             //here is my bug
             // it segmentation fault using eclipse CDT.
             // By using Borland C++ there's no problem. 
           
           
               printf("%d",((cVec[ index ]&(1 << bit))? 1 : 0));
                
       }
      printf(" ");
   }
   free( cVec );
};
//==========================================================
int main(  ) {
  //string strValue;
   char str[] = { 0 };
   //copyStr = (char*)malloc(strValue.length() + 1);
   printf("Enter a string for convertion : ");
   //system("pause");
   scanf("%s",&str );
   //strcpy(copyStr, strValue.c_str());
   charToBinary( str );
   printf("\n");
  //free( copyStr );
   system("pause");
}

Can anyone help me please???

Recommended Answers

All 9 Replies

>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 <string.h>, 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 <stdlib.h>. 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;
}
commented: Always a breath of fresh air through mouldy code :) +20

>Hello guys this is my code.
It's not often that looking at code can leave me speechless.

:icon_biggrin: Glad of your prompt recovery, otherwise, I can not fathom what a post of yours, in full bloom speech mode would have meant. :P

>int* cVec = (int*)malloc ( alloc );
You strike me as a C++ programmer[...]

Even when they are commented these:

//copyStr = (char*)malloc(strValue.length() + 1);
//strcpy(copyStr, strValue.c_str());

would remove any doubt concerning it.
C doesn't support method calls to objects.

>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.

I would say, the first clue is using namespace std; Which it works in C++ but not in C. fputs ( "The typed string into Ascii mode: ", stdout ); would need a followed call to fflush(stdout); for consistency.

I would like to ask a question if I may.

/* Trim the newline character */
    str[strcspn ( str, "\n" )] = '\0';

How would that call to strcspn() compare in over head and speed, to the classic:

size_t len = strlen(string) -1;
if ( string[len] == '\n')
    string[len] = '\0';

Ok Ok stop shotting me.if you read the comments below you will make a more complete view about my thought.I use borland C++ 5.02 so that the reason whose i use syntax of C++ as first and other functions like system ( "pause" ); etc as second...The problem is the compatibility to the prototype ANSI.I'm professional computer programmer on army systems so i didn't use to make so make bullshits as you think....

Ok Ok stop shotting me[...]

A simple thank you, would have been sufficient. She's not "shotting you". You should be grateful that Narue took the time to point in detail how you could improve your code. No every one gets that honor.

>I can not fathom what a post of yours, in full bloom speech mode would have meant.
You should have seen some of my diatribes on cprog. ;)

>I would say, the first clue is using namespace std; Which it works in C++ but not in C.
I was working under the assumption that the code was originally written in C++ and being ported to C by someone who isn't terribly familiar with C.

>fputs ( "The typed string into Ascii mode: ", stdout ); would
>need a followed call to fflush(stdout); for consistency.
That's unnecessary. fflush is only needed when one wants to explicitly flush the stream immediately. The most common cases for this are error logging and user prompts. For output that doesn't fall under the real-time requirement (as in this case), there's no need to flush right away. It would also be largely redundant as there's a call to puts after the loop that ends up flushing due to the printing of newlines, and a putchar at the end of the function that flushes as well.

>How would that call to strcspn() compare in over head and speed, to the classic:
It's invariably slower.

@SiRuS

>Ok Ok stop shotting me.
I took quite a bit of time out of vastly more important things to break down your code, show you what was wrong, and describe in detail why it was wrong as well as how to fix it. If you can't see that as far more help than you deserve, you're welcome to piss off.

>if you read the comments below you will make a more complete view about my thought.
I don't care about your "thought", only your code. Crappy code is indefensible, so you'd be wise to accept constructive criticism with grace and gratitude.

>I use borland C++ 5.02 so that the reason whose i use syntax
>of C++ as first and other functions like system ( "pause" ); etc
Borland C++ 5.02 compiles straight C code quite nicely. Your excuse is irrelevant.

>The problem is the compatibility to the prototype ANSI.
Hardly. Borland C++ 5.02 was released in 1997. C was standardized in 1989 and updated in 1995. By 1997 any C++ compiler not sufficiently conforming to C89 would be immediately ostracized by the programming community because failure to conform to C89 quite often results in broken C++ as well.

>I'm professional computer programmer on army systems
That's a frightening thought.

>so i didn't use to make so make bullshits as you think....
I recommend taking the time to write well. Even if English is not your native language, it's not difficult to realize that this statement is nonsensical.

Well, i believe that a simple thanks is enough for you and for your try.You make me mad due to yours behavior.I didn't say that you have got enough knowledge,but i believe and thats the start of my continue in this forum that you should be more polite to the others.

Well, i believe that a simple thanks is enough for you and for your try.You make me mad due to yours behavior.I didn't say that you have got enough knowledge,but i believe and thats the start of my continue in this forum that you should be more polite to the others.

If you can't stand the heat then get out of the kitchen. That means -- take constructive criticism for what its worth. When you post crappy C code that has no chance to compile on any C compiler then you have to expect unfavorable criticism.

Don't get mad at Narue -- you should be mad at yourself for doing such a poor job at writing C code.

>Well, i believe that a simple thanks is enough for you and for your try.
Yet given two opportunities, you've failed to offer even that. Your first reply was a whiny flame and your second dismisses my effort as a "try" before proceeding to focus completely on my behavior (which was quite mild and objective).

>You make me mad due to yours behavior.
Then you should be absolutely furious due to your behavior. Let's look at my behavior, if that's what you want, and hopefully you'll see that your response was completely out of line. Here's the only possible statement I made that could be seen as rude by any measure before you decided to make an ass of yourself:

It's not often that looking at code can leave me speechless.

Given the many errors and issues I then objectively pointed out, this statement is one of fact about the code and clearly not aimed at you as the programmer. If you took offense, that's your problem, but keep in mind that treating my legitimate help as an attack means that you are at fault in terms of rudeness.

I won't demand an apology, because I honestly don't care about winning or losing an argument against someone such as yourself, but I will strongly recommend that you consider your behavior and adjust your attitude before posting again. Not because I'm a moderator, as the rules themselves dictate how you must act on Daniweb (I don't put on my moderator hat except to enforce the written rules), but because the C community here expects a certain level of maturity. If you fail to reach that level, you'll likely find that your stay here is both unpleasant and unproductive.

Thread closed.

Be a part of the DaniWeb community

We're a friendly, industry-focused community of developers, IT pros, digital marketers, and technology enthusiasts meeting, networking, learning, and sharing knowledge.