Confusion C begining

Question

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


int main()
{
 char a[250];
 char c1[1],c2[1];
int n,i;


printf("Give text: ");
gets(a);

printf("Give c1: ");
gets(c1);
  printf("Give c2: ");
 gets(c2);

n=strlen(a);

for(i=0;i<n;i++)
{
    if(a[i]==c1)
 {
      a[i]=c2;
   }
  if(a[i]==c2)
 {
    a[i]=c1;
   }
   }
     printf("%s",a);
return 0;
}

In a text I need to switch c1 with c2 and reverse, but when I start the program after I give a, c1, c2 nothing happened. Where am I wrong?

You shouldn't use `gets()`, which has unavoidable risk of buffer overrun, deprecated in C99 and removed from C11. — MikeCAT, Jun 04 '16 at 16:17
`gets(c1);` with `char c1[1]` is bad because this will only accept zero characters + terminating null character. `a[i]=c2;` and `a[i]=c1;` looks weird because it is assigning what is converted from the pointer converted from the array. — MikeCAT, Jun 04 '16 at 16:18
You want to read on how C-"string" are implemented. A C-"string" defined `char c[1]` can hold nothing more then the "emtpy-string": `""`. — alk, Jun 04 '16 at 16:30
And pleeease, properly indent your code, at least prior presenting it to the world for reading ... :-( — alk, Jun 04 '16 at 16:32
I mean what possessed you to 'char c1[1],c2[1]; ??? Were you a stingy accountant in another life? Shtap with the bean-counting, nd besides, [1] is just silly anyway, as others have pointed out:( — Martin James, Jun 04 '16 at 16:35
Nitpicking: `strlen()` doesn't return a signed integer but an `unsigned`, typically for recent C compilers a `size_t`. — alk, Jun 04 '16 at 16:41

score 2 · Accepted Answer · edited May 23 '17 at 12:22

2

First of all, don't use gets(), it's inherently dangerous, use fgets() instead.

On top of that, when you used gets(c1), c1 being an one-element array, you already overrun the allocated memory which invokes undefined behavior.

That said, you have c1 and c2 as one-element arrays, which are not wrong but neither required. Define them as simple char variables

char c1;
char c2;

and use them like

 scanf(" %c", &c1);  // mind the space and don't forget to to check the return 
 scanf(" %c", &c2);  // value of scanf() to ensure proper scanning.

After that, the check for a[i] == c2 should come as else construct, otherwise, you'll be overwriting the previous operation. Something like

for(i=0;i<n;i++)
{
    if(a[i]==c1)
   {
      a[i]=c2;
   }
  else if(a[i]==c2)
   {
    a[i]=c1;
   }
}

edited May 23 '17 at 12:22

Community

1
1

answered Jun 04 '16 at 16:20

Sourav Ghosh

133,132
16
183
261

As it stands `&c1` evaluates to `char**`. `"%c"` expects a `char*`. – alk Jun 04 '16 at 16:33
@alk erm... din't I mention `define them as simple char variables and use them like` already? :) – Sourav Ghosh Jun 04 '16 at 17:14
Indeed, you did. Please excuse ... :} – alk Jun 04 '16 at 17:18

score 0 · Answer 2 · answered Jun 04 '16 at 16:26

gets() shouldn't be used because it has unavoidable risk of buffer overrun, is deprecated in C99 and is removed from C11.
c1 and c2 have insufficient buffer size.
You should check if readings are successful.
You should dereference c1 and c2 before compareing to retrieve characters read.
You should use else if, or the modified character will be modified again.

Try this:

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

/* this is a simple implementation and the buffer for \n is wasted */
char* safer_gets(char* buf, size_t size){
  char* lf;
  if (fgets(buf, size, stdin) == NULL) return NULL;
  if ((lf = strchr(buf, '\n')) != NULL) *lf = '\0';
  return buf;
}

int main()
{
  char a[250];
  char c1[4],c2[4]; /* need at least 3 elements due to the inefficient implementation of safer_gets */
  int n,i;


  printf("Give text: ");
  if(safer_gets(a, sizeof(a)) == NULL)
  {
    fputs("read a error\n", stderr);
    return 1;
  }

  printf("Give c1: ");
  if(safer_gets(c1, sizeof(c1)) == NULL)
  {
    fputs("read c1 error\n", stderr);
    return 1;
  }
  printf("Give c2: ");
  if(safer_gets(c2, sizeof(c2)) == NULL)
  {
    fputs("read c2 error\n", stderr);
    return 1;
  }

  n=strlen(a);

  for(i=0;i<n;i++)
  {
    if(a[i]==*c1)
    {
      a[i]=*c2;
    }
    else if(a[i]==*c2)
    {
      a[i]=*c1;
    }
  }
  printf("%s",a);
  return 0;
}

Why `c[4]`? One for the `char`, one for the `\n` and one for the `0`-terminator gives 3. :-S Is the 4th the famous Angst-Byte? ;-) — alk, Jun 04 '16 at 16:36
And hmhm, and I doubt that seeing `*c1` makes things easier to understand then just finding a `c[0]`... — alk, Jun 04 '16 at 16:39

Confusion C begining

2 Answers2