Re: [RP] ratpoison, patches, and the future

[Jérémie Courrèges-Anglas]

Jeff Abrahamson <address@hidden> writes:

[...]

> Jérémie sent me some code feedback in private. Most of it was a bit
> ordinary stuff that needn't be repeated. Worth repeating here, however:

[...]

>    - He's not so keen on my refactoring of cmd_select() and
>    set_active_window_body(), suggesting they don't bring real improvement. I
>    disagree. Both functions were overly long to my eye and harder to
>    understand for it.

Let's take a look at cmd_select() and what refactoring could *actually*
be useful.

> cmd_select (int interactive UNUSED, struct cmdarg **args)
> {
>   cmdret *ret = NULL;
>   char *str;
>   int n;
> 
>   /* FIXME: This is manually done because of the kinds of things
>      select accepts. */
>   if (args[0] == NULL)
>     str = get_input (MESSAGE_PROMPT_SWITCH_TO_WINDOW, hist_SELECT,
>                    window_completions);
>   else
>     str = xstrdup (ARG_STRING(0));
> 
>   /* User aborted. */
>   if (str == NULL)
>     return cmdret_new (RET_FAILURE, NULL);
> 
>   /* Only search if the string contains something to search for. */
>   if (strlen (str) > 0)

Here we add an indentation level when we could just return.

>     {
>       if (strlen (str) == 1 && str[0] == '-')

Here we could use strcmp(3).

But just moving this block of code into yet another function (badly
named, it's not a command) won't make the rest of the code any easier to
read.  It is already readable.

[...]

>     }
>   else
>     /* Silently fail, since the user didn't provide a window spec */
>     ret = cmdret_new (RET_SUCCESS, NULL);
> 
>   free (str);
> 
>   return ret;
> }

> (A quick git annotate on window.c even suggests that
>    Jérémie may be the author of the FIXME on set_active_window_body(). ;-)

Too quick.  e56b2ca is a revert. I un-did a similar refactoring that was
actually incorrect for three reasons: two bugs introduced, and a FIXME
comment removed, replaced by a wrong comment. Pseudo refactoring didn't
magically help the author of the commit.

"Refactor duplicate branches of if() into a single block with
leading ?:." does makes sense, I had a similar commit in one of my local
branches.

"Refactor a bit of set_active_window_body() into a helper function." is
the same mechanical change as the previous, incorrect one I've just
discussed above, except that, while it looks right from a functionality
PoV:
- you did not bother moving / removing the FIXME comment while it does
  not make sense anymore where it is
- it clutters the code with pointers to pointers instead of simple
  assignements
- it does not address the first issue: the code is looking bad *right
  now*, splitting it into smaller chunks before cleaning it will only
  help us lose sight of the big picture.

I pushed the first change, here's a wip proposal regarding further
cleanup.

0001-Third-try-at-cleaning-set_active_window_body.patch
Description: Text Data

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE