Error with manually copying string using pointers

Question

I am creating this program as part of an assignment for college. The objective is to copy char* slogan = "Comp10120 is my favourite module"; to a new string while removing consonants and capitalising all letters. This is my code:

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

void printStrings();

char *slogan = "Comp10120 is my favourite module";
char *p = slogan;
char *slogan_copy = NULL;

int main ()
{   
    //Get size of original string
    int slogan_size = 0;
    while (*p++ != '\0')
        slogan_size++;

    // Dynamically allocate memory to copy of string
    slogan_copy = (char*) malloc ((slogan_size+1) * sizeof(char));
    //Place string terminator at end of copy string
    slogan_copy[slogan_size] = '\0';

    //Reset p pointer to start of string
    p = slogan;
    int offset = 0;

    while (*p != '\0')
    {
        //If the current element in the string is a consonant,
        //or as defined in the if statement,
        //if p is not a vowel and is between a and z or A and Z:
        if ((!(*p == 'a' || *p == 'e' || *p == 'i' || *p == 'o' || *p == 'u')) 
            && (((*p > 'a') && (*p < 'z')) || ((*p > 'A') && (*p < 'Z'))))
            p++;
        else
            //Copy element to slogan_copy and capitalise
            slogan_copy[offset++] = *p++;
            slogan_copy[offset] = toupper(slogan_copy[offset]);
    }

    //Place string terminator after last element copied.
    slogan_copy[offset] = '\0';

    printStrings();

    return 0;
}

void printStrings ()
{
    printf("Origianl String: %s\n",*slogan);
    printf("Modified String: %s",*slogan_copy);
}

When I try to execute, I get the error

initializer element is not constant
 char *p = slogan;
           ^~~~~~

I am assuming that it is because I am trying to perform operations on slogan as if it was just a regular array of characters, and not a pointer to a string. However, I don't know how to fix this error.

In addition to this, I tried changing char*slogan = "Comp10120 is my favourite module"; to char slogan[] = "Comp10120 is my favourite module"; to see if it would work, out of curiosity. It complies, but crashes upon execution. Any ideas as to how I could modify my code for it to work?

Answer 1

Your code is not indented correctly: the else branch has 2 statements but they are not wrapped inside a block with { and }, so only the first statement in executed conditionally and the second is always executed, causing unexpected behavior regarding the uppercasing feature.

Furthermore, the uppercasing is not applied to the correct offset as offset is incremented too early, and uppercase vowels would be removed too.

A sane rule for coding style is to always use braces for all compound statements but the simplest ones. Rewrite the test this way:

    int c = toupper((unsigned char)*p++);
    if (!isalpha(c) || c == 'A' || c == 'E' || c == 'I' || c == 'O' || c == 'U') {
        //Copy and capitalise element to slogan_copy
        slogan_copy[offset++] = c;
    }

There are other problems in the code, eg: passing incorrect data for the printf arguments, and using global variables for no good reason.

Here is an improved version:

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

int main() {   
    const char *slogan = "Comp10120 is my favourite module";
    char *slogan_copy = NULL;

    //Get size of original string
    int slogan_size = 0;
    while (slogan[slogan_size] != '\0')
        slogan_size++;

    // Dynamically allocate memory to copy of string
    slogan_copy = malloc(slogan_size + 1);

    //Reset p pointer to start of string
    const char *p = slogan;
    int offset = 0;

    while (*p != '\0') {
        int c = toupper((unsigned char)*p++);
        if (!isalpha(c) || c == 'A' || c == 'E' || c == 'I' || c == 'O' || c == 'U') {
            //Copy and capitalise element to slogan_copy
            slogan_copy[offset++] = c;
        }
    }

    //Place string terminator after last element copied.
    slogan_copy[offset] = '\0';

    printf("Original string: %s\n", slogan);
    printf("Modified string: %s\n", slogan_copy);

    return 0;
}

Answer 2

As per your request in a comment here is a simplified and correct version:

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

int main()
{
  const char *slogan = "Comp10120 is my favourite module";

  // Dynamically allocate memory to copy of string
  char *slogan_copy = malloc((strlen(slogan) + 1) * sizeof(char));

  //Reset p pointer to start of string
  const char *p = slogan;
  int offset = 0;

  while (*p != '\0')
  {
    //If the current element in the string is a consonant,
    //or as defined in the if statement,
    //if p is not a vowel and is between a and z or A and Z:
    char c = toupper(*p++);
    if (!isalpha(c) || c == 'A' || c == 'E' || c == 'I' || c == 'O' || c == 'U' || )
      slogan_copy[offset++] = c;
  }

  //Place string terminator after last element copied.
  slogan_copy[offset] = '\0';

  printf("Origianl String: %s\n", slogan);
  printf("Modified String: %s\n", slogan_copy);

  return 0;
}

Output:

Origianl String: Comp10120 is my favourite module
Modified String: O10120 I  AOUIE OUE

Answer 3

A few improvements are needed.

1) In the function printf format %s expects pointer to the buffer. There is no need to dereference slogan or slogan_copy.

2)

slogan_copy[offset++] = *p++;
slogan_copy[offset] = toupper(slogan_copy[offset]);

The above will not work. It will make the next character upper not the current.

3) In C language there is no need to cast malloc.

4) Global variables should be avoided at all cost. The break encapsulation. Pass variables as a parameters, you will get greater flexibility.

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

void printStrings (const char *format, const char *s);

int main (void)
{   
    char *slogan = "Comp10120 is my favourite module";
    char *p = slogan;
    char *slogan_copy;
    int offset = 0;
    int slogan_size = 0;

    //Get size of original string

    while (*p++ != '\0')
        slogan_size++;

    // Dynamically allocate memory to copy of string

    slogan_copy = malloc ((slogan_size+1) * sizeof(char));

    //Place string terminator at end of copy string
    slogan_copy[slogan_size] = '\0';

    //Reset p pointer to start of string
    p = slogan;

    while (*p != '\0')
    {
        //If the current element in the string is a consonant,
        //or as defined in the if statement,
        //if p is not a vowel and is between a and z or A and Z:
        if ((!(*p == 'a' || *p == 'e' || *p == 'i' || *p == 'o' || *p == 'u')) 
            && (((*p > 'a') && (*p < 'z')) || ((*p > 'A') && (*p < 'Z'))))
            p++;
        else{
            //Copy element to slogan_copy and capitalise
            slogan_copy[offset] = *p;
            slogan_copy[offset] = toupper(slogan_copy[offset]);

            *p++;
            offset++;
        }
    }

    //Place string terminator after last element copied.
    slogan_copy[offset] = '\0';

    printStrings("Origianl String: %s\n", slogan);
    printStrings("Modified String: %s\n", slogan_copy);

    return 0;
}

void printStrings (const char *format, const char *s)
{
    printf(format,s);
}

Output:

Origianl String: Comp10120 is my favourite module                                                                                            
Modified String: O10120 I  AOUIE OUE  

Answer 4

you have quite a lot of mistakes in your program. do consider your use of global variables and consider using const where it is needed, However it is a preety good starting point so I have tested your program and it seems to work with 4 simple corrections:

1.remove p initialazation in the global env

8: //char *p = slogan;
9: char *p;

2. set p within main block

   int main () { p = slogan; ... }

3. remove the astrix from the slogan in your printf statments it is already a pointer to a char array

   printf("Origianl String: %s\n",slogan); printf("Modified String: %s",slogan_copy);

hope this helps