Get line from string function

Question

I have asked a previous question but at the last moment I realized that the code does not work as intended as it has some small bugs.

I got some ideas for improvement though, but they didn't seem to work as well, so I decided to re-write it. This time, I was following the logic all the time.

char *tbl_getline (char *source, size_t line)
{
    size_t current_line = 1, columns;
    size_t iter;

    for(iter = 0; source[iter] != '\0' && current_line != line; iter++)
        if(source[iter] == '\n') current_line++;
    for(columns = 0; source[iter + columns] != '\n' && source[iter + columns] != '\0'; columns++);

    char *buff = malloc(columns + 1);
    memcpy(buff, source + iter * sizeof(char), columns);
    buff[columns] = '\0';
    return(buff);
}

And that was the code able to return a line from a string in a relatively good speed.

Answer 1

size_t current_line = 1, columns;

It's generally better to either initialize every variable or none in a declaration. This is especially so since you declare another variable without initialization on the next line.

size_t iter;

This use of iter is ... uncommon. The name iter has a very specific meaning. I would have expected this to be an iterator. Instead, it's just a normal index variable. You seem to be using it as a generic name for a variable used to iterate in a for loop, but there already is a standard variable for that: i.

for(iter = 0; source[iter] != '\0' && current_line != line; iter++)

So as I read that, if current_line is greater than line, you should keep on incrementing until the program crashes or we loop around? Admittedly that should never happen, but this code is not robust in the face of a change that would make it do so.

memcpy(buff, source + iter * sizeof(char), columns);

The iter * sizeof(char) is incorrect. It will multiply iter by sizeof(char) twice: once, explicitly; a second time, implicitly, for the pointer arithmetic. The correct way to write this is just source + iter. This probably works here, because sizeof(char) is usually 1. It's highly vulnerable to someone using a multibyte char though. And of course, it would give the wrong result for almost any other data type.

Answer 2

Looks fairly good.

Most people would find the following more readable than what you have:

for(iter = 0; source[iter] != '\0' && current_line != line; iter++) {
    if(source[iter] == '\n') {
        current_line++;
    }
}

It also returns an empty string if the source is shorter than the requested line. Maybe that's intended but your calling program won't know if it was an empty line or beyond the end of the source.

As another poster points out:

  memcpy(buff, source + iter * sizeof(char), columns);

should be

  memcpy(buff, source + iter , columns);

and by the opposite argument I would use

 char *buff = malloc((columns + 1)*sizeof(*buff));

for clarity as a good habit.