This pull's not very interesting, but worth merging. * First commit: 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. * Second commit: Split build cleanup out of destroy_project() as the latter is quite long. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/2272
-- Commit Summary --
* Split out reading old [build_settings] from build_load_menu() * Gather project build finalisation code into cleanup_project_build()
-- File Changes --
M src/build.c (47) M src/project.c (19)
-- Patch Links --
https://github.com/geany/geany/pull/2272.patch https://github.com/geany/geany/pull/2272.diff
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 pushed 0 commits.
@elextr:
these seem to be just waste of time noise
Please avoid this kind of language. 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.
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 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 approved this pull request.
LGTM, and I agree with @ntrel's rationale on splitting this.
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.
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 a 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.
github-comments@lists.geany.org