String reversal function

Question

I have an assignment to create a function that reverses the letters of each word in a string individually. I feel like this should be a rather simple task. I did figure out a solution, although I feel like it uses too many variables as a sort of hack method. Any suggestions on how to make it more efficient/shorter?

#include<iostream>
#include<conio.h>
#include<string.h>

using namespace std;

void rev(char str[80])
{
char t;
int l = 0, n = strlen(str), k = 0;

for (int i = 0; i <= n; i++)
{
    if (str[i] == ' ' || str[i] == '\0')
    {
        for (int j = i - 1; j > (k + i - 1) / 2; l++, j--)
        {
            t = str[j];
            str[j] = str[l];
            str[l] = t;
        }
        k = i + 1;
        l = k;
    }
}

}

void main()
{
    char str[80];
    cout << "Please enter a string : ";
    cin.getline(str, 80);
    cout << "The original string is : " << str << "\n";
    rev(str);
    cout << "The new string is : " << str << "\n";
    system("pause");
}

Answer 1

1. main should be int main()

2. Why C-style strings? Use std::string

3. One function - one task. You are mixing 2 things into one function:

   • Reverse chars in a word
   • Find words in a sentence separated by spaces

   I feel like your problem with too many variables will resolve when you split the tasks you are doing into separate functions.

4. Use better code formatting (auto-format).

5. Type mixing - strlen's return value is std::size_t. Do not assign to int. As a follow-up, use a loop index of type std::size_t to match.

6. This is my preference - not using using namespace std; and system("pause");. I think a lot of posts could be found explaining why.

Answer 2

I'm just going to assume you cannot use std::string at all here, otherwise the code would be much shorter for learning purposes.

• If you have to use a fixed-size C-string, then you should at least make it much larger so that the input string won't likely overflow. If that were to happen here, it would cause a buffer overflow. Otherwise, use a dynamically-allocated array to hold the correct size of the input string, while remembering to deallocate the array afterwards.

  char* str = new char[80];
  
  // reallocate and/or use as needed
  
  delete [] str;
  

• I don't think you need <conio.h>, especially in C++, so just remove it.

• It's usually preferred to put a space between #include and whatever is being included.

• This is not so clear at a glance:

  int l = 0, n = strlen(str), k = 0;
  

  It's more preferred to have each variable declared or initialized on separate lines:

  int l = 0;
  int n = strlen(str);
  int k = 0;
  

• Do not use single-character variable names except for loop counter variables. It could be very hard for others to tell what they're used for, and even you may end up forgetting this after a while. If you're trying to achieve shorter but still good code, then this isn't really the way to do it.

• Instead of that manual swap code, you can just use std::swap():

  std::swap(str[1], str[j]);
  

• Do not use system("PAUSE") to pause. It's not portable and uses more resources. Instead, use something else that still acts as a pause, such as:

  std::cin.get();
  

  One difference is that this requires a character to be entered as opposed to proceeding right after the next keypress.

Answer 3

I suggest you separate the word reversal from the actual algorithm:

void reverse_string(char* start, char* end)
{
    char tmp;

    for (--end; start < end; ++start, --end)
    {
        tmp = *start;
        *start = *end;
        *end = tmp;
    }
}

void reverse_words(char* str)
{
    char* end_of_string = str + strlen(str);
    char* start_of_word = str;
    char* end_of_word = strchr(str, ' ');

    while (end_of_word)
    {
        reverse_string(start_of_word, end_of_word);
        start_of_word = end_of_word + 1;
        end_of_word = strchr(start_of_word, ' ');
    }

    reverse_string(start_of_word, end_of_string);
}