current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 7 down vote favorite
1

Rewrite readlines to store lines in an array supplied by main, rather than calling alloc() to maintain storage. How much faster is the program?

Here is my solution:

int readlines(char **lineptr, char *storage, int maxlines, int storage_size) {

    int len;
    int nlines;

     nlines = 0;
     while((len = _getline(storage, MAXLEN))) {
        if(nlines >= maxlines || !(storage_size - len)) {
            return -1;
        }
         else {
            storage[len - 1] = '\0';
            lineptr[nlines] = storage;

            storage += len; // next free space in the buffer supplied by main 
            storage_size -= len;
            nlines++;
        }
    }
    return nlines;
}

Now, the function takes 2 more arguments: a pointer to the array that will store all the read lines and its size. There is no need now to use another array to store the line temporary, I just can pass portions of the storage to getline.

The condition !(storage_size - len) verifies if there is enough space in storage to store the line that was just read. At each iteration storage is incremented to point to the next free space in the array and the value of the variables storage_size is reduced.

(This exercise can be found at page 124 in K&R.)

share|improve this question

1 Answer 1

up vote 4 down vote accepted

I don't know what _getline is; perhaps it's something like the file-input function described here.

This looks strange ...

int nlines;

nlines = 0;

I think that C now (like C++) allows you to delay defining a local variable until you initialize it. So there's no need to declare your variables at the top of the function.

Passing MAXLEN is probably wrong: because if (MAX_LEN > storage_size) then you give _getline the opportunity to write past the end of the buffer.

Allowing a buffer overrun is a bug.

Instead you could do something like:

int n_lines = 0;

for (;;)
{
    // define local buffer inside function
    char buffer[MAX_LEN];
    int line_length = _getline(buffer, MAXLEN);
    if (line_length == 0)
        break;
    if (n_lines >= maxlines)
        return -1;
    if (line_length > storage_size)
        return -1;
    buffer[line_length - 1] = '\0';
    // copy local buffer to passed-in buffer
    strcpy(storage, buffer);
    lineptr[n_lines] = storage;
    storage += line_length;
    storage_size -= line_length;
    ++n_lines;
}

return n_lines;

Or you can read directly into the passed-in buffer, to avoid strcpy and avoid defining MAXLEN:

int line_length = _getline(buffer, storage_size);
if (line_length == storage_size)
{
    // fail unless there's nothing more to read
    char temp[1];
    if (0 != _getline(temp, 1))
        return -1;
}

I'm not sure what this statement does ...

storage[len - 1] = '\0';

... if the end-of-line marker is a two-character "\r\n" sequence (which it is on Windows).

You could possibly edit your variable names; if you're going to use_underscores as you did for storage_size, then perhaps:

  • maxlines -> max_lines
  • len -> line_length
  • lineptr -> lines or lines_array
  • nlines -> num_lines or line_number or line_counter

The least obvious variable name was len.

share|improve this answer

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.