10
\$\begingroup\$

I am completely re-writing the textbox GUI widget. In order to set the position cursor after the last character in the last line of a text, it is good if you know the exact column of it. So I created a small function that calculates this. It is meant to work sonicly.

int get_last_line_length (char *string)
{
    const int len = strlen(string) - 1;
    int i;

    for(i = len; i != -1 && string[i] != '\n'; i--);

    return (len - i);
}
\$\endgroup\$
  • \$\begingroup\$ I'm a little confused; if you needed to put the cursor in the character space at the end of the line, wouldn't you just find the length of the line and add one to it? (strlen(line) + 1). \$\endgroup\$ – SirPython Jun 27 '15 at 0:03
  • \$\begingroup\$ Don't be confused. As I said, I am re-writing it from scratch. Which means I have to draw the cursor itself, animate it and find the exact x/y canvas coordinates to draw it to. strlen(text) is the iterator and get_last_line_length(text) is last column. \$\endgroup\$ – Edenia Jun 27 '15 at 0:05
8
\$\begingroup\$

It's good that you've used strlen, but there's actually another standard function that could be useful here. Instead of your reverse loop, you can use strrchr to find a pointer to the last occurrence of a character (or NULL if said character is not found). As discussed in the comments, this could be (and likely is) slower in the case when a newline is not found, but unless this function is called very often or is in a very performance critical path, I can't imagine it will be an issue. If you do need to optimize it to your original implementation, I would pull the reverse loop logic out into a function.

Anyway, a bit of a review:

  • Your string parameter should be a pointer to const since it's not modified.
  • Personal style thing: I'm not a fan of return (...). If it matters, it also tends to be pretty rare style wise in C.
  • You should probably explicitly document what happens if a line break is not found in the text. I wouldn't be sure whether strlen(string) or -1 we be returned.
\$\endgroup\$
  • \$\begingroup\$ It is designed to return the string's len if line break wasn't found. \$\endgroup\$ – Edenia Jun 27 '15 at 0:17
  • \$\begingroup\$ Thanks. I am not sure if using strrchr on that way will perform less operations than my inverse loop..(in my biased opinion it will be only slower) but one thing is certain - I don't know what would it benefit anyway. \$\endgroup\$ – Edenia Jun 27 '15 at 0:37
  • \$\begingroup\$ @Edenia it would indeed be slower in the case that a new line isn't found. Unless this is in a tight loop though, I doubt it will be noticeable and the clarity will likely be worth any tiny hit (unless it ends up being a hot spot). \$\endgroup\$ – Corbin Jun 27 '15 at 0:47
  • \$\begingroup\$ I know I look overexacting. But I work into a SDL-based engine restricted to FPS. There any operation can be noticed by the FPS drop. Yes it is runtime coding using the "EiC" interpreter. \$\endgroup\$ – Edenia Jun 27 '15 at 0:50
5
\$\begingroup\$

If len is const, then char *string should be const as well. Actually, nobody really cares if the local variable is const; it's more important that the parameter be const.

I'm not a big fan of len = strlen(string) - 1, since the -1 makes it not the length of the string.

Consider working forward instead of backwards. I particularly like strlen(last_line), which makes this code more self-explanatory.

int get_last_line_length(const char *string)
{
    const char *newline;
    const char *last_line = string;
    while ((newline = strchr(last_line, '\n'))) {
        last_line = newline + 1;
    }
    return strlen(last_line);
}

That said, I suspect that your implementation might be faster.

\$\endgroup\$
  • \$\begingroup\$ I was about to Add Comment "I suspect that my implementation might be still faster though" ..and then I saw the last line of your answer and I was like how did my unposted comment got there. \$\endgroup\$ – Edenia Jun 27 '15 at 0:55
  • 1
    \$\begingroup\$ I'd say that your original code is good enough, especially if you are aiming for speed. \$\endgroup\$ – 200_success Jun 27 '15 at 0:57
  • \$\begingroup\$ In that case I would aim for speed, because it slows down everything aside the text box.. and the implemented base drawing functions are just impossibly slow. Anyway, your approach is nice and I would use it in a regular circumstance. \$\endgroup\$ – Edenia Jun 27 '15 at 1:00
4
\$\begingroup\$

As 200_success said, the len variable that's not really the length is not intuitive. It would be good to either rename the variable to last_pos, or to change the code.

A minor and possibly subjective thing, but instead of != -1 in the loop condition, >= 0 seems slightly more intuitive, hinting a descending counter, and aiding readability a little bit.

Lastly, also a minor thing, but a for loop without a body is sometimes frowned upon. If the main purpose of the loop is to count, not something else (the loop body), then it's better to make the counting operation more prominent by converting the for loop to a while loop.

int get_last_line_length (char *string)
{
    const int len = strlen(string);
    int i = len - 1;

    while (i >= 0 && string[i] != '\n') {
        --i;
    }

    return len - i - 1;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ If you want to avoid having an empty for loop, I suggest for (i = len - 1; i >= 0; i--) { if (string[i] == '\n') break; }. That way, the loop is still obviously a countdown for i. \$\endgroup\$ – 200_success Jun 27 '15 at 6:22
  • \$\begingroup\$ Not a bad idea. I had another reason though I didn't explain: when the counter variable is needed outside, I prefer a while loop \$\endgroup\$ – Stop ongoing harm to Monica Jun 27 '15 at 6:39

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.