Drasil icon indicating copy to clipboard operation
Drasil copied to clipboard

Eliminate Duplicate String "InputParameters"

Open peter-michalski opened this issue 3 years ago • 8 comments

Details:

"InputParameters" Number of instances of string: 21 Number of files containing the string: 6 ./Drasil/code/drasil-code/Language/Drasil/Code/CodeQuantityDicts.hs:"InputParameters" ./Drasil/code/drasil-code/Language/Drasil/Code/Imperative/Descriptions.hs:"InputParameters" ./Drasil/code/drasil-code/Language/Drasil/Code/Imperative/DrasilState.hs:"InputParameters" ./Drasil/code/drasil-code/Language/Drasil/Code/Imperative/Import.hs:"InputParameters" ./Drasil/code/drasil-code/Language/Drasil/Code/Imperative/Modules.hs:"InputParameters" ./Drasil/code/drasil-code/Language/Drasil/Code/Imperative/Parameters.hs:"InputParameters"

peter-michalski avatar Sep 16 '21 19:09 peter-michalski

My expectation is that you will also propose a fix when you create the issue (or soon thereafter). If you are not in a position to propose a fix, then you should analyze what's going on in the code further and add your analysis to the issue.

JacquesCarette avatar Sep 16 '21 22:09 JacquesCarette

I propose that we create functions for each of the duplicate strings. The functions will return the string. We then place the functions in some tbd /code file, and import the file and use the function where necessary.


InputParametersString :: String  --function declaration 
InputParametersString = "InputParameters" --function definition 

peter-michalski avatar Sep 22 '21 13:09 peter-michalski

That is too naive a solution. The proper Drasil way to do this is to understand what "InputParameters" means as a concept, what scope it is used in, and then create the appropriate chunk to record that data. The use sites can then use the appropriate combinators to extract that information.

JacquesCarette avatar Sep 22 '21 15:09 JacquesCarette

./Drasil/code/drasil-code/Language/Drasil/Code/CodeQuantityDicts.hs:

This appears to be creating a dictionary, with the datatype Actor tied to "InputParameters". Not clear what implVar is..

./Drasil/code/drasil-code/Language/Drasil/Code/Imperative/Descriptions.hs

--Defines descriptions generators. 
inputClassDesc :: GenState Description
inputClassDesc = do
  g <- get
  let cname = "InputParameters"

runs the IO action "get", sets cname (classname?) to "InputParameters", from file: -- | Returns a description for the generated function that stores inputs, -- if it exists. Checks whether explicit inputs, derived inputs, and constants -- are defined in the InputParameters class and includes each in the -- description if so.

./Drasil/code/drasil-code/Language/Drasil/Code/Imperative/DrasilState.hs: -- | Private State, used to push these options around the generator. Comments include getting states, variables, others for InputParameters module InputParameters is the name of a module

./Drasil/code/drasil-code/Language/Drasil/Code/Imperative/Import.hs:

inputVariable :: (OOProg r) => Structure -> ConstantRepr -> SVariable r -> 
  GenState (SVariable r)
inputVariable Unbundled _ v = return v
inputVariable Bundled Var v = do
  g <- get
  let inClsName = "InputParameters"

-- if current module is InputParameters, 'inParams' otherwise.

./Drasil/code/drasil-code/Language/Drasil/Code/Imperative/Modules.hs:

getInputDecl :: (OOProg r) => GenState (Maybe (MSStatement r))
getInputDecl = do
  g <- get
  v_params <- mkVar (quantvar inParams)
  constrParams <- getInConstructorParams 
  cps <- mapM mkVal constrParams
  let cname = "InputParameters"

-- If there are inputs and they are exported by a module, they are 'Bundled' in -- the InputParameters class

genInputModSeparated :: (OOProg r) => GenState [SFile r]
genInputModSeparated = do
  ipDesc <- modDesc inputParametersDesc
  ifDesc <- modDesc (liftS inputFormatDesc)
  dvDesc <- modDesc (liftS derivedValuesDesc)
  icDesc <- modDesc (liftS inputConstraintsDesc)
  sequence 
    [genModule "InputParameters" ipDesc [] [genInputClass Primary],

-- | Generates separate modules for each input-related component. similar for genInputModCombined

genInputClass :: (OOProg r) => ClassType -> 
  GenState (Maybe (SClass r))

-- | Returns 'Nothing' if no inputs or constants are mapped to InputParameters in -- the class definition map.

genInputConstructor :: (OOProg r) => GenState (Maybe (SMethod r)) -- | Generates a constructor for the input class, where the constructor calls the -- input-related functions. Returns 'Nothing' if no input-related functions are -- generated.

./Drasil/code/drasil-code/Language/Drasil/Code/Imperative/Parameters.hs: let cname = "InputParameters" sets class name

From all of this it looks like InputParameters is a class and/or module with a class. I am seeing this class defined in ./Drasil/code/stable/ many times, such as for nopcm, glassbr, projectile..in C#, java, Python, C files (no Haskell files)..which may be the reason for the class being a String in the above Haskell files.

Please let me know if I am going in the right direction..

peter-michalski avatar Sep 23 '21 18:09 peter-michalski

It's the correct direction. In more detail:

  • a lot of the information (about where the strings occur in the code itself) is too low-level to appear in the issue itself. This is information you need to collect as part of your own understanding
  • the one-line conclusion "From all of this it looks like InputParameters is a class and/or module with a class." is the main low-level take-away
  • the other good part is that you looked up where this sows up in code/stable, i.e. the second half of this.

The parts you are missing:

  • why is it "the same string" in multiple places? Does it have to be, i.e. is it on purpose, or is it an accident? [It's probably on purpose!]
  • so why is this name being "shared" in multiple places mean? what kind of coordination is going on?
  • in the occurrences in code/stable, what purpose does this "InputParameters" seem to serve?
  • the name "InputParameters" is quite evocative - is it correct?

There's probably more missing things, but those will become clear when the above points themselves are clearer.

JacquesCarette avatar Sep 23 '21 20:09 JacquesCarette

why is it "the same string" in multiple places? Does it have to be, i.e. is it on purpose, or is it an accident? [It's probably on purpose!]

Looking at /stable, the string is present in glassbr, nopcm, pdcontroller, projectile. Within each project the strings are found in the same set of files for each language in the project (set of C++, C#, Java, Pythin, Swift). Most common files are:

Control --includes InputParamters file, calls InputParameters function designLog --Successfully matched Actor "InputParameters" with Object "InputParameters"....so the string is an "Actor" for the Object "InputParameters" doxConfig --# This file describes the settings to be used by the documentation system doxygen... InputParameters --class, brief Structure for holding the input values and derived values Makefile DerivedValues (rare)

InputParameters is a file/class that holds input values. I believe Haskell uses the "InputParameters" string to reference this class in all /stable languages. Please confirm and comment on this. Also, please clarify what an Actor is in Drasil. I think this may be a key in how it can use the string to reference the class, but am not sure.

so why is this name being "shared" in multiple places mean? what kind of coordination is going on?

Descriptions.hs --checks whether explicit inputs, derived inputs, and constants are defined in the InputParameters class DrasilState.hs --Comments include getting states, variables, others for InputParameters module Modules.hs --If there are inputs and they are exported by a module, they are 'Bundled' in the InputParameters class

So a .hs class/ or module InputParameters is also created? Parameters are imported from the /stable language specific "InputParameters" class?

" let cname = "InputParameters" " is found several times. This must be for calling a .hs class OR for referencing the /stable "InputParameters" class, OR some sort of map. "Mapping" is mentioned in several of the /code files. Not certain which of these coordinations is happening, but probably the latter. Please confirm.

In the occurrences in code/stable, what purpose does this "InputParameters" seem to serve? the name "InputParameters" is quite evocative - is it correct?

Answer for this is in the answer for the first question. InputParameters is a class for input parameters. Quite evocative indeed. Clear in /stable but not as clear in /code.

Some outstanding questions: Coordination of .hs files, order of building the projects (languages), coordination between .hs files and projects (not specifically the InputParameters, which is being discussed already)

peter-michalski avatar Sep 28 '21 01:09 peter-michalski

You need to differentiate between low-level details (it's a String) and higher-level conceptual interpretation (it's a name for a concept).

The concept is that of "input parameters" (somewhat unsurprisingly). There are many ways that these can show up -- they can just be a bunch of variables names that will hold the value of the different parameters. Or they can be joined up into a struct, so that a single thing can be passed around. [That's a design choice.] So it's possible to disperse the code for dealing with input parameters throughout the code, or have a single module that takes care of it. In different languages, the concept of 'module' will then appear in different guises -- mostly classes in OO languages.

You should think hard about the level of discourse of my previous paragraph, and compare it to the level of discourse in your analysis.

About Actor: it turns out that the classic OO understanding of objects + methods doesn't quite work so well for encapsulating some of the modularizations that we want. There is an alternate conceptualization of OO which views the basic entity as an "Actor" (rather than an object) that has an independent existence, and thus the only way to communicate with it (and vice-versa) is via messages. The Actor model is frequently used in concurrent programming. While we're not necessarily ready to go there, some languages (and Swift for sure, but Objective-C too, which we used to support). It's used a lot in Akka, for Java and Scala. We used it in Drasil to get a certain decoupling of various pieces that were too tangled before.

The point behind "InputParameters" is that it is the name of a single concept. In generated code, that shows up as using that string to refer to the same thing (call it, pass messages, etc). As it is a single concept, inside Drasil, there String itself should occur once (inside the appropriate Chunk), and extracted from there at point of use. Then the actual concept should be in a Haskell data-structure, passed around via the name given to it in Haskell.

There are clear advantages to this:

  1. if you misspell "InputParameters" in one place, you're not debugging for days on end - it won't compile!
  2. you get traceability. "InputParameters" will come from a single concept inside Drasil
  3. if we want to change its name, there is a single place to do so.

So when you are answering the question "why is this name being shared in multiple places?", you are reverse-engineering the different guises that correspond to 'dealing with input parameters'. They are morally all the same, but can show up in the code looking different. That's the knowledge we want to encode in Drasil.

The "let cname = "InputParameters" " is actually a mini-form of local abstraction, where we can use 'cname' to make absolutely sure it refers to the same thing, always. At some level, it is that simple.

So "InputParameters" is the name of a concept (not a class!) encapsulating 'input parameters'. What's wrong in Drasil is that this name is a String instead of being properly captured as a concept. Similarly, what is an 'input parameter' and what can we do with do (and how and ...) is scattered in the (Haskell) code itself, instead of being localized. So indeed, it's not so clear in /code.

I think some of your outstanding questions are somehow 'off', in the sense that I think you're thinking of this at too low a level, and so they are not interesting questions that need an answer. There will definitely be related questions that will have interesting answers. I'll wait for those.

JacquesCarette avatar Sep 30 '21 21:09 JacquesCarette

This String can be added to the function name map from #2966

peter-michalski avatar May 03 '22 17:05 peter-michalski