Initial commit #1
Conversation
|
quick note that the dependencies listed in package.json are not used in anything in the initial commit, just dependences for beta functionality. |
There was a problem hiding this comment.
Great work! This is a fun project, and I'm stoked to see where you go with it! 🤹
Most of my comments have to do with formatting, organization, or naming/style conventions.
Let me know if you want me to look back over it after you make your updates, or if you'd prefer just a first pass from me 🌻
.gitignore
Outdated
| @@ -0,0 +1,3 @@ | |||
| node_modules/ | |||
| .env | |||
| log/ | |||
There was a problem hiding this comment.
Consider sorting these alphabetically and also by type. Maybe:
log/
node_modules/
.env
twitch_main.js
Outdated
| // Called every time a message comes in | ||
| // target is the twitch channel the message is coming from | ||
| // context is information about the user -- see example_context | ||
| // msg is the actual message | ||
| // self is a boolean of if the message is coming from this client | ||
| function onMessageHandler (target, context, msg, self) { |
There was a problem hiding this comment.
Here's the Google style guide for js:
https://google.github.io/styleguide/jsguide.html
I'm linking it cause I used the python one a lot for standards on writing docstring when I first started at Freenome.
I am totally bikeshedding, but some things here you could tweak:
- Multi-line comments in js are
/* */, and - I've hyperlinked here how google recommends formatting js docstring
Optionally also consider using typescript for typing. Probably not worth for now since the project is young, but would be worth checking out if it grows.
There was a problem hiding this comment.
thank you for bikeshedding, especially since I'm so new to javascript. definitely thinking about switching over to typescript
twitch_main.js
Outdated
| // Called every time a message comes in | ||
| // target is the twitch channel the message is coming from | ||
| // context is information about the user -- see example_context | ||
| // msg is the actual message | ||
| // self is a boolean of if the message is coming from this client | ||
| function onMessageHandler (target, context, msg, self) { |
There was a problem hiding this comment.
There's also this idea of "self-documenting code", which if you haven't heard of it before gets at the idea of structuring your code and naming your variables, functions, etc, in such a way that you can minimize inclusion of comments. Grabbed the first link that seemed okay
Again, small project, but the idea is that over time, because they're not required to be accurate (like the actual code), comments commonly fall out of date. As a result, it's a good idea to minimize use of comments as much as possible.
An alternative to your current function signature could be:
function onMessageHandler (sending_twitch_channel, user_info, user_msg, msg_from_self) {There was a problem hiding this comment.
great idea to be more self documenting, I like these names better (went with something slightly different, but similar)
twitch_main.js
Outdated
| //twitch client | ||
| const tmi = require('tmi.js'); | ||
|
|
||
| //and we need the bot | ||
| const { bot } = require('./bot_brain.js'); |
There was a problem hiding this comment.
Lol so I know these come from this tutorial, but I'd rename these:
tmi->twitch_clientopts->options
Since I think this is easier to understand ⭐
There was a problem hiding this comment.
Whoops, grabbed the wrong line for opts
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Inconsistent indentation. These are 4 spaces, but earlier you have 2
bot_brain.js
Outdated
| if (r_num > 9) { | ||
| msg_out = "matanbot is currently under development, please excuse the lack of functionality" | ||
| } else if (r_num > 8){ | ||
| msg_out = "I'm so glad someone recognizes my potential as being the best bot on the channel" | ||
| } else if (r_num > 6) { | ||
| msg_out = "I am a juggle bot, that is all I am" |
There was a problem hiding this comment.
Nice easter eggs! 😛
bot_brain.js
Outdated
| var r_num = Math.floor((Math.random() * 10) + 1); | ||
| var msg_out = ""; |
There was a problem hiding this comment.
These should be consts
There was a problem hiding this comment.
msg_out can't be a const because I am changing it in the function body - is there a cleaner way to do this so I'm not modifying the value of the variable?
| function message_main (context, msg) { | ||
| // Remove whitespace from chat message | ||
| const commandName = msg.trim(); | ||
|
|
There was a problem hiding this comment.
Personally, I would take out this extra line, but that's just my style.
More bikeshedding that is up to you and/or whatever linter you maybe pick up
bot_brain.js
Outdated
| const upper_case = commandName.toUpperCase(); | ||
|
|
||
| //random number 1 to 10 | ||
| var r_num = Math.floor((Math.random() * 10) + 1); |
There was a problem hiding this comment.
I would also rename r_num to a more obvious name (like random_num), but up to you
https://dzone.com/articles/naming-conventions-from-uncle-bobs-clean-code-phil
bot_brain.js
Outdated
| count = count + 1; | ||
| if (r_num > 9) { | ||
| msg_out = "matanbot is currently under development, please excuse the lack of functionality" | ||
| } else if (r_num > 8){ | ||
| msg_out = "I'm so glad someone recognizes my potential as being the best bot on the channel" |
There was a problem hiding this comment.
Optionally break these if statement bodies into their own functions, which would remove the extra layers of nesting here.
Probably put the r_num statement into this if body, unless it is intended to be used elsewhere.
Consider also renaming count to a more descriptive name.
There was a problem hiding this comment.
If you do break them out, I have been working on keeping to a pretty functional programming style, which I would also recommend.
It allows for super deterministic plug-n-play methods, which also minimizing coupling of your code (cough workday cough), allowing for easier future modifications/expansion to your code
There was a problem hiding this comment.
not going to do this for this push - but definitely considering it going forward! would love to chat about how best to structure the functions, and what exactly they should take in and pass out. Ideally, I'd like some re-usable function to read some json file or something with all the commands, so I can add functionality w/o really adding code. Not sure entirely how I wanna do that yet though.
|
going to close and merge the PR here, don't wanna get too caught up on style right now. I have a bunch of beta code that would be a better use of time to be working on. thank you for reviewing! much appreciated |
This is the initial commit.
Twitch_main.js is the entry point and uses the main method from bot_brain.js
twitch main sets up twitch client and handles messages coming in - sometimes sending message back out.
https://dev.twitch.tv/docs/irc has documentation on setting up twitch client.