Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setup Twitch Timers #5

Merged
merged 7 commits into from
Jan 8, 2021
Merged

Setup Twitch Timers #5

merged 7 commits into from
Jan 8, 2021

Conversation

matanp
Copy link
Owner

@matanp matanp commented Jan 5, 2021

Messages bot should send at given intervals.

Data contained in timers.json -
{
message: what to say to twitch,
time: time in seconds before saying message if more than minLines of chat have been logged,
minLines: minimum lines before message repeated,
prioritizeLines: if true - will not wait until time before saying message out
}

@matanp matanp requested a review from unfamiliarish2 January 5, 2021 09:06
twitch_main.js Outdated
@@ -58,4 +74,13 @@ function onMessageHandler(target_channel, user_info, user_msg, from_self) {
// Called every time the bot connects to Twitch chat
function onConnectedHandler(addr, port) {
console.log(`* Connected to ${addr}:${port}`);
for (let timer of timer_data.timers) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using more descriptive names than timer_data, timer, and also probably timer.time
thought on timer.time - on name alone, it's not clear to me whether this is the time counted by the timer so far, or something else - I know from reading it that this is how often the timer should "go off", but had to dig more into the code to get that out

What do the timers do? Is it more important that they're timing something, or performing an action periodically? What does timer.time refer to, what happens at time?
(i know functionally, but these are prompts for naming)

Try for a name that describes their functionality, so that this code is more self-documenting. I know I advocate for code that is named for actual functionality and not use case, but be cautious about going too generic

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

struggling with the naming. named the json object repeated_messages_out, didn't change the other names

twitch_main.js Outdated
Comment on lines 80 to 83
if (timer.countLines >= timer.minLines) {
timer.countLines = 0;
client.say(channelOut, timer.message);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love that this logic is duplicated on lines 58-61 - probably break into a function

It's odd to me that you're storing copies of timer.countLines. This is fine, but I'd recommend:

  • having a running count of the total messages submitted by users
  • checking/modding this value against time.minLines to determine printing

Yeah, modding is probably more time costly than simple addition, but all of these actions are negligible (also storing the duplicate data is negligible), but I'd argue this would improve readability (which is mostly what I'm after)

something like (tho this might not exactly work):

if (user_message_count % timer.minMessages > 0) {
  do something
}

This will also simplify your logic in onMessageHandler, and if you break out the duplicated logic I highlighted above, can make it a simply deterministic function with no side effects

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought: there's gotta be some standard for setting non-global, state variables that get instantiated on program start and made available during runtime. Too lazy to google it, but could be helpful (esp since I think I nudged you before to not use global vars)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

storing a running count of total messages has slight functional implications I want to avoid.

I'm trying to calculate the number of messages in the timer period, which needs to be separate for each timer, because the timers don't all have the same period.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that was why I mentioned modding

can you clarify what the "functional implications [you] want to avoid" are?
I wonder if instead of storing the count for each timer, you could instead have a generic timers.msgCount, to avoid dup and also global functional issues

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed on pulling in state variables. I think medium long term I will have have some sort of config file stored in a database, so I can write a way to modify it using a frontend, then call it from the backend.

Copy link
Owner Author

@matanp matanp Jan 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't what I'm doing modding? the functional implications are what I described in the 2nd paragraph. each timer has a different period, and lines check. I don't want one timer 'going off' to affect the lines checks for other timers. that's why I want to count lines per timer.

sorry if I'm confusing what you're meaning.

@@ -10,6 +11,8 @@ const bot = require("./bot_brain.js");

const connectOBSWebsocket = require("./obs_helper").connect;

const channelOut = process.env.CHANNEL;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This var is duplicated with client_config.channels - is that intentional?

What is the distinction between channelOut and target_channel referenced in onMessageHandler? Is it possible that these would ever be different channels?

Your code below speaks to channelOut on line 60 and target_channel on line 68 - if these should be the same channel, consider just using one var

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

intentional - ish. I only have target_channel in onMessageHandler and I'm now saying things out from other places. changed all uses to channelOut, for consistency

matanp added 3 commits January 8, 2021 00:06
made tab width consistent to 4 across all files
reformat package.json
@matanp matanp merged commit 5bd94d4 into main Jan 8, 2021
@matanp matanp deleted the timers branch January 8, 2021 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants