6
\$\begingroup\$

I've just started to read K&R and currently, I'm dealing with problem 1-19:

Write a function reverse(s) that reverses the character string s. Use it to write a program that reverses its input a line at a time.

Here is what I've implemented:

#include <stdio.h>

#define MAX_LINE_LENGTH 1000

void reverseLine(char reversedLine[], char line[], int lineSize);

int main(void) {

    int c;
    int i = 0;

    char line[MAX_LINE_LENGTH];
    while((c = getchar()) != EOF && c != '\n') {
        line[i++] = c;
    }
    if (c == '\n')
        line[i++] = '\0';

    int realSize = i;
    char reversedLine[realSize];

    reverseLine(reversedLine, line, realSize);
    printf("%s\n", reversedLine);

    return 0;
}

void reverseLine(char reversedLine[], char line[], int lineSize){
    int j, k;
    for(j = lineSize - 2, k = 0; j >= 0; j--, k++){
        reversedLine[k] = line[j];
    }
    reversedLine[k] = '\0';
}

I'm having problems on grasping the spirit exactly as I am supposed to be. So, any help to improve my solution will be appreciated!

\$\endgroup\$
  • \$\begingroup\$ "the solution I implemented for problem 1-19" Please include a problem description. \$\endgroup\$ – Mast Dec 5 '17 at 14:51
  • \$\begingroup\$ Ah, yes, my bad. Edited it now. \$\endgroup\$ – Said Dec 5 '17 at 16:30
  • \$\begingroup\$ @SamOnela That's not how duplicates work on Code Review. Please read the relevant meta or find us in chat. \$\endgroup\$ – Mast Dec 5 '17 at 17:33
  • \$\begingroup\$ Sorry yeah just found that... \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Dec 5 '17 at 17:34
  • \$\begingroup\$ It looks like reversedline is allocated one character larger than it needs to be. \$\endgroup\$ – Snowbody Dec 5 '17 at 20:40
5
\$\begingroup\$

Good initial code.


  1. No protection against buffer overflow.

    char line[MAX_LINE_LENGTH];
    while((c = getchar()) != EOF && c != '\n') {
      if (i < MAX_LINE_LENGTH) {  // add
        line[i++] = c;
      }
    }
    
  2. No check for "too long a line"

    // add test
    if (i  == MAX_LINE_LENGTH) {
      fprintf(stderr, "Input too long.\n");
      return EXIT_FAILURE;
    }
    
  3. Always add the null character termination

    // if (c == '\n')  // comment out line
    line[i++] = '\0';
    
  4. Code looks wrong with the - 2, yet it appears code functions correctly. The 2-ness needs explaining. Below is a size_t version. Note use of const.

    void reverseLine(char reversedLine[], const char line[], size_t lineSize){
      if (lineSize > 0) {
        size_t lineLen = lineSize - 1;
        size_t i;
        for (i = 0; i < lineLen; i++) {
          reversedLine[i] = line[lineLen - i];
        }
        reversedLine[i] = '\0';
      }
    }
    
  5. Minor: for array sizing and indexing size_t is the best type. int may be too narrow in extreme cases. Yet keep in mind that size_t is some unsigned type.

  6. Minor: "length" vs "size": When these two are applied to strings, using "size" to refer to the array size of the string and "length" as the value return from strlen() adds clarity. IMO, MAX_LINE_LENGTH should be LINE_SIZE or stay with MAX_LINE_LENGTH and use char line[MAX_LINE_LENGTH + 1]; ... if (i < sizeof line) { ... if (i == sizeof line) {.


Advanced issue: Overlap

void reverseLine(char reversedLine[], char line[], int lineSize);

Strictly speaking, the function, reverseLine() does not, by its declaration, indicate that reversedLine[] and lineSize[] should not overlap. It could fail in bizarre ways should they overlap.

C provides restrict to 1) indicate pointers should not point to overlapped data and 2) allow the compiler to perform additional optimizations that require that assumption.

// [] form also shifted to * form
void reverseLine(char * restrict reversedLine, const char * restrict line, int lineSize);

Alternatively code could be re-written to cope with overlapped buffers. I'll leave that as an exercise.


Design idea: In C, many of the basic string functions assume the destination array is big enough. As the standard library functions are crafted for performance, checking size allocation is left as a responsibility to the caller.

reverseLine() could be re-designed to do that test. Instead of leaving the size test to the caller, consider that reverseLine() is not a basic building block and it could check for sufficient memory. As with such things, error handling is an important, but for now, TBD consideration.

// rLineSize is the space available at `rLine`
// The length of the string pointed to by `line` can use `strlen()`
void reverseLine2(char * restrict rLine, size_t rLineSize, const char * restrict line) { 
  size_t lineSize = strlen(line) + 1;
  if (lineSize > rLineSize) Handle_Error();
  ...
\$\endgroup\$
  • \$\begingroup\$ I had no idea about that much number of missing points standing out in my code. Those are quite new and interesting stuff to me. I'll examine your comment time-to-time since I've just started to study C :) Thank you for your time and detailed explanation! \$\endgroup\$ – Said Dec 6 '17 at 18:03
  • \$\begingroup\$ @SaidBuyukarslan Additional idea: Write a stand-along function for user input rather than code in main(). Modularize code for code re-use. \$\endgroup\$ – chux Dec 6 '17 at 18:07

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.