CS50 Week 2: Converting vowels to numbers and outputting new string

Question

Problem: User inputs a word or phrase as command-line argument and a new word is printed with some vowels replaced by numbers.

Question: How do I improve my code design and style? Is it possible to not have to repeat x[i] == '...' in my if statement?

#include <cs50.h>
#include <stdio.h>
#include <string.h>
string replace(string x);
int main(int argc, string argv[])
{
 if (argc != 2) // if user does not input 2 words, then print 'ERROR' message
 {
 printf("ERROR\n");
 return 1;
 }
 else
 {
 replace(argv[1]);
 printf("\n");
 }
}
string replace(string x)
{
 for(int i = 0, c = strlen(x); i < c; i++) //for each character 'i', do the following until the position of character is less than the length of the string
 {
 if (x[i] == 'a' || x[i] == 'e' || x[i] == 'i' || x[i] == 'o')
 {
 switch (x[i])
 {
 case 'a':
 printf("6");
 break;
 case 'e':
 printf("3");
 break;
 case 'i':
 printf("1");
 break;
 case 'o':
 printf("0");
 break;
 default:
 printf("%s\n", x);
 }
 }
 else //if not a vowel as above, print same character
 {
 printf("%c", x[i]);
 }
 }
 return 0;
}

Answer 1

We don't need to forward-declare replace() if we define it before main(). That saves having to keep the two signatures maintained in parallel. As a general guide, put main() last in your program, and the lowest-level functions first.

---

We can improve the error handling:

#include <stdlib.h>

 fprintf(stderr, "Usage: %s <string>\n", argv[0]);
 return EXIT_FAILURE;

---

replace() returns a string but we don't use that. I would expect something like

 printf("%s\n", replace(argv[1]));

Because the replace() function has two responsibilities (transforming the input and printing it), it's less reusable than a function that just does one thing.

---

We have a type mismatch in int c = strlen(x). The function returns a size_t, so we should make c the same type. Instead of using strlen(), consider using a pointer that advances until it reaches a null character.

---

We don't need if/else around the switch. Use the default: label for the non-matching letters.

---

Modified code

Applying my improvements (and using ordinary char* instead of CS50 string):

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
char *replace(char *const s)
{
 for (char *p = s; *p; ++p) {
 switch (*p) {
 case 'a':
 *p = '6';
 break;
 case 'e':
 *p = '3';
 break;
 case 'i':
 *p = '1';
 break;
 case 'o':
 *p = '0';
 break;
 /* default: no change */
 }
 }
 return s;
}
int main(int argc, char *argv[])
{
 if (argc != 2)
 {
 fprintf(stderr, "Usage: %s <string>\n", argv[0]);
 return EXIT_FAILURE;
 } else {
 printf("%s\n", replace(argv[1]));
 }
}

• Junyu: But in cs50 week2, the course haven't introduce pointer yet.