1

I am working on a code which requires a function. This function gets a string as input and returns a string.

What I have planned so far is to get a str[], remove all $'s and spaces, and store this in another string which is returned later:

char *getstring(char str[])
{
    int i=0;
    char rtn[255];
    while (i<strlen(str))
    {
       if (str[i] != " " || str[i] != "$" )
             rtn[i] = str[i];
       else
             rtn[i] = '';
    } 
    return str;
}

I dont feel like this will work. Any ideas?? :-S

Improve this question

4 Answers 4

Reset to default
2

Problem #1:
Your function returns a pointer to a (temporary) string which is allocated on the stack. This means that once your function ends, the memory for rtn is released, and this invalidates the char * pointer that the function returns.

What you would rather do is:

void getstring(char str[], char rtn[])
{
    /* Some processing on rtn... */
}

The caller to getstring should handle the allocation and deallocation of the string passed as rtn.

Problem #2:
Your while loop will run indefinitely, because i never increases. Put i++ somewhere inside the loop.

Problem #3:
The condition in your if-statement is faulty. You compare str[i], which is a char, to " " or "$", which are string literals (having '\0' in the end). This is wrong.
You need to compare them to chars, which are indicated by apostrophes (not quotation marks).
Also, note that the test condition is wrong as well. You need a logical AND operator instead of OR.
Change the if-statement to:

if (str[i] != ' ' && str[i] != '$')

Problem #4:
What does rtn[i] = ''; mean? '' is an empty character constant, which is illegal in C.
Did you want to just skip a character in str?

Problem #5:
You have indexation problems. Because str and rtn may obviously be of different length, you need to manage two running indices, one for each string.

Problem #6:
rtn is not necessarily null-terminated when the function returns. Assign '\0' to the end of rtn before the function returns (i.e. rtn[i] = '\0'; after the while-loop ends).

Here's your code with all the problems mentioned above fixed:

void getstring(char str[], char rtn[])
{
    int i = 0, j = 0;
    while (i < strlen(str))
    {
       if (str[i] != ' ' && str[i] != '$')
             rtn[j++] = str[i];
       i++;
    }
    rtn[j] = '\0';
}

And here's a more efficient version that uses pointers instead of indices, and doesn't use strlen:

void getstring(char *str, char *rtn)
{
    while (*str)
    {
       if (*str != ' ' && *str != '$')
             *rtn++ = *str;
       *str++;
    }
    *rtn = '\0';
}
Improve this answer
Sign up to request clarification or add additional context in comments.

4 Comments

@C0de_Hard I've added some more problems that I've found :) Regarding the malloc, I'd rather not do it. It is recommended that a caller should manage the memory-resources, otherwise you might get spaghetti code and many bugs to follow.
"What does rtn[i] = ''; mean? '' is an empty character constant, which is illegal in C. Did you want to just skip a character in str?" Yes you were right!! I just want to skip that character!
So here is my question.. when i try to just skip $ and spaces this logic works very fine :) But the below fails... I work in UNIX platform. I want to remove those ^M characters using this. When this goes from ^A - ^Z this doesn't seem to work the same. Any idea? any sort of help is appreciated! :)
@Code_Hard I believe that ^M in Unix is the carridge return, and you can test it with if str[i] == '\r' in C. Also, I suggest you look into the dos2unix command.
1

It definitely cannot work. You're not incrementing the 'i' counter and the assignment of '' does not skip symbols.

The in-place variant (not an optimal in speed terms)

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

void getstring(char* str)
{
    int j, i = 0, len = strlen(str);

    while(i < len)
    {
        char ch = str[i];

        if(ch == ' ' || ch == '$')
        {
            /// shift by one
            for(j = i ; j < len + 1; j++) { str[j] = str[j + 1]; }

            len--;
            i--;
        }

        i++;
    } 

    str[len] = 0;
 }

 int main()
 {
      char test_string[] = "Some string $with$ spaces";

      printf("Src = %s\n", test_string);
      getstring(test_string);
      printf("Res = %s\n", test_string);
      return 0;
 }

And the variant with reallocation

char *getstring(const char* str)
{
    int i_src = 0;
    int i_dest = 0;
    int len = strlen(str);

    char* rtn = (char*)malloc(len + 1);

    while (i_src < len)
    {
       char ch = str[i_src];
       if ( (ch != ' ') && (ch != '$') )
       {
            rtn[i_dest] = ch;
            /// increment destination index here
            i_dest++;
       }
       /// increment the source index always
       i_src++;
    } 

    rtn[i_dest] = 0;

    return rtn;
}

Don't forget to free() the result later.

Improve this answer

4 Comments

Viktor, Yes this is helping me indeed to raise a lot of questions! :) Let me complete and get back to you!
why didn't you return the str in the first type? I don't see this test_string being declared a global!! :(
It's on the stack. And it is faster with alloca. I don't see the problems here unless the string is reeeeally long. It's just a unit-test I've made (in the main function) to check that it works. You ask about a function, not the whole task.
See, I modify the str[] itself. You've asked for in-place version, I did it (my first variant was with the malloc() call, then I added the in-place)
0

Better let the caller supply the destination string (the caller probably knows how large it should be), and return somehing it does not know: the number of bytes written to the rtn[] array.

size_t mygetstring(char *rtn, char *str)
{
    size_t pos, done;

    for (pos=done=0; rtn[done] = str[pos]; pos++)
    {
       if (rtn[done] == ' ' || rtn[done] == '$' ) continue;
       done++;
    } 
    return done; /* number of characters written to rtn[], excluding the NUL byte */
}
Improve this answer

2 Comments

whats size_t?? God!! are you talking in C :O ?
size_t is an unsigned type large enough to contain the size of any object. It is for instance the type of strlen()s return value, or for malloc()s argument. And the type yielded by the sizeof operator.
0

  1. If you want to return new string from a function, you should dynamically allocate memory for the string. In your codechar rtn[255]; you allocate memory for rtn on the stack, it will be wiped out after exiting from a function. You should write:

    char *rtn = (char *) malloc(strlen(str));

    Of course you should keep tracking on memory to prevent memory leaks and call free() to release memory.

  2. Assignment of '' does not skip symbol, you should not increment i when you see undesirable symbol (and you have missed the increment in your code).

  3. You are returning the wrong value, you should return rtn instead of str.

The correct code will be like:

char *getstring(char str[])
{
    int i=0, rtn_pos = 0;
    size_t len = strlen(str);
    char *rtn = (char*) malloc(len + 1);
    for (i = 0; i < len; ++i)
       if (str[i] != ' ' && str[i] != '$' )
             rtn[rtn_pos++] = str[i];
    rtn[rtn_pos] = '\0';
    return rtn;
}
Improve this answer

7 Comments

Use single quotes when comparing a character literal.
and use malloc(strlen(str) + 1) not only strlen(str)
ouah, thanks for pointing to missing memory byte for '\0'! But it is better to leave malloc casting. Yes, there is no need to cast from malloc in std C, but if code will be compiled as C++ it will be an error.
The strlen() in the loop is still ugly: the length of the string only needs to be taken once. Also: a for() loop is less accident-prone than the loose ++i; at the end of the while loop body.
wildplasser, you are completely right. I'll take your notes into account.
|

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.