CODEWITHSUNDEEP
cstring
C0de_Hard
C0de_Hard

Reputation: 72

Function to get a string and return same after processing in C

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

Upvotes: 1

Views: 226

Answers (4)

Eitan T
Eitan T

Reputation: 32920

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';
}

Upvotes: 2

vkorchagin
vkorchagin

Reputation: 656

  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;
}

Upvotes: 0

wildplasser
wildplasser

Reputation: 44240

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 */
}

Upvotes: 0

Viktor Latypov
Viktor Latypov

Reputation: 14467

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.

Upvotes: 1

Related Questions