phetsims / trig-tour

"Trig Tour" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/trig-tour
GNU General Public License v3.0
2 stars 2 forks source link

replace StringUtils.format #92

Closed pixelzoom closed 5 years ago

pixelzoom commented 5 years ago

Noted in https://github.com/phetsims/scenery-phet/issues/446#issuecomment-460732512...

In CoordinatesRow:

var XYEqualsPattern = '(' + '{0}' + ',' + '{1}' + ')' + equalString;
var XYEqualString = StringUtils.format( XYEqualsPattern, xString, yString );

First, let's do something about those var names. Var names start with lowercase, not uppercase.

Second is the use of a string pattern. If you're using StringUtils.format to avoid string concatenation, why use string concat to create the pattern? And why tack on equalString using string concat? I would expect to see:

var xyEqualString = StringUtils.format( '({0},{1}){2}', xString, yString, equalString );

Since this doesn't involve any translated strings, it should technically be converted to named placeholders and StringUtils.fillIn, i.e.:

var xyEqualString = StringUtils.fillIn( '({{x}},{{y}}{{equal}}', {
  x: xString,
  y: yString,
  equal: equalString
} );

But... I see little value in using a string pattern here, and I think string concat would be fine, probably even clearer. I.e.:

var xyEqualString = '(' + xString + ',' + yString + ')' + equalString;
pixelzoom commented 5 years ago

This would also be an excellent place to try an ES6 template string. Note the use of backquotes:

var xyEqualString = `(${xString},${yString})${equalString}`;
jessegreenberg commented 5 years ago

Thanks, changed to an ES6 template string in the above commit and removed and removed StringUtils.format here.