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

document magic numbers #30

Closed pixelzoom closed 4 years ago

pixelzoom commented 4 years ago

Related to #1 (code review).

  • [ ] Are there any magic numbers that should be factored out as constants and documented?

A couple of places where there are some magic numbers that could use documentation about why/how these specific values were chosen:

// Lattice.js
       this.lattice = new Lattice ( new Vector3( 3.82, 3.89, 7.8 ) , 0 );
       this.sourceWavelengthProperty = new NumberProperty( 8 );

// CrystalNode.js
fill: new RadialGradient( 2, -3, 2, 2, -3, 7 )
heldentodd commented 4 years ago

There are several magic numbers in the code that I just chose to look good. The fill for the circles to represent atom positions (above), I stole from somewhere, and many of the others I played with util it looked good - for example, the amplitude of the sine waves, line thicknesses, many of the default values, the scale factor from the model to the view, and the positioning of the various elements. For these, can I just make a comment in the code? I don't think they are really unified in any way. Should I perhaps use the words "magic number" in the comments?

pixelzoom commented 4 years ago

... For these, can I just make a comment in the code? I don't think they are really unified in any way. Should I perhaps use the words "magic number" in the comments?

Yep, a comment would be great. No need to use the words "magic number" unless you feel it's useful. For values chosen to make something "look right", I'll often document as:

  radius: 10, // determined empirically

For some options (like the lineWidth option to a Path), you typically don't need a comment because those options are rather ubiquitous, and values are almost always chose "to look good". For example, it's typically not necessary to comment lineWidth in an example like this:

... new Path( ..., {
  lineWidth: 3,
  stroke: 'red'
} );

But if it's something less typical (like the examples I noted in https://github.com/heldentodd/xray-diffraction/issues/30#issue-628764037), then a comment will help the reader - who might be your future self! For example, if it's a number that changes the size of something, indicate that in a comment, and (if appropriate) also indicated something like "bigger value results in blah blah blah".

heldentodd commented 4 years ago

OK, I think I have documented or eliminated the magic numbers now. I typically used the word arbitrary in the comment so that I can find them in the code easily.