Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replace StringUtils.format #92

Closed
pixelzoom opened this issue Feb 5, 2019 · 2 comments
Closed

replace StringUtils.format #92

pixelzoom opened this issue Feb 5, 2019 · 2 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 5, 2019

Noted in phetsims/scenery-phet#446 (comment)...

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
Copy link
Contributor Author

pixelzoom commented Feb 5, 2019

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

var xyEqualString = `(${xString},${yString})${equalString}`;

pixelzoom added a commit that referenced this issue Feb 7, 2019
jessegreenberg added a commit that referenced this issue Apr 17, 2019
@jessegreenberg
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants