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

Reply via email to