build: Split up build_load_menu()

[ntrel]

Split out legacy code from build_load_menu() and put assign_cmd() in the middle, nearer where it's used - follow up tweak to #2256.

[elextr]

Both of these seem to be just waste of time noise.

Whats the point of breaking what is linear code into something that appears to be separate functionality. Disagree with it.

[ntrel]

@elextr:

these seem to be just waste of time noise

Please avoid this kind of language, it makes me uncomfortable. It sounds like you are annoyed. Obviously I do not think it is a waste of time or I would not have submitted it. I often don't submit changes I have done that aren't of clear benefit to a project.

I have removed the second commit (and updated the PR description), to avoid making a subtle argument for it. Regarding the first:

Smaller functions are easier to understand, and build_load_menu is long and complicated.
build_load_menu has 2 top-level scope variables that aren't used by load_old_menu.
load_old_menu has 4 top-level scope variables that aren't used by build_load_menu.
Each function reads different groups of data.

This should tell you that the code doesn't belong in one function.

[b4n]

LGTM, and I agree with @ntrel's rationale on splitting this.

[elextr]

Please avoid this kind of language, it makes me uncomfortable. It sounds like you are annoyed. Obviously I do not think it is a waste of time or I would not have submitted it. I often don't submit changes I have done that aren't of clear benefit to a project.

Programming is performed by people, and people have emotions. In this case the emotion is exasperation not anger. Geany is riddled with such spaghetti that divides simple linear code into sequential function calls and that makes it much harder to follow.

Not every function is actually doing something short and simple, thats just life, and breaking such code into multiple parts makes it harder to follow.

In this case the function of build_load_menu is "do this then do that", and your argument that one or two variables are not shared by this and that is irrelevant.

I'm sure we will continue to disagree on this, but thats life, @b4n will just have to keep adjudicating.

[codebrainz]

Not speaking to this PR itself, but in I tend to agree with @elextr in principle. A lot of Geany's code factoring is too granular and it makes it a pain to follow the flow when you have to keep jumping between short functions that do a small part of the overall goal. I don't think too many lines of code is - in and of itself - a valid reason to break up a function into smaller parts.

As discussed in the other PR, I'm also not fond of short helper functions which do trivial stuff everyone already understands in order to save a few lines of code. Some examples I looked at recently are the utils_free_pointers() function and the SETPTR() macro. These just increase the mental overhead required to understand the code by having to know all these functions rather than just plain C stdlib or GLib.

Sorry I'm slightly off topic here, I haven't review the changes here yet to see whether it seems justified. Maybe we could work toward some guidelines on factoring of functions as a separate discussion.