3
\$\begingroup\$

Below code reverse a word given by user input example.

input : This is code Review

output : siht si edoc weiveR

steps

step 1: check for whitespace if found set the start and end position and pass to reverseword

step 2: check for null terminator and set start and end position pass to reverseword

#include<stdio.h>
#include <ctype.h>
#define MAX 100
#define SPACE 1
#define NEXT 1
#define MIN_CHAR 1
void  reverseword(int start,int end,char *input)
{
        char temp;
        int i;
        while(end > start)
        {
                temp=input[start];
                input[start]=input[end];
                input[end]= temp;
                start++;
                end--;
        }
}

int main()
{
        char input[MAX];
        scanf("%[^\n]",input);
        int pos;
        int start=0,end=0;
        for(pos=0 ; input[pos] ; ++pos)
        {
                if(isspace(input[pos]) && pos>MIN_CHAR) // case when space is found
                {
                        end = pos-SPACE;
                        reverseword(start,end,input);
                        start= pos+SPACE;
                }
                if(input[pos+NEXT]=='\0' && pos>MIN_CHAR) // case for last world
                {
                        end =pos;
                        reverseword(start,end,input);
                }
        }
}
\$\endgroup\$
4
\$\begingroup\$

Enable warnings

The -Wall on GCC leads to:

reverse.c:10:13: warning: unused variable ‘i’ [-Wunused-variable]

Do not repeat yourself

You check pos > MIN_CHAR in two places. This could be done only once:

    if (pos > MIN_CHAR)
    {
        if(isspace(input[pos])) // case when space is found
        {
            end = pos-SPACE;
            reverseword(start,end,input);
            start= pos+SPACE;
        }
        if(input[pos+NEXT]=='\0') // case for last world
        {
            end =pos;
            reverseword(start,end,input);
        }
    }

Be consistent in your code style

The code style seems a bit inconsistent. For instance, sometimes you have whitespaces before "=", sometimes after, sometimes both, sometimes none... You'll find various coding styles online and different tools to check you code compliancy.

Declare variables in smallest possible scope

I find it a good habit to define variable in the smallest scope in order to make things easier to track. In your case, temp could be declared inside the loop.

Getting rid of the your #define-d values

All your values (except for MAX) could be removed because the link between the name and the value is hard to understand at first.:w

So far, I've got:

#include<stdio.h>
#include <ctype.h>
#include <string.h>
#define MAX 100

void reverseword(int start, int end, char *input)
{
    while (end > start)
    {
        char temp = input[start];
        input[start] = input[end];
        input[end] = temp;
        start++;
        end--;
    }
}

int main()
{
    // Updated for testing purposes
    //        char input[MAX];
    //        scanf("%[^\n]",input);
    char * original = "this is code review";
    char * input = strdup(original);
    printf("%s\n", input);
    int pos;
    int start = 0;
    for (pos = 0 ; input[pos] ; ++pos)
    {
        if (pos > 1)
        {
            if (isspace(input[pos])) // case when space is found
            {
                int end = pos-1;
                reverseword(start, end, input);
                start = pos + 1;
            }
            if (input[pos+1] == '\0') // case for last world
            {
                int end = pos;
                reverseword(start, end, input);
            }
        }
    }
    printf("%s\n", input);
}

Typo: You meant "word", not "world"

Not much to add here.

Handle the last word at the end

You can handle the final case the end. Then you can merge the condition check togetherin the loop.

Bug found

We have the following behaviors:

  • "a this is code review" -> "siht a si edoc weiver"
  • " a this is code review" -> "a siht si edoc weiver"

I have to go, I'll try to continue asap.

\$\endgroup\$
  • \$\begingroup\$ Why don't you use a headline (# example) instead of bold text? \$\endgroup\$ – Zeta Jan 4 '18 at 23:53
  • \$\begingroup\$ Oops, no specific reason :-( \$\endgroup\$ – Josay Jan 5 '18 at 9:06
2
\$\begingroup\$

Various problems exists with scanf("%[^\n]",input);

  1. It can over-fill leading to UB.

  2. If the first character is a '\n', input[] remains un-assigned leading to UB and '\n' still in stdin.

  3. A rare input error is not detected leading to subsequent UB.

  4. On typical input, '\n' remains in stdin, which could make a difference with a larger program.

Use fgets()

char input[MAX + 1];
if (fgets(input, sizeof input, stdin) == NULL) exit(EXIT_FAILURE);
input[strcspn(input, "\n")] = '\0';  // lop off potential \n

When ch < 0 is....(ch) is undefined behavior. Best to make unsigned char.

// if(isspace(input[pos]) && ....
if(isspace((unsigned char) input[pos]) && ....

With the context of OP's code with small strings, reverseword() using int is not a problem. For maximal functionality, C best uses size_t for array indexing and so should reverseword(). Recommend to define variables closer to where they are needed. int i; is superfluous.

void reverseword(size_t start, size_t end, char *input) {
  while(end > start) {
    char temp = input[start];
    input[start] = input[end];
    input[end] = temp;
    start++;
    end--;
  }
}

Inconsistent indentation implies OP is not using an auto-formatter to maintain a consistent style. Life is too short for manually maintaining the format. Use a programing environment that has an auto formatter and use it.

\$\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.