search

m-lab / ndt

Network Diagnostic Tool
Other
11 stars 7 forks source link

Separating Frontend and Backend Functionality in NDT HTML5 Client - Part 1 #54

Closed shredtechular closed 8 years ago

shredtechular commented 8 years ago

Per feedback contained in #49 this PR splits the changes in that PR into a more manageable PR. This first PR contains and only contains the changes needed to separate the backend functionality from the script.js file into an ndt-library.js file. When moving that functionality to the ndt-library.js file, it needs to be removed from the script.js file and then minimal changes to make sure the application still works.

The idea is to ease how an implementer might include an NDT js library onto their website and by including only the necessary js, an implementer could create their own frontend using the NDT js library.

As the current implementation would require a website to include five different scripts, this PR starts to address that breaking up the script.js file into frontend and backend functionality. This removes the dependency for an NDT js library to need jQuery, the gauges.min.js library, or the script.js file. Those files would ONLY be needed if a user wanted the NDT frontend that exists on the site today, but not if it was creating a custom frontend.

1 small change was made to the css and ndt-browser-client.js files in order to make the application still function properly.

Finally, also added the ndt-library.js include into the widget.html.

Once this PR and part 2 #55 is merged, #49 can be closed.

This change is Reviewable

critzo commented 8 years ago

:lgtm: But I would also like a second reviewer to take a look. @fernandalavalle or @mtlynch ?

Previously, shredtechular (Andrew Newhouse) wrote… > #### Separating Frontend and Backend Functionality in NDT HTML5 Client - Part 1 > > Per feedback contained in #49 this PR splits the changes in that PR into a more manageable PR. This first PR contains and only contains the changes needed to separate the backend functionality from the `script.js` file into an `ndt-library.js` file. When moving that functionality to the `ndt-library.js` file, it needs to be removed from the `script.js` file and then minimal changes to make sure the application still works. > > The idea is to ease how an implementer might include an NDT js library onto their website and by including only the necessary js, an implementer could create their own frontend using the NDT js library. > > As the current implementation would require a website to include [five different scripts](https://github.com/m-lab/m-lab.github.io/pull/76/files#diff-56ff23ff6448049981daee3c0344f9cdR22), this PR starts to address that breaking up the [script.js](https://github.com/m-lab/ndt/blob/master/HTML5-frontend/script.js) file into frontend and backend functionality. This removes the dependency for an NDT js library to need jQuery, the gauges.min.js library, or the script.js file. Those files would **ONLY** be needed if a user wanted the NDT frontend that exists on the site today, but not if it was creating a custom frontend. > > 1 small change was made to the css and `ndt-browser-client.js` files in order to make the application still function properly. > > Finally, also added the ndt-library.js include into the widget.html. > > Once this PR and part 2 #55 is merged, #49 can be closed. > >

Comments from Reviewable

pboothe commented 8 years ago

I am taking a look right now.

Previously, critzo wrote… > :lgtm: But I would also like a second reviewer to take a look. @fernandalavalle or @mtlynch ?

Comments from Reviewable

pboothe commented 8 years ago

Overall comment so far is that this would benefit from a run through JSLint, it uses globals when it doesn't have to, and we should postpend all private variables with an underscore. I'll keep going through it (half done) but I wanted to give initial comments right away.

Previously, pboothe (Peter Boothe) wrote… > I am taking a look right now.

Comments from Reviewable

pboothe commented 8 years ago

Please put /*jslint bitwise: true, browser: true */ after the introductory comment in every file (if it's not already there) then run the JSlint commandline tool (or use the jslint website, but the commandline too is easier to run multiple times).

Previously, pboothe (Peter Boothe) wrote… > Overall comment so far is that this would benefit from a run through JSLint, it uses globals when it doesn't have to, and we should postpend all private variables with an underscore. I'll keep going through it (half done) but I wanted to give initial comments right away.

Review status: 0 of 5 files reviewed at latest revision, 35 unresolved discussions, all commit checks successful.

_HTML5-frontend/ndt-browser-client.js, line 87 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > Does our style guideline allow for one line conditionals?

"Short conditional statements may be written on one line if this enhances readability. You may use this only when the line is brief and the statement does not use the else clause." - from https://google.github.io/styleguide/cppguide.html#Conditionals which is included by reference from https://google.github.io/styleguide/javascriptguide.xml?showone=Code_formatting#Code_formatting

So yes. This is fine.

_HTML5-frontend/ndt-library.js, line 2 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > I think it's the reverse – the other file is the library and this is the interface that provides callbacks and m-lab-ns support.

I'd like to call a moratorium on the word Interface here, where we could be using it in both the sense of GUI and in the independent sense of "the abstract object and function signatures which are fleshed out somewhere else".

_HTML5-frontend/ndt-library.js, line 28 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > Since these are private instance variables do we denote them in any decorator or a prepended underscore?

"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_

_HTML5-frontend/ndt-library.js, line 38 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > Is this right?

Agree, please just delete the line.

_HTML5-frontend/ndt-library.js, line 40 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > This seems overloaded – a bool of True/False or an applet container?

A better pattern to use here is to return the test object or to throw an exception if the browser doesn't support websocket NDT.

_HTML5-frontend/ndt-library.js, line 44 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > I'm not sure that the class should expect NDT to be a window properties rather than an object passed in.

I agree here. Please don't use global variables unless you absolutely must.

_HTML5-frontend/ndt-library.js, line 50 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > I don't understand, if getServer fails, it does getServer again and then returns false?

Hopefully refactoring this to use exceptions will make it clearer.

_HTML5-frontend/ndt-library.js, line 81 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > First, I suspect we would consider this a private class property, but why do we set it that way, instead of returning it? i.e. this is "check" but it starts to make decisions. Also why are we falling back? In the interface this is an explicit option that is chosen. > >

I think that offering up a Java Applet is confusing in the year 2016, and we should not do it unless we must.

HTML5-frontend/ndt-library.js, line 122 [r1] (raw file):

    if (this.websocket_client) {
      return this.websocket_client;
    }

This is why we have else {. This is not a guard, instead the function is 

if (x) {
  a();
} else {
  b();
}```

When I describe this function in english, I find myself naturally using the word "otherwise", which means it needs an `else`.

---

*[HTML5-frontend/ndt-library.js, line 132 \[r1\]](https://reviewable.io:443/reviews/m-lab/ndt/54#-KHkQGHDqbQM4SLs_U8k-r1-132:-KHvGOmnLNkODwJMfjKO:-1719433933) ([raw file](https://github.com/m-lab/ndt/blob/81ab6032f062529bcf360801bc800bca95ca9f11/HTML5-frontend/ndt-library.js#L132)):*
<details><summary><i>Previously, collina (Collin Anderson) wrote…</i></summary>
> Style complaint? Either way, please break out it's a bit obtuse.</details>
Reads better with parens (because nobody remembers operator precedence rules outside of PEMDAS).
```javascript
var mlabNsService = ('https:' == location.protocol) ? 'ndt_ssl' : 'ndt';

_HTML5-frontend/ndt-library.js, line 134 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > We need to standardize on "self" or "that" for dealing with scope issues.

I prefer that, because it is the preferred style in "Javascript: The Good Parts" (funny image: http://blog.movereem.nl/JavaScript-sources/ )

_HTML5-frontend/ndt-library.js, line 136 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > Declarations at start of function.

jslint will catch this!

Comments from Reviewable

pboothe commented 8 years ago

_HTML5-frontend/ndt-library.js, line 70 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > already false in instantiation?

can be shortened and clarified to 

try {
  var ndt_js = new NDTjs();
  hasWebsockets = ndt_js.checkBrowserSupport();
} catch(e) {
  hasWebSockets = false;
}

Comments from Reviewable

shredtechular commented 8 years ago

Ran JSLint on the ndt-library.js & script.js files and addressed the issues it found Screen Shot 2016-05-31 at 12.39.05 PM.png

Previously, pboothe (Peter Boothe) wrote… > Please put > `/*jslint bitwise: true, browser: true */` > after the introductory comment in every file (if it's not already there) then run the JSlint commandline tool (or use the jslint website, but the commandline too is easier to run multiple times). >

Review status: 0 of 5 files reviewed at latest revision, 35 unresolved discussions, all commit checks successful.

_HTML5-frontend/ndt-library.js, line 2 [r1] (raw file):_

Previously, pboothe (Peter Boothe) wrote… > I'd like to call a moratorium on the word Interface here, where we could be using it in both the sense of GUI and in the independent sense of "the abstract object and function signatures which are fleshed out somewhere else". >

The intention of this code is an object that has methods for a custom front end to utilize to run NDT tests. So, I'm not really sure what I should do with these comments. Are you suggesting something else for the file overview?

_HTML5-frontend/ndt-library.js, line 19 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > NDTJSInterface? >

Not sure what this means, are you saying want to rename the object NDTJSInterface? Above comment says not to use "Interface". Do you want to call it something else?

_HTML5-frontend/ndt-library.js, line 24 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > This is asynchronous, right? If so, this won't block and the rest might not be populated as it progresses. >

That's correct, it is asynchronous. We have error trapping if it is not ready and the front end can call the function again.

_HTML5-frontend/ndt-library.js, line 28 [r1] (raw file):_

Previously, pboothe (Peter Boothe) wrote… > "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_ >

Added a trailing underscore to the private props and methods of the object and annotated them with @private, however (JSLint doesn't seem to like trailing underscores)[https://jslinterrors.com/unexpected-dangling-_-in-a].

_HTML5-frontend/ndt-library.js, line 38 [r1] (raw file):_

Previously, pboothe (Peter Boothe) wrote… > Agree, please just delete the line. >

Removed.

_HTML5-frontend/ndt-library.js, line 40 [r1] (raw file):_

Previously, pboothe (Peter Boothe) wrote… > A better pattern to use here is to return the test object or to throw an exception if the browser doesn't support websocket NDT. >

Added an exception and now returns an object.

_HTML5-frontend/ndt-library.js, line 44 [r1] (raw file):_

Previously, pboothe (Peter Boothe) wrote… > I agree here. Please don't use global variables unless you absolutely must. >

changed to not using global here, instead object property

_HTML5-frontend/ndt-library.js, line 46 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > var declaraction >

Became N/A after fixing other issues.

_HTML5-frontend/ndt-library.js, line 50 [r1] (raw file):_

Previously, pboothe (Peter Boothe) wrote… > Hopefully refactoring this to use exceptions will make it clearer. >

refactored to use exceptions now.

_HTML5-frontend/ndt-library.js, line 64 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > sets to false, then true, then false, I would assume false and then find if true. >

Fixed per @pboothe's suggestion

_HTML5-frontend/ndt-library.js, line 70 [r1] (raw file):_

Previously, pboothe (Peter Boothe) wrote… > can be shortened and clarified to > > ``` javascript > try { > var ndt_js = new NDTjs(); > hasWebsockets = ndt_js.checkBrowserSupport(); > } catch(e) { > hasWebSockets = false; > } > ``` > >

Fixed and returning true/false

_HTML5-frontend/ndt-library.js, line 77 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > if you assume true, then you might not need this line >

Simplified function based on @pboothe's suggestion.

_HTML5-frontend/ndt-library.js, line 81 [r1] (raw file):_

Previously, pboothe (Peter Boothe) wrote… > I think that offering up a Java Applet is confusing in the year 2016, and we should not do it unless we must. >

Simplified function based on @pboothe's suggestion. The whole point of this PR is to separate front end from backend in existing js. I think removing functionality/tests should be a separate PR.

_HTML5-frontend/ndt-library.js, line 82 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > Empty space. >

Fixed.

_HTML5-frontend/ndt-library.js, line 86 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > empty space >

Fixed.

_HTML5-frontend/ndt-library.js, line 88 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > if neither hasWebsocket or hasJava? >

Simplified function based on @pboothe's suggestion.

_HTML5-frontend/ndt-library.js, line 93 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > It's more of an HTML entity and not information, right? >

Fixed.

_HTML5-frontend/ndt-library.js, line 94 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > Where does "false" come from – looks like True or the HTML entity? >

Fixed.

_HTML5-frontend/ndt-library.js, line 97 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > I would opt for these functions to be populated with variables like use_websocket_client passed as parameters, rather that use instance properties >

Changed this instance to parameter and reviewed the rest to see if there were other instances like this. I think in some of the other methods it makes sense to use instance properties to limit what an implementer would need to do, so I think this is better now?

_HTML5-frontend/ndt-library.js, line 102 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > var declaration beyond start of function. >

Fixed

_HTML5-frontend/ndt-library.js, line 117 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > "Object"? >

Fixed

_HTML5-frontend/ndt-library.js, line 122 [r1] (raw file):_

Previously, pboothe (Peter Boothe) wrote… > This is why we have `else {`. This is not a guard, instead the function is > > `````` javascript > if (x) { > a(); > } else { > b(); > }``` > > When I describe this function in english, I find myself naturally using the word "otherwise", which means it needs an `else`. >

JSLint doesn't like the else and returns "Unnecessary 'else' after disruption." if I try:

``if (this.websocket_client) {
       return this.websocket_client;
   } else {
       return $('#NDT');
   }``

Were you wanting something else here?

_HTML5-frontend/ndt-library.js, line 132 [r1] (raw file):_

Previously, pboothe (Peter Boothe) wrote… > Reads better with parens (because nobody remembers operator precedence rules outside of PEMDAS). > > ``` javascript > var mlabNsService = ('https:' == location.protocol) ? 'ndt_ssl' : 'ndt'; > ``` > >

Fixed, used suggestion with parens.

_HTML5-frontend/ndt-library.js, line 134 [r1] (raw file):_

Previously, pboothe (Peter Boothe) wrote… > I prefer `that`, because it is the preferred style in "Javascript: The Good Parts" (funny image: http://blog.movereem.nl/JavaScript-sources/ ) >

OK, changed to that.

_HTML5-frontend/ndt-library.js, line 136 [r1] (raw file):_

Previously, pboothe (Peter Boothe) wrote… > jslint will catch this! >

Fixed.

_HTML5-frontend/ndt-library.js, line 141 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > Declarations at start of function. >

Fixed.

_HTML5-frontend/ndt-library.js, line 159 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > string >

Fixed.

_HTML5-frontend/ndt-library.js, line 167 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > string >

Fixed.

_HTML5-frontend/ndt-library.js, line 188 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > var declaration beyond start of function. >

Fixed.

_HTML5-frontend/ndt-library.js, line 231 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > As mentioned elsewhere, can this be done outside of a property of window? >

I thought the point of this entire PR was to make it as easy as possible for an implementer to make use of an NDT "library". From my perspective it seems like this would be easiest for an implementer rather than making them instantiate it? And since this would be properly namespaced with all other methods/properties attached to this object, it seems like this is okay from the (google doc)[https://google.github.io/styleguide/javascriptguide.xml?showone=Naming#Naming]? Maybe I'm not understanding what you guys have in mind here though?

"Use namespaces for global code ALWAYS prefix identifiers in the global scope with a unique pseudo namespace related to the project or library."

_HTML5-frontend/script.js, line 8 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > Can we at this point change the script ("script.js") filename to something that is more understandable? >

We can, but trying to limit the amount of changes for this PR and all this is designed to do is to separate frontend from backend code. Plus, as I understand it the process says,

"Don’t mix code moves/renames with other changes Moving code around needs to be done in its own pull request because otherwise the reviewer can’t easily see what changed in the code after it was moved. The diff can’t distinguish between move and move+change."

https://docs.google.com/document/d/1Uhd_BUiwwNgBd4KcyfBKqUhOvuEZMAoqOtWCVXVbvkE/edit#

_HTML5-frontend/script.js, line 65 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > var declaration beyond start of function. >

Fixed.

_HTML5-frontend/script.js, line 452 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > var declaration beyond start of function. >

Fixed.

Comments from Reviewable

collina commented 8 years ago

Review status: 0 of 5 files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

_HTML5-frontend/ndt-library.js, line 2 [r1] (raw file):_

Previously, shredtechular (Andrew Newhouse) wrote… > The intention of this code is an object that has methods for a custom front end to utilize to run NDT tests. So, I'm not really sure what I should do with these comments. Are you suggesting something else for the file overview? >

While Peter disagrees with my use of Interface, my broader concern was the labeling as "library" – it's more an intermediary between NDTjs and the web page, so if you could find another name than library, I think it would be helpful.

_HTML5-frontend/ndt-library.js, line 19 [r1] (raw file):_

Previously, shredtechular (Andrew Newhouse) wrote… > Not sure what this means, are you saying want to rename the object NDTJSInterface? Above comment says not to use "Interface". Do you want to call it something else? >

Yes, something else.

_HTML5-frontend/ndt-library.js, line 81 [r1] (raw file):_

Previously, shredtechular (Andrew Newhouse) wrote… > Simplified function based on @pboothe's suggestion. The whole point of this PR is to separate front end from backend in existing js. I think removing functionality/tests should be a separate PR. >

Agreed.

HTML5-frontend/ndt-library.js, line 122 [r1] (raw file):

Previously, shredtechular (Andrew Newhouse) wrote… > JSLint doesn't like the else and returns "Unnecessary 'else' after disruption." if I try: > > ``` > ``if (this.websocket_client) { > return this.websocket_client; > } else { > return $('#NDT'); > }`` > ``` > > Were you wanting something else here? >

JSLint seems correct here, I think the original code was correct.

_HTML5-frontend/ndt-library.js, line 231 [r1] (raw file):_

Previously, shredtechular (Andrew Newhouse) wrote… > I thought the point of this entire PR was to make it as easy as possible for an implementer to make use of an NDT "library". From my perspective it seems like this would be easiest for an implementer rather than making them instantiate it? And since this would be properly namespaced with all other methods/properties attached to this object, it seems like this is okay from the (google doc)[https://google.github.io/styleguide/javascriptguide.xml?showone=Naming#Naming]? Maybe I'm not understanding what you guys have in mind here though? > > "Use namespaces for global code > ALWAYS prefix identifiers in the global scope with a unique pseudo namespace related to the project or library." >

We can punt this to later.

_HTML5-frontend/script.js, line 8 [r1] (raw file):_

Previously, shredtechular (Andrew Newhouse) wrote… > We can, but trying to limit the amount of changes for this PR and all this is designed to do is to separate frontend from backend code. Plus, as I understand it the process says, > > "Don’t mix code moves/renames with other changes > Moving code around needs to be done in its own pull request because otherwise the reviewer can’t easily see what changed in the code after it was moved. The diff can’t distinguish between move and move+change." > > https://docs.google.com/document/d/1Uhd_BUiwwNgBd4KcyfBKqUhOvuEZMAoqOtWCVXVbvkE/edit# >

Ack. If you can put in a PR after pull with the rename, I'll approve it right away.

Comments from Reviewable

shredtechular commented 8 years ago

Reviewed 1 of 5 files at r1. Review status: 1 of 5 files reviewed at latest revision, 33 unresolved discussions, all commit checks successful.

_HTML5-frontend/ndt-library.js, line 2 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > While Peter disagrees with my use of Interface, my broader concern was the labeling as "library" – it's more an intermediary between NDTjs and the web page, so if you could find another name than library, I think it would be helpful. >

Do you have a suggestion here if you don't like the "library" label?

Comments from Reviewable

shredtechular commented 8 years ago

Review status: 1 of 5 files reviewed at latest revision, 33 unresolved discussions, all commit checks successful.

_HTML5-frontend/ndt-library.js, line 19 [r1] (raw file):_

Previously, collina (Collin Anderson) wrote… > Yes, something else. >

If not Library or Interface, any other suggestions then?

Comments from Reviewable