4
\$\begingroup\$

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");
}
\$\endgroup\$
6
\$\begingroup\$
  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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Disagree about the use of std::string - this assignment is a self-contained exercise, and a sequence of characters in memory is fine; the features of std::string are not useful here. \$\endgroup\$ – einpoklum Oct 1 '17 at 7:48
  • 1
    \$\begingroup\$ @einpoklum assignment is worded as "letters of each word in a string". String in C++ is represented by std::string. Where did you get a sequence of characters I do not know. \$\endgroup\$ – Artemy Vysotsky Oct 1 '17 at 7:57
  • 1
    \$\begingroup\$ @einpoklum The size of the string in OP's code is hard coded. With std::string, you don't have that "problem". \$\endgroup\$ – Rakete1111 Oct 1 '17 at 8:13
3
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

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);
}
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.