Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm learning C from K.N. King book and just arrive at a question to reverse the words in a sentence.

Here is the output:

Enter a sentence: you can cage a swallow can't you?

Reversal of sentence: you can't swallow a cage can you?

Though I've done this and it runs correctly but I want to make my code shorter. Here is my solution:

#include <stdio.h>

#define N 100

int main(void) {
  char ch[N], terminate;
  int i, j, k, len;
  printf("Enter a sentence: ");
  for(i = 0; i < N; i++) {
    ch[i] = getchar();
    if(ch[i] == '?' || ch[i] == '.' || ch[i] == '!' || ch[i] == '\n') {
      len = i;        //Determine the length of the string
      terminate = ch[i];
      break;
    }
  }
  int word_count = 0;
  for(j = len - 1; j >= 0; j--) {
    word_count++;
    if(ch[j] == ' ') {
      for(k = j+1; k < len; k++) {
        printf("%c", ch[k]);
      }
      printf("%c", ch[j]);  // print space character
      len = len - word_count;
      word_count = 0;
    }

  }

  for(i = 0; i < len; i++) {
    printf("%c", ch[i]);
    if(ch[i] == ' ') {
      break;
    }
  }
  printf("%c", terminate);

  return 0;
}

This can be done only with loops, arrays, getchar() and putchar() method using only <stdio.h> header and no extra header(restrict <string.h> header functions and methods). Also, not use any puts() and gets() functions.

I guess this can be done only with two loops.

P.S. I am not asking any other solution, I just ask you that can I make my code shorter further.

share|improve this question
    
Just a note - your reversal is incorrect; it should be "you can't swallow a cage can you?" –  Adam V Feb 20 '13 at 22:07
    
Thanks, I'll correct it –  ashish2expert Feb 21 '13 at 3:37

3 Answers 3

up vote 2 down vote accepted

I'm not quite sure if you are aiming only for smaller length or also for better code/readability. Depending on the answer, you can take the following style comments into account (which are somehow pretty personal except for the last one):

  • In C code, I find it easier to have bigger indentations
  • I prefer having a whitespace after keywords such as for and if.
  • It's usually recommended to define variables in the smallest possible scope. In your case, if you compile with the -std=c99 flag or equivalent, you could define your variables containing indexes at the beginning of your for loops.

This being said, three things I found :

  • Because of the way you increment/update word_count and len, I had the feeling that we would always have the following property at the end of the loop : len == word_count + j. (I guess) you can prove/verify this using loop invariants. This is true just before j gets decremented and this is also true just after word_count gets incremented. In order to be 100%, I used the following macro : #define assert(c) if (!(c)) { printf ("" #c "' is not true!\n"); return 0; } and then I added assert(len == word_count+j); after incrementing word_count, after the k-look and at the end of the j-loop and the whole thing worked perfectly convincing me that my feeling was right.
    TL;DR : The conclusion of this is that we don't need the word_count variable if you replace len - word_count by j.

  • Because of the way j goes through the array, at the end of the j-loop, len is equal to the smallest index j leading to a space. Thus, we can easily deduce that for i in [0;len[, ch[i] is not a space. You can again verify this with the following assert : assert(ch[i]!=' ');.

  • At this stage, the code is slightly shorter but still we have the same loop appearing twice : for(k = j+1; k < len; k++) printf(...) to print all the words but the last one and for(i = 0; i < len; i++) printf(...) to print the last word. Thus, my last trick was to make the j-loop go one step further (so that it reaches -1 which is pretty good because then your k-loop and your i-loop becomes equivalent as j+1==0) and print the last word. This comes at the cost of additional checks but it does work on your example.

    int main(void) {
    char ch[N], terminate;
    int len;
    printf("Enter a sentence: \n");
    for (int i = 0; i < N; i++) {
    ch[i] = getchar();
    if (ch[i] == '?' || ch[i] == '.' || ch[i] == '!' || ch[i] == '\n') {
        len = i;
        terminate = ch[i];
        break;
    }
    }
    
    for (int j = len - 1; j >= -1; j--) {
    if (j<0 || ch[j] == ' ') {
        for (int k = j+1; k < len; k++) {
        printf("%c",ch[k]);;
        }
        if (j>=0) {
        printf("%c",ch[j]);;
            len = j;
        }
    }
    
    }
    printf("%c\n", terminate);
    
    return 0;
    }
    
share|improve this answer
    
Thanks for the shorter code. –  ashish2expert Feb 20 '13 at 13:13

I have a few comments.

  1. main takes argc/argv normally. Also start the function with the opening brace in colum 0

  2. Define one variable per line.

  3. Your request for input might be better printed to stderr so that it can be separated from program output.

  4. Your initial loop should test for EOF. Currently it doesn't You can test this in the *NIX shell with (program is your executable):

    /bin/echo -n "hello world" | ./program
    
  5. terminate seems redundant

  6. You can and perhaps should define loop variables in the loop (as long as you don't need the value outside the loop):

    for (int i = 0; i < n; ++i) {
    
  7. The second for-loop is a little messy. For a start it has a nested loop - my advice is to avoid nested loops and extract the inner loop to a function. This is not always practical, but more often than not it is useful. In this case, you can write a little function (eg. print_chars) that prints a specified number of characters.

  8. Your terminating loop can be replaced by a call to the print_chars function.

  9. putchar is preferable to printf("%c", ...)

  10. while, for, if etc should be followed by a space.

  11. Testing for ' ' might be better done using isspace, which will test for other space-like characters too.

Here is my version, for what it is worth:

#include <stdio.h>
#include <ctype.h>

#define N 100

static inline void print_chars(const char *s, long n)
{
    for (int i = 0; i < n; ++i) {
        putchar(s[i]);
    }
}

int main(int argc, char **argv)
{
    char line[N];
    int ch; /* note that ch is an int to allow it to hold 'EOF' */
    int len = 0;

    fprintf(stderr, "Enter a sentence: ");

    while ((ch = getchar()) != EOF) {
        if (ch == '?' || ch == '.' || ch == '!' || ch == '\n' || len >= N-1) {
            break;
        }
        line[len++] = (char) ch;
    }
    line[len] = '\0';

    char *end = line + len;
    char *s = end;
    while (--s >= line) {
        if (isspace(*s)) {
            print_chars(s + 1, end - s - 1);
            putchar(*s);
            end = s;
        }
    }
    print_chars(line, end - line);
    if (ch != EOF) {
        putchar(ch);
    }
    return 0;
}
share|improve this answer
    
Thanks for the answer, but I'm at chapter-8 of K.N. king book and upto this function like fprintf, pointers are not covered. –  ashish2expert Feb 20 '13 at 17:53

The others have pointed out a lot so I will not repeat them.

But part of programming is spotting code that can be split out into other functions for readability. You have a few places were functions would make the code more readable:

#include <stdio.h>

#define N 100

int endOfSentence(int x) {return x == '?' || x == '.' || x == '!' || x == '\n';}
char* findWord(char* ch, int len)
{
    if (len == 0)
    {return NULL;}

    while(len > 0 && ch[len-1] != ' ')
    {--len;}

    return ch+len;
}
void printWord(char* beg, char* end)
{
    while(beg != end) {putchar(*beg);++beg;}
}

int main(void) {
  char ch[N];
  char terminate;
  int i = 0;
  int j, k;

  printf("Enter a sentence: ");
  while(i < N && !endOfSentence(ch[i] = getchar())) { ++i;}

  int len = i;        //Determine the length of the string
  terminate = ch[i];

  char* word;
  while(len >= 0 && (word = findWord(ch, len)))
  {
       printWord(word, ch+len);
       len = word - ch - 1;
       if (len > 0)
       {    putchar(' ');}
  }

  putchar(terminate);
  putchar('\n');

  return 0;
}

Fixed:

> gcc 22914.c
> ./a.out 
Enter a sentence: you can cage a swallow can't you?
you can't swallow a cage can you?
share|improve this answer
1  
Sorry, but that is not up to your usual high standard. Try compiling it. When you fix the errors, try running it... –  William Morris Feb 21 '13 at 1:52
    
Yes. C is much harder than C++ –  Loki Astari Feb 21 '13 at 19:00

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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