heldentodd / xray-diffraction

This repository contains the code for one simulation, but eventually three are planned: bragg-law, Single Crystal Diffraction, and Powder Diffraction.
GNU General Public License v3.0
2 stars 1 forks source link

inline the derivation function for DerivedProperty #21

Closed pixelzoom closed 4 years ago

pixelzoom commented 4 years ago

Related to #1 (code review).

In Lattice.js, the derivation function for this.latticeConstantsP is defined as a separate function:

this.latticeConstantsP = new DerivedProperty( [this.aConstantProperty, this.bConstantProperty, this.cConstantProperty ], combineConstants );

...

  function combineConstants( a, b, c ) {
    return new Vector3(a, b, c);
  }

Similarly, in XrayDiffractionModel.js:

       this.pLDProperty = new DerivedProperty( [this.lattice.latticeConstantsP, this.sourceAngleProperty ], computepLD );
       this.pLDWavelengthsProperty = new DerivedProperty( [this.pLDProperty, this.sourceWavelengthProperty], computepLDWavelengths );

...

function computepLD( constants, theta ) {
  return 2 * constants.z * Math.sin(theta);
}

function computepLDWavelengths( pLD, wavelength ) {
  return pLD/wavelength;
}

I'm guessing that there might be a misconception about how to instantiate a DerivedProperty. Unless the derivation function is used in multiple places, it's perfectly OK for it to be inlined. And it's easier to read and less likely to become out-of-sync if it's inlined.

So for example Lattice.js would become:

this.latticeConstantsP = new DerivedProperty( 
  [this.aConstantProperty, this.bConstantProperty, this.cConstantProperty ], 
  ( a, b, c ) => new Vector3( a, b, c )
);
heldentodd commented 4 years ago

Good point. I admit that I am still a bit uncomfortable with the => function. Also, the need for some of these derived properties has more to do with my future plans than the current sim.

I've inlined all three derived property functions from the project since they are all 1 line. Thank you.

pixelzoom commented 4 years ago

I was also initially uncomfortable with arrow functions (=>). As someone who has studied programming languages, JavaScript is (to put it nicely) not my favorite language. It seems to invent new constructs solely for the purpose of working around features that were done poorly in the first place. Arrow functions are one such example. If they had gotten "scope" correct for functions, there would be no need to introduce yet-another type of function construct with different scope behavior. Arrow function do have only one (dubious) advantage, and that's more compact syntax. And now, I will step down off of my soapbox :)