What's wrong with the for loops in this code?

Question

Im reading the book The C Programming Language, and I cant figure out what is wrong with my code in this exercise.

The exercise asks for you to implement a version of the squeeze() function that deletes each character in s1 that matches with any character in the string s2.

Here is my code:

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

int main ()
{
    char s1[100];
    char s2[100];
    int c;
    int i;
    int j;
    int flag=0;
    char s3[100];

    printf("TYPE THE FIRST STRING\n");
    for(i=0;s1[i-1]!='\n';i++)
    {
        scanf("%c", &s1[i]);
    }


    printf("\n\nTYPE THE SECOND STRING\n");
    for(i=0;s2[i-1]!='\n';i++)
    {
        scanf("%c", &s2[i]);
    }


    for(i=0;s1[i]!='\n';i++)/*sets the "i" character of s1 to be compared*/
    {
        for(j=0;s2[j]!='\n';j++)/*compares the "i" character in s1 with
        {                         every character on s2*/
            if(s1[i]==s2[j])
            {
                flag=1;/*sets the flag variable to one if it finds 2 equal 
                break;   characters and stops the loop*/
            }
        }
        if(flag!=1)/*if it have not found a matching pair of chacters,
        {            this part copies the s1 char to s3*/
            s3[i]=s1[i];
        }
        flag=0;
        printf("\n");
    }
    printf("\n\n%s",s3);

    return 0;
}

The problem is that if I input "test" in s1 and "second" in s2, it only prints "t". It stops as soon it finds a matching pair. The flag is not properly working ? I'm 2 hours+ in this exercise and cannot solve it.

Answer 1

for (i = 0; s1[i-1] != '\n'; i++)

The predicate on the first iteration of this loop will access s1[-1], which is out of bounds, and in turn invokes undefined behavior. The loop involving s2 has the same issue.

Additionally, your strings are never NUL terminated, and their buffers are vulnerable to overflowing via scanf.

You have multiple instances of /* */ comments spanning multiple lines, which is commenting out sections of your code: two opening {, and the break.

Even after fixing the issue with the comments

if (flag != 1) {
    s3[i] = s1[i];
}

is going to leave holes in your final string. You'll need a separate index counter for s3 that only increases as the string grows in length.

A quick example where we initialize our buffers (in particular this NUL terminates s3), and use fgets to limit our input.

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

int main(void) {
    char s1[100] = { 0 },
         s2[100] = { 0 },
         s3[100] = { 0 };
    int flag = 0;
    size_t k = 0;

    if (
        !fgets(s1, sizeof s1, stdin) ||
        !fgets(s2, sizeof s2, stdin)
    ) {
        fprintf(stderr, "Could not read input.\n");
        return EXIT_FAILURE;
    }

    for (size_t i = 0; s1[i] && s1[i] != '\n'; i++) {
        for (size_t j = 0; s2[j] && s2[j] != '\n'; j++) {
            if (s1[i] == s2[j]) {
                flag = 1;
                break;
            }
        }

        if (flag == 0)
            s3[k++] = s1[i];
        flag = 0;
    }

    printf("%s\n", s3);
}

---

Alternatively, using strchr, we can compose a tidy function:

#include <string.h>

void squeeze(char *dest, const char *source, const char *filter) {
    for (; *source; source++)
        if (!strchr(filter, *source))
            *dest++ = *source;

    *dest = '\0';
}