kilo commentary: getWindowSize (step 30)

In step 30 of implementing the kilo editor, this version of the getWindowSize function is presented:

int getWindowSize(int *rows, int *cols) {
  struct winsize ws;

  /* Note that the "1 ||" in the condition is deliberate, to force execution of the error arm of the if. */
  if (1 || ioctl(STDOUT_FILENO, TIOCGWINSZ, &ws) == -1 || ws.ws_col == 0) {
    if (write(STDOUT_FILENO, "\x1b[999C\x1b[999B", 12) != 12) return -1;
    editorReadKey();
    return -1;
  } else {
    *cols = ws.ws_col;
    *rows = ws.ws_row;
    return 0;
  }
}

This is a really ugly function, particularly considering how short it is. (For what it's worth, this is also where I got in reading through the kilo implementation when I decided I wanted to start blogging my reactions to it.)

First of all, there's another magic string and constant here, "\x1b[999C\x1b[999B" and this is definitely a place where it really feels like a bug waiting to happen. Another problem is that the outer if statement is structured backwards:

if (try something) {
  handle error case
} else {
  handle success case
}

I think it's best to keep the success case code close to the test, so having the error case code in between becomes problematic, especially as and when the error code becomes more complex. A second, related issue is that the error case code is essentially (the start of) an alternate strategy for retrieving the terminal dimensions, nested within the code for the first strategy. Notice how much nicer this reads with just a bit of refactoring:

int getWindowSize(int *rows, int* cols) {
  struct winsize ws;
  /* Note the De Morgan-ized condition to move the success condition to the "then" part of the if. */
  if (0 && ioctl(STDOUT_FILENO, TIOCGWINSZ, &ws) != -1 && ws.ws_col != 0) {
    *cols = ws.ws_col;
    *rows = ws.ws_row;
    return 0;
  }

  if (write(STDOUT_FILENO, "\x1b[999C\x1b[999B", 12) == 12) {
    editorReadKey();
  }

  return -1;
}

Now the two strategies for determining the terminal window size are separated, and the success code is right next to the test in both cases. Also note how much clearer it is here that the second strategy is incomplete, largely because it is no longer hidden inside the logic of the first strategy. Fix up the magic strings and numbers, and this code is vastly improved over the original presentation. Also note we appear to be doing a partial re-implementation of ncurses...