search

m-lab / ndt

Network Diagnostic Tool
Other
11 stars 7 forks source link

Separating Frontend and Backend Functionality in NDT HTML5 Client - Part 2 #55

Closed shredtechular closed 8 years ago

shredtechular commented 8 years ago

NOTE - this PR is dependent on #54 and will need to be rebased after #54 is reviewed/accepted.

Per feedback contained in #49 this PR is part 2 of spliting the changes in that PR into a more manageable PR. This first PR contains additional cleanup of the script.js file adding in comments that follow Google JS Code Style.

Once #54 and this PR is merged, #49 can be closed.

This change is Reviewable

collina commented 8 years ago

Aren't private variables typically denoted with prefixed underscores rather than appended ones? http://stackoverflow.com/questions/4484424/underscore-prefix-for-property-and-method-names-in-javascriptfe

Previously, shredtechular (Andrew Newhouse) wrote… > #### Separating Frontend and Backend Functionality in NDT HTML5 Client - Part 2 > > **NOTE** - this PR is dependent on #54 and will need to be rebased after #54 is reviewed/accepted. > > Per feedback contained in #49 this PR is part 2 of spliting the changes in that PR into a more manageable PR. This first PR contains additional cleanup of the script.js file adding in comments that follow [Google JS Code Style](https://google.github.io/styleguide/javascriptguide.xml). > > Once #54 and this PR is merged, #49 can be closed. > >

Comments from Reviewable

collina commented 8 years ago

Are all of this methods actually private that are specified as such (e.g. testStatus)?

Previously, collina (Collin Anderson) wrote… > Aren't private variables typically denoted with prefixed underscores rather than appended ones? http://stackoverflow.com/questions/4484424/underscore-prefix-for-property-and-method-names-in-javascriptfe >

_Comments from Reviewable_

shredtechular commented 8 years ago

I thought it was weird to put them as "appended" vs. prepended. However, the Google JS Style guide says, "trailing" and even shows examples of them being appended? Am I misunderstanding?

"Private properties and methods should be named with a trailing underscore. Protected properties and methods should be named without a trailing underscore (like public ones)."

https://google.github.io/styleguide/javascriptguide.xml?showone=Code_formatting#Naming

Also, "We recommend the use of the JSDoc annotations @private and @protected to indicate visibility levels for classes, functions, and properties."

https://google.github.io/styleguide/javascriptguide.xml?showone=Code_formatting#Visibility__private_and_protected_fields_

Previously, collina (Collin Anderson) wrote… > Are all of this methods actually private that are specified as such (e.g. testStatus)? >

Review status: 0 of 5 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

collina commented 8 years ago

Yeah you are right, that's strange but okay.

Previously, shredtechular (Andrew Newhouse) wrote… > I thought it was weird to put them as "appended" vs. prepended. However, the Google JS Style guide says, "trailing" and even shows examples of them being appended? Am I misunderstanding? > > "Private properties and methods should be named with a trailing underscore. > Protected properties and methods should be named without a trailing underscore (like public ones)." > > https://google.github.io/styleguide/javascriptguide.xml?showone=Code_formatting#Naming > > Also, "We recommend the use of the JSDoc annotations @private and @protected to indicate visibility levels for classes, functions, and properties." > > https://google.github.io/styleguide/javascriptguide.xml?showone=Code_formatting#Visibility__private_and_protected_fields_ >

Comments from Reviewable

shredtechular commented 8 years ago

Closing out in favor of #55