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

ISLCObjectEnum should be Enumeration #147

Closed
zepumph opened this issue Jun 11, 2019 · 5 comments
Closed

ISLCObjectEnum should be Enumeration #147

zepumph opened this issue Jun 11, 2019 · 5 comments

Comments

@zepumph
Copy link
Member

zepumph commented Jun 11, 2019

From #134, this really feels right, and it would make small bugs like passing in undefined instead of an enum a bit easier. For example a method like this is sloppy:

    /**
     * @param {ISLCObjectEnum} objectEnum
     * @returns {string}
     */
    getOtherObjectLabelFromEnum( objectEnum ) {
      return objectEnum === OBJECT_ONE ? this.object2Label : this.object1Label;
    }
@zepumph
Copy link
Member Author

zepumph commented Jun 11, 2019

@jessegreenberg please review this as part of the code review at large. I tried to move as much static content into the Enumeration as possible.

@jessegreenberg
Copy link
Contributor

Looks great, I agree making this an Enumeration is right. My only nit-pick is about formatting, I think it is atypical to create the Enumeration inline with the inverseSquareLawCommon.register call. Modified in the above commit, does this seem reasonable?

@zepumph
Copy link
Member Author

zepumph commented Jun 28, 2019

No strong preference here, just note that a search for , new Enumeration yields 10 others. Ready to close if you are.

@zepumph
Copy link
Member Author

zepumph commented Jun 28, 2019

(also I don't see the commit in this issue)

@zepumph zepumph assigned jessegreenberg and unassigned zepumph Jun 28, 2019
jessegreenberg added a commit to phetsims/inverse-square-law-common that referenced this issue Jun 28, 2019
@jessegreenberg
Copy link
Contributor

just note that a search for , new Enumeration yields 10 others

Indeed, I sit corrected. I reverted the change. This issue can be closed.

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