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

better names for a couple of things #16

Closed pixelzoom closed 4 years ago

pixelzoom commented 4 years ago

Related to #1 (code review):

  • [ ] Names (types, variables, properties, Properties, functions,...) should be sufficiently descriptive and specific, and should avoid non-standard abbreviations. For example:

Overall good naming, but there are a couple of places that could be improved.

In XrayDiffractionModel.js, I'm guessing that "pLD" is an abbreviation for something:

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

In LightPathNode.js, these could benefit from better names, or documentation describing them:

      let shape0 = new Shape();
      const shape1 = new Shape();
pixelzoom commented 4 years ago

I also see the "pLD" abbreviation in a few other places: computepLD, computepLDWavelengths, parameter pLD, pLDRegion1, pLDRegion2,... If this is a standard abbreviation for this domain, it would be good to note in implementation-notes.md. If it's non-standard, consider replacing it with something more verbose/descriptive.

heldentodd commented 4 years ago

PLD is my abbreviation for path length difference. It is how much farther the lower ray travels than the upper ray. In order for the reflected rays to be in phase (and thus not cancel out) this has to be and integer multiple of the wavelength.

Enough physics. For now, I will just capitalize PLD when it is not at the start. Other than that, I am certainly willing to change the name, but I think having a function name like computePathLengthDifferenceInWavelengths makes the code a little hard to read. What do you thing?

I also went ahead and added a sentence in implementation-notes.md.

pixelzoom commented 4 years ago

If it's a standard abbreviation, then PLD is certainly appropriate to use. Thanks for adding to implementation-notes.md!

pixelzoom commented 4 years ago

By the way... PhET doesn't have a standard or template for implementation-notes.md, so they are a bit inconsistent. But I usually include a "Terminology" section, where things like "path length difference (PLD)" are defined. There's no need to change your implementation-notes.md. But if you want to take a peek at a couple of other examples:

https://github.com/phetsims/gas-properties/blob/master/doc/implementation-notes.md https://github.com/phetsims/equality-explorer/blob/master/doc/implementation-notes.md

... are a couple for recent sims where I've been the primary developer.

heldentodd commented 4 years ago

Yes, your implementation notes are beautiful. I tried to copy them, but they will need more work. I'm sure they will improve.