Function that get 3 word for strings, than combine them to 1 string and finally print the string in reverse

Question

I'm learning C and I got a task to make 3 function.

The first get 3 addresses of string, get 3 words from the user, and enter into the addresses.
The second get the 3 addresses of string and combine them to a new large string. The last should get the new address and print in reverse the string.

I wrote this and will happy to hear of problems or things to improve.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#define N 10
void swap(char *x, char *y) {
 char temp;
 temp = *x;
 *x = *y;
 *y = temp;
}
void initWords(char* w1, char* w2, char* w3) {
 printf("Please enter a word\n");
 scanf("%s", w1);
 printf("Please enter another word\n");
 scanf("%s", w2);
 printf("Please enter one last word\n");
 scanf("%9s", w3);
}
char* concatWords(char* w1, char* w2, char* w3) {
 char* new_w = malloc(strlen(w1)+strlen(w2)+strlen(w3));
 strcat(new_w, w1);
 strcat(new_w, w2);
 strcat(new_w, w3);
 return new_w;
 free(new_w);
}
void printReversedWord(char* word) {
 int i;
 int number = strlen(word);
 for (i=0; i<=(number/2); i++) {
 swap(&word[i], &word[number-1-i]);
 }
 printf("%s", word);
}
int main() {
 char w1 [N + 1];
 char w2 [N + 1];
 char w3 [N + 1];
 initWords(w1, w2, w3);
 char* new_w = concatWords(w1, w2, w3);
 printReversedWord(new_w);
 
 return 0;
}

thanks a lot!

Answer 1

• free(new_w) has no effect. It is unreachable. And unnecessary (and dangerous if placed before return).

• malloc(strlen(w1)+strlen(w2)+strlen(w3)) allocates one character less than necessary. There is no room for a zero terminator.

• scanfs make the code prone to the buffer overflow. Prefer fgets.

• I understand it is not a part of assignment, but think of separation of responsibilities. A function that just reverses the word is better than a function which reverses and prints it.

• number is not the greatest name. new_word_length perhaps?

Answer 2

Bug

strcat(s1, s2) requires that s1 point to a string. new_w does not point to a string as it lacks a certain null character.

char* new_w = malloc(strlen(w1)+strlen(w2)+strlen(w3));
strcat(new_w, w1); // Bad

Better as

char* new_w = malloc(strlen(w1)+strlen(w2)+strlen(w3) + 1);
strcpy(new_w, w1);

Avoid Schlemiel the Painter's Algorithm

Instead, retain the length of the strings and copy with that information. Use size_t for string lengths. Test for allocation success.

char* concatWords(const char* w1, const char* w2, const char* w3) {
 size_t l1 = strlen(w1);
 size_t l2 = strlen(w2);
 size_t l3 = strlen(w3);
 char* new_w = malloc(l1 + l2 + l3 + 1); // +1 for the null character.
 if (new_w) {
 strcpy(new_w, w1);
 strcpy(new_w + l1, w2);
 strcpy(new_w + l1 + l2, w3);
 // or 
 memcpy(new_w, w1, l1);
 memcpy(new_w + l1, w2, l2);
 memcpy(new_w + l1 + l2, w3, l3 + 1);
 }
 return new_w;
}

Poor code

Quality code never uses "%s" without a width limit as in scanf("%s",.. );. It is worse than gets().

Advanced solution.

#define STRINGIFY(x) #x
#define TOSTRING(x) STRINGIFY(x)
scanf("%" TOSTRING(N) "s", w1);

... or use fgets() and Removing trailing newline character from fgets() input.

Also check return value of scanf().

Side effect

Goal is "print in reverse the string.", but instead code reverses the string and then prints. This leaves the string reversed as a side-effect. Instead, simply print the string in reverse without changing the string.

void printReversedWord(const char* word) {
 size_t len = strlen(word);
 while (len > 0) {
 printf("%c", word[--len]); // or research putchar()
 }
}