Code Improvement: #821
Code Improvement: #821
Conversation
Using inline if
|
Thank you for your effort. I dont think that there are any peformance gains here, and personally find this to be less readable code. |
|
No problem dude. It is not a performance improvement. It is a personal suggestion to avoid unnecessary lines. Cheers ;) |
anonrig
left a comment
anonrig
left a comment
There was a problem hiding this comment.
Thank you for the PR. I've added some comments to improve readability. Can you fix it?
lib/Slack_web_api.js
Outdated
| data.token = config.token; | ||
| } | ||
|
|
||
| data.token = data.token ? data.token : config.token; |
There was a problem hiding this comment.
data.token = data.token || config.token will make it more readable.
There was a problem hiding this comment.
The problem with the || operator is that if the var is not defined it will throw an error.
Example:
testFunction(i) {
i = i || 10; //PERFECT
let t = t || 10; //ERROR because t is not defined
}
There was a problem hiding this comment.
If data is defined on the same scope and in the same line, JS interpreter will throw an error. You're right. But the thing is data variable is defined before this line and therefore will not throw an error if data.token is undefined.
Example:
> var data = {}
< undefined
> data.token = data.token || 3;
< 3
> data.token
< 3There was a problem hiding this comment.
Cool!!!
I will change it ;)
Thanks man!
lib/Slack_web_api.js
Outdated
|
|
||
| data.client_id = data.client_id ? data.client_id : bot.config.clientId; | ||
| data.client_secret = data.client_secret ? data.client_secret : bot.config.clientSecret; | ||
| data.redirect_uri = data.redirect_uri ? data.redirect_uri : bot.config.redirectUri; |
There was a problem hiding this comment.
Please use the OR operator for these operations
lib/Slack_web_api.js
Outdated
| if (!currentGroup[nextGroupName]) { | ||
| currentGroup[nextGroupName] = {}; | ||
| } | ||
| currentGroup[nextGroupName] = currentGroup[nextGroupName] ? currentGroup[nextGroupName] : {}; |
There was a problem hiding this comment.
Again currentGroup[nextGroupName] = currentGroup[nextGroupName] || {}
| } else { | ||
| slack_api.callAPI('files.upload', options, cb, false); | ||
| } | ||
| slack_api.callAPI('files.upload', options, cb, !!options.file); |
| return cb(null, json); | ||
| } | ||
| return cb(json.error, json); | ||
| return cb((json.ok ? null : json.error), json); |
|
Hey so why did this get merged in? There are only performance losses with ternary operators when replacing If/Else blocks in Javascript (although they are completely negligible here), and I don't find this to be more readable code, especially for newer devs (which botkit has lots of). @anonrig can you give some context to your choice here? |
4 participants
Using inline if