Please, can someone tell me what is the problem in my syntax. I want to find the duplicate letters in a word. it is working properly if I declare a character array here itself but not working with scanf.

#include<stdio.h>

// Finding the duplicate alphabets in a string

int length(char str[]) //Finding the length of the string
{
    int len;
    while(str[len]!='\0')
    {
        len  ;
    }
    return len;
}
void duplicate(char str[],int n)
{
    int i,j,flag;
    for(i=0;i<=n-2;i  ) //Selecting the alphabet for comparison
    {
        flag=0;
        if(str[i]!='\0')
        {
            for(j=i 1;j<=n-1;j  ) //comparison of alphabets
            {
                if(str[j]==str[i])
                {
                    flag=1;
                    str[j]=0;
                }
            }
            if(flag==1)
            {
                printf("%c is the duplicate character\n",str[i]);
            }   
            
        }        
        
    } 
}
int main()
{
    char str[100];
    scanf("%s",&str);
    int n= length(str);
    duplicate(str,n);
}

CodePudding user response：

The problems that I noticed:

1. main: scanf("%s",&str); is the wrong type for str (char (*)[100]) and should be scanf("%s", str);. char str[100] uses a magic 100 value, instead #define STR_LEN 100 so you can do char str[STR_LEN 1]. The way you use scanf is subject to buffer overflow instead you should use scanf("%" str(STR_LEN) "s", STR_LEN, str) and you need #define str(s) xstr(s) and #define xstr(s) #s. I suggest using fgets instead.

2. length: int len; is uninitialized and should be int len = 0; (len is not a great variable name as it's usually 1 bigger than last index, but you use it to index with). Why did you write your own instead of using strlen? As you only return values 0 or great, consider using unsigned instead of int for the type of i and the return value.

3. duplicate (minor issue): it's good practice to minimize variable scope so for(int i = 0; ... and declare flags where you initilize it. You should technically ensure that n > INT_MIN 1 for underflow, or change type to an unsigned value, or just calculate it yourself internally.

You can also create an array of counts for each letter. Initialized to 0, and add 1 as you find each letter. Then report the letters with count > 1. This would be a O(n) algorithm instead of the original O(n^2).

#include <limits.h>

#define CHARS (UCHAR_MAX 1)

void duplicate(char *str) {
    unsigned char counts[CHARS] = { 0 }; // 0, 1 or 2 for 2 
    for(unsigned i=0; str[i]; i  ) {
        char *c = counts   (unsigned) str[i];
        *c  = *c <= 1;
    }
    for(unsigned i=0; i<CHARS; i  ) {
        if(counts[i] > 1) {
            printf("%c is the duplicate character\n", (char) i);
        }
    }
}

CodePudding user response：

As advised, always ensure that your local variables are initialized before attempting to use them. As for your problem, if your sole desire is to find duplicate strings then you could approach it this way:-

#include<stdio.h>
#define CHAR_SIZE 100
int length(char[]);
void duplicate(char[], int);

// Finding the duplicate alphabets in a string
int length(char str[]) //Finding the length of the string
{
    int len = 0;
    while(str[len] !='\0')
        len  ;
    return len;
}

//find duplicates
void duplicate(char str[],int n)
{
    for(int i =0; i < n; i  )
        for(int j=i 1; j<n; j  )
            if(str[i] == str[j])
                printf("%c is the duplicate character\n",str[i]);
}

//test case
int main()
{
    char str[CHAR_SIZE];
    puts("Enter string\n");
    scanf("%s",str);
    int n= length(str);
    printf("len of entered str is %d\n\n", n);
    duplicate(str,n);
}