Why my returned value of strchr is ignored?

Question

I have to make a function, that will code my sentence like this: I want to code all words with an o, so for example I love ice cream becomes I **** ice cream.

But my function ignores the result of strchr. And I don't know why.

This is my code:

#define _CRT_SECURE_NO_WARNINGS
#include <stdio.h>
#include <stdlib.h>

#define LEN 1000

char *Shift(char *str, char *let) {
    const char *limits = " ,-;+.";
    char copy[LEN];
    strcpy(copy, str);
    char *p;
    char *ptr;
    ptr = strtok(copy, limits);
    for (int j = 0; ptr != NULL; ptr = strtok(NULL, limits), ++j) {
        int len = 0;
        if (strchr(ptr, let) != NULL) {
            p = strstr(str, ptr);
            for (int i = 0; i < strlen(ptr); i++) {
                p[i] = "*";
            }
        }
    }
    return str;
}

int main() {
    char *s = Shift("I love my cocktail", "o");
    puts(s);
}

Expected output is: I **** my ********

but I've got just printed the original string

Answer 1

There are multiple problems:

• copying the string to a fixed length local array char copy[LEN] will cause undefined behavior if the string is longer than LEN-1. You should allocate memory from the heap instead.
• you work on a copy of the string to use strtok to split the words, but you do not use the correct method to identify the parts of the original string to patch.
• you should pass a character to strchr(), not a string.
• patching the string with p[i] = "*" does not work: the address of the string literal "*" is converted to a char and stored into p[i]... this conversion is meaningless: you should use p[i] = '*' instead.
• attempting to modify a string literal has undefined behavior anyway. You should define a modifiable array in main and pass the to Shift.

Here is a corrected version:

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

char *Shift(char *str, char letter) {
    const char *limits = " ,-;+.";
    char *copy = strdup(str);
    char *ptr = strtok(copy, limits);
    while (ptr != NULL) {
        if (strchr(ptr, letter)) {
            while (*ptr != '\0') {
                str[ptr - copy] = '*';
                ptr++;
            }
        }
        ptr = strtok(NULL, limits);
    }
    free(copy);
    return str;
}

int main() {
    char s[] = "I love my cocktail";
    puts(Shift(s, 'o'));
    return 0;
}

The above code still has undefined behavior if the memory cannot be allocated. Here is a modified version that operates in place to avoid this problem:

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

char *Shift(char *str, char letter) {
    char *ptr = str;
    while ((ptr = strchr(ptr, letter)) != NULL) {
        char *p = ptr;
        while (p > str && isalpha((unsigned char)p[-1]))
             *--p = '*';
        while (isalpha((unsigned char)*ptr)
            *ptr++ = '*';
    }
    return str;
}

int main() {
    char s[] = "I love my cocktail";
    puts(Shift(s, 'o'));
    return 0;
}

Note that you can also search for multiple characters at a time use strcspn():

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

char *Shift(char *str, const char *letters) {
    char *ptr = str;
    while (*(ptr += strcspn(ptr, letters)) != '\0') {
        char *p = str;
        while (p > str && isalpha((unsigned char)p[-1]))
            *--p = '*';
        while (isalpha((unsigned char)*ptr)
            *ptr++ = '*';
    }
    return str;
}

int main() {
    char s[] = "I love my Xtabentun cocktail";
    puts(Shift(s, "oOxX"));
    return 0;
}

Answer 2

For starters the function strchr is declared like

char *strchr(const char *s, int c);

that is its second parameter has the type int and the expected argument must represent a character. While you are calling the function passing an object of the type char * that results in undefined behavior

if (strchr(ptr, let) != NULL) {

It seems you mean

if (strchr(ptr, *let) != NULL) {

Also you may not change a string literal. Any attempt to change a string literal results in undefined behavior and this code snippet

        p = strstr(str, ptr);
        for (int i = 0; i < strlen(ptr); i++) {
            p[i] = "*";
        }

tries to change the string literal passed to the function

char *s = Shift("I love my cocktail", "o");

And moreover in this statement

            p[i] = "*";

you are trying to assign a pointer of the type char * to a character. At least you should write

            p[i] = '*';

If you want to change an original string you need to store it in an array and pass to the function the array instead of a string literal. For example

char s[] = "I love my cocktail";
puts( Shift( s, "o" ) );

Pay attention to that there is no great sense to declare the second parameter as having the type char *. Declare its type as char.

Also the function name Shift is confusing. You could name it for example like Hide or something else.

Here is a demonstration program.

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

char * Hide( char *s, char c ) 
{
    const char *delim = " ,-;+.";

    for ( char *p = s += strspn( s, delim ); *p; p += strspn( p, delim ) )
    {
        char *q = p;
        p += strcspn( p, delim );

        char *tmp = q;
        while ( tmp != p && *tmp != c ) ++tmp;

        if ( tmp != p )
        {
            for ( ; q != p; ++q ) *q = '*';
        }

    }

    return s;

}

int main( void ) 
{
    char s[] = "I love my cocktail";

    puts(s);

    puts( Hide( s, 'o' ) );
}

The program output is

I love my cocktail
I **** my ********

The for loop

for ( ; q != p; ++q ) *q = '*';

within the function can be rewritten as a call of memset

memset( q, '*', p - q );