Short summary: I placed these functions into a separate header, because they are *stateless*. That is, one piece less complexity in the CmdInterpreter. Some further context info to that change from 2019 https://github.com/Yoshimi/yoshimi/pull/56 The actual commit to extract these functions, which were previously part of CmdInterpreter.cpp
commit e0a3a7ebbcb2dfceeb39e97a722e42a8004b219b Author: Ichthyostega <p...@ichthyostega.de> Date: Sun Aug 11 17:32:47 2019 +0200 Refactoring: split out the stateless part of CmdInterpreter Basically all those functions to communicate with the SynthEngine do not by themselves rely on the state/data fields in the CmdInterpreter. Thus move them into the function libaray CliFuncs.h Now parsing and cmd interpretation are mostly separated; hoever, cmd interpretation itself is still scattered over several classes
Find below the mail with explanations I sent you for the review. Together, this might give a better understanding of the changes at that time. -------- Forwarded Message -------- Subject: CLI/MiscFunc refactorings: some findings Date: Wed, 14 Aug 2019 12:28:08 +0200 From: Ichthyostega <p...@ichthyostega.de> To: Will Godfrey <willgodf...@musically.me.uk> Hi Will, with this mail, I'd like to show you some points to note and observations I've made during the work of the last days. Regarding the premise, we have a piece of code which we perceived as slightly problematic, for the future, when moving ahead. To find out more about that, I made a series of transformations on the "cli" branch. Because changing and remoulding things allows to find out about their nature: what can be changed easily, what resists to change, what reads clearer afterwards, or, to the opposite, what turns out to be better left in original shape. To do such transformations requires a certain degree of boldness; and thus it is very important that we now question the results. Am 13.08.19 um 22:32 schrieb Will Godfrey:> One thing slowing me down at the moment with understanding the new
structure is all the function name changes. This is *not* a complaint, it's just that I'm so used to the old names I tend to lose track.
This is an important point. I am rather new to the code base and thus immediately stumble over things which claim by their name to do A, while in fact they do B. Especially I recall - a global var "miscList" and a function to "pop" some message ("miscMsgPop"). As it turned out, this is not a list (it is implemented by a list), and is not "misc", but dedicated to messages only. And most notably, it is *not* used in the way of a stack. I spent quite some time looking puzzled at the code and could not find any matching push/pop pairs. Thus I propose to call it TextMsgBuffer and to "fetch" from it, not "pop" - a huge function in the CLI called "readStatus", which turned out to *alter* the status, even to establish it and set some central state variables, like the kitNumber and the kitMode. Thus I renamed it into "buildStatus" to indicate it is a mutating function. Generally speaking, it is problematic for code maintenance when state retrieval and state alteration are mingled and mixed up. I have spent numerous hours in my developer life searching for mysterious bugs in despair, just because some superficially harmless getter function in fact manipulated some crucial detail hidden deep down. == some things to note == * TextMsgBuffer is a *singleton*. This is something new for the codebase. "Singleton" is a programming pattern how to package global variables. They are still global (which means they are problematic), but at least a Singleton gives us a constructor/destructor, a clear lifecycle, and better control about how they are accessed / altered * I have eliminated all those "mix-in" baseclasses, which do not actually hold any shared state. These can be turned into library functions in namespaces. This is a change I would propose to retain, because I can not think of any good argument in favour of the old base class shape. - MiscFuncs -> NumericFuncs.h , FormatFuncs.h, CliFuncs.h - FileMgr -> FileMgrFuncs.h - SynthHelper -> SynthHelper.h * I investigated the data flow and data usage in the CLI / CmdInterface. Bottom line is, not much can be done here easily, since there is a group of state variables, which is highly cohesive and linked crosswise. It is used by a huge piece of code, which is likewise highly cohesive. I would recommend *against splitting* this code, because by all my experience, splitting a piece of cohesive code makes maintenance harder. This huge central piece of code is now in the Class CliInterpreter, together with all the shared commonly used state. At least I was able to make some state variables local, and to extract some smaller functions, which are stateless (and not using those common state variables). * I separated the CLI processing loop from the CliInterpreter. The latter is now a private component, and used by the former. * please note that you can return a struct (or class) by *value* from functions. This is way better than to return just a code and sneak additional values out via some shared data field. I applied this transformation to the return value of CmdInterpreter::cmdIfaceProcessCommand() see the struct Reply defined at the top of CmdInterpreter.h * the last step I did was to extract the parsing mechanics and encapsulate it into a parser class. See the header Parser.h Note that the users of this parser no longer directly manipulate the char* which constitutes the parsing state. This may seem superficially, but in fact is a far reaching change. We could now change the parsing technology or state handling transparently, as long as we retain the API of this parser class. Looking ahead for a further discussion; feel free to question anything I touched. If the result of some refactoring is that we find it does not improve matters, than it was worth all the effort. -- Hermann _______________________________________________ Yoshimi-devel mailing list Yoshimi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/yoshimi-devel