Feedback #1
Feedback #1
Conversation
| @@ -253,16 +253,15 @@ class Scratch3MQTTBlocks { | |||
| } | |||
|
|
|||
| isConnected(){ | |||
| if ( ! this._client ) return false; | |||
| return this._client.connected; | |||
| return this._client | |||
There was a problem hiding this comment.
ternary operators can reduce functions to a single return statement
There was a problem hiding this comment.
Not sure if I understand this correctly. How does this return then the "connected attribute of the _client object ?"
There was a problem hiding this comment.
If this._client is defined we execute the ? this._client.connected statement, otherwise we return : false.
I've split up the statements into multiple lines, but it could have been one single line :)
return this._client ? this._client.connected : false
| this._client.end(); | ||
| } | ||
|
|
||
| if ( this.isConnected() ) { |
There was a problem hiding this comment.
refactored this to use the public interface instead :)
| this._client.on('error',function(){ | ||
|
|
||
| this._client.on('error', () => { |
There was a problem hiding this comment.
We'll need to use an arrow function here, those will automatically bind the this context inside the function to the one outside of it.
If we use a regular function, the this context inside of our closure is a new context and doesn't have the end() function defined.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions
There was a problem hiding this comment.
Hmm, interesting... I guess I need to start learning javascript properly :)
| @@ -303,60 +302,48 @@ class Scratch3MQTTBlocks { | |||
|
|
|||
| disconnectFromBroker(args) { | |||
| console.log("[mqtt] disconnecting from MQTT broker... "); | |||
| if ( this._client != null ) { this._client.end(); } | |||
| if ( this._client ) { this._client.end(); } | |||
There was a problem hiding this comment.
Using truthy and falsey comparison instead of comparing with null or undefined values can eliminate potential bugs; because unfortunately the javascript equality matrix is pretty complicated https://dorey.github.io/JavaScript-Equality-Table/unified/ 😢
| } | ||
|
|
||
| subscribe( args ) { | ||
| if ( !this.isConnected() ) { |
There was a problem hiding this comment.
Using early returns prevents nested if-statements and makes it a bit more readable when we're doing some validation before continuing.
| @@ -36,7 +37,9 @@ const base = { | |||
| }) | |||
| ] | |||
| }, | |||
| plugins: [] | |||
| plugins: [ | |||
| new NormalModuleReplacementPlugin(/^mqtt$/, "mqtt/dist/mqtt.js") | |||
There was a problem hiding this comment.
For some reason I couldn't get webpack to compile the mqtt dependency, so I had to manually refer it to the compiled source.
|
Aaaaaaaa :-), didn't look at the code, only the snippit on github.
Nice, thanks, will look at the merge if i have some time later this week
:-).
…On Sun, 3 Feb 2019 at 23:35, Gilles De Mey ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/extensions/scratch3_mqtt/index.js
<#1 (comment)>:
> @@ -253,16 +253,15 @@ class Scratch3MQTTBlocks {
}
isConnected(){
- if ( ! this._client ) return false;
- return this._client.connected;
+ return this._client
If this._client is defined we execute the ? this._client.connected
statement, otherwise we return : false.
I've split up the statements into multiple lines, but it could have been
one single line :)
return this._client ? this._client.connected : false
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AOeeOgCuD2iHZy3CpwXkbCZwY6VQownRks5vJ2QagaJpZM4afoJo>
.
|
I've done some small refactoring, tested locally by linking
scratch-vmandscratch-gui:)