Refactoring experiences

A simple example is this code snippet:

  if (mode == 0 || pop_data->cmd_capa) {
    strfcpy (buf, "CAPA\r\n", sizeof (buf));
    switch (pop_fetch_data (pop_data, buf, NULL, fetch_capa, pop_data)) {
    case 0:
      {
        pop_data->cmd_capa = 1;
        break;
      }
    case -1:
      return -1;
    }
  }

What's the meaning of mode == 0 || pop_data->cmd_capa? What do the return values 0 and -1 of pop_fetch_data() mean? Ergo: this is awful code, as it is not immediately obvious what it does. After some searching in the code, the meaning might get revealed to you, but that's not assured.

Anyway, I started refactoring the code and trying to remove all those magic numbers. I had to do quite a lot of changes, but as a result, the code now looks like this:

if (mode == 0 || pop_data->cmd_capa != CMD_NOT_AVAILABLE) {
    strfcpy (buf, "CAPA\r\n", sizeof (buf));
    switch (pop_fetch_data (pop_data, buf, NULL, fetch_capa, pop_data)) {
    case PQ_OK:
      {
        pop_data->cmd_capa = CMD_AVAILABLE;
        break;
      }
    case PQ_ERR:
      {
        pop_data->cmd_capa = CMD_NOT_AVAILABLE;
        break;
      }
    case PQ_NOT_CONNECTED:
      return PQ_NOT_CONNECTED;
    }
  }

Now the code gets much clearer (except for mode == 0 ;-): if the availability of the CAPA is either positive or unknown (these are the two other states besides not available), we send it to the server, and fetch the result (fetch_capa is a function pointer), and evaluate the return value: if it's OK, then the CAPA command is definitely available, if it's ERR, then it's definitely not available, and if pop_fetch_data() returns "not connected", we return that to our caller.

As you can see, a few simple enums make the source a lot more readable, and I did changes similar to the ones demonstrated above in dozen of places within the code. If you look closely, you will also see the that the original code didn't cover one error situation (although it is quite unlikely, namely that CAPA worked when called the first time, and then stops working)

I tested the changes, but if you use POP3 with mutt-ng and encounter any problems, please file a bug report, so that we can eventually do further investigations.