[React] How does this code look to you?

Friend and I spent a few hours yesterday trying to get setInterval and useEffect to work on our little workout app. Brains were fried, and it works, but we're curious on your opinions on how we 've done it. Can't copy paste the code, but everything is inside the Timer.jsx file. https://github.com/callum-laing/react-playground/blob/main/src/components/Timer.jsx
import { useEffect, useState } from "react";
import "./Timer.css";

const Timer = () => {
const workoutCount = 5.0;
const [appState, setAppState] = useState({ count: workoutCount, state: "stopped" });
const [timer, setTimer] = useState(null)
const cooldownCount = 3;

//Pure Function
const newTick = (state, cooldownCount, workoutCount) => {
if (state.state == "stopped") {
return { count: workoutCount, state: state.state };
}

if (state.count > 0) {
return { count: Math.max(state.count - 0.05, 0), state: state.state };
} else {
// get below 0
if (state.state == "workout")
// in workout
return { count: cooldownCount, state: "cooldown" };
// in cooldown mode
else return { count: workoutCount, state: "workout" };
}
};

const startTimer = () => {
let interval = setInterval(() => {
const event = new CustomEvent("tick");
document.dispatchEvent(event)
}, 50)
setTimer(interval);
setAppState({ count: workoutCount, state: "workout" });
};

const stopTimer = () => {
clearInterval(timer);
setAppState({ count: workoutCount, state: "stopped" });
};

const toggleTimer = () => {
if (appState.state !== "stopped") {
stopTimer();
} else {
startTimer();
}
};

useEffect(() => {
let listener = document.addEventListener("tick", () => {
setAppState((prevState) => {
return newTick(prevState, cooldownCount, workoutCount)
});
});

return document.removeEventListener("tick", listener)
}, []);
import { useEffect, useState } from "react";
import "./Timer.css";

const Timer = () => {
const workoutCount = 5.0;
const [appState, setAppState] = useState({ count: workoutCount, state: "stopped" });
const [timer, setTimer] = useState(null)
const cooldownCount = 3;

//Pure Function
const newTick = (state, cooldownCount, workoutCount) => {
if (state.state == "stopped") {
return { count: workoutCount, state: state.state };
}

if (state.count > 0) {
return { count: Math.max(state.count - 0.05, 0), state: state.state };
} else {
// get below 0
if (state.state == "workout")
// in workout
return { count: cooldownCount, state: "cooldown" };
// in cooldown mode
else return { count: workoutCount, state: "workout" };
}
};

const startTimer = () => {
let interval = setInterval(() => {
const event = new CustomEvent("tick");
document.dispatchEvent(event)
}, 50)
setTimer(interval);
setAppState({ count: workoutCount, state: "workout" });
};

const stopTimer = () => {
clearInterval(timer);
setAppState({ count: workoutCount, state: "stopped" });
};

const toggleTimer = () => {
if (appState.state !== "stopped") {
stopTimer();
} else {
startTimer();
}
};

useEffect(() => {
let listener = document.addEventListener("tick", () => {
setAppState((prevState) => {
return newTick(prevState, cooldownCount, workoutCount)
});
});

return document.removeEventListener("tick", listener)
}, []);
GitHub
react-playground/src/components/Timer.jsx at main ยท callum-laing/re...
Contribute to callum-laing/react-playground development by creating an account on GitHub.
10 Replies
Joao
Joaoโ€ข10mo ago
That's not too bad actually. A bit messy but could be clean up easily by moving the timer onto a separate hook. I also really like that you are using toLocaleString to output the timer, instead of toFixed, very nice touch. As I said I would move the timer to an external file to clean things up (also makes it reusable if you want to have multiple timers running at once). And instead of using a custom event and listening for ticks every 50ms you can use requestAnimationFrame. It'll run more smoothly and you it's designed for things like that. Although the use of a custom event and listening for it it perfectly valid, but for something that runs so often I like requestAnimationFrame better. I'm also not a big fan of combining non-related pieces of information under the same call of useState. Here, count and state... mmm, well, I guess you could argue they are related, and you are modifying them at once so it kinda makes sense but it's something to consider though this is mostly preference. Try to avoid magic numbers as well: count: Math.max(state.count - 0.05, 0) what is this supposed to represent? It's not immediately obvious so maybe move that onto a variable with a descriptive name instead. For extra tidiness you should consider moving the possible states of the app onto a separate variable, making it an enum.
const APP_STATES = {
WORKOUT: 'workout',
COOLDOWN: 'cooldown',
STOPPED: 'stopped',
};

setAppState({ count: workoutCount, state: APP_STATES.STOPPED });
const APP_STATES = {
WORKOUT: 'workout',
COOLDOWN: 'cooldown',
STOPPED: 'stopped',
};

setAppState({ count: workoutCount, state: APP_STATES.STOPPED });
This is self-documenting so it helps readability and understanding how the app behaves. But also, helps to avoid silly typos like stopepd that would make it annoying to correct, plus it helps code editors provide you with autocompletion. As for code structure, it'd be better to move the components into its own folder and include the js and css files there. This is not a problem for a single component, but when you have 10, 20, 30... you'd have twice that amount of files. So, under components:
src/
components/
Timer/
Timer.jsx
Timer.css
AnotherComponent/
AnotherComponent.jsx
AnotherComponent.css
src/
components/
Timer/
Timer.jsx
Timer.css
AnotherComponent/
AnotherComponent.jsx
AnotherComponent.css
CDL
CDLโ€ข10mo ago
All valid feedback, thanks @Joao . I think after a few hours we just wanted the darn thing to work, so the mess is uhh... yeah sure, haha! I'm new to React, and my friend codes in C#/F# so it's new to both of us, we weren't aware of requestAnimationFrame, that's a neat alternative for sure
Joao
Joaoโ€ข10mo ago
For sure make it work first, that's the most important part! setInterval will do just fine, so don't worry about it for now. My main focus would be on making the timer into a hook to make things cleaner.
CDL
CDLโ€ข10mo ago
setInterval is when all hell broke loose LOL, it was easy up until that point, then it turned into a bit of a clown fiesta
Joao
Joaoโ€ข10mo ago
playing with time always is ๐Ÿ˜„ I can probably give you a quick example of how it would work, just a sec
Joao
Joaoโ€ข10mo ago
function loop(timestamp) {
const diff = timestamp - previousTick;
previousTick = timestamp;
loopCounter += diff;

if (loopCounter >= 1000) {
loopCounter = 0;

if (playing) {
decreaseCounter(timeInMs);

if (timeInMs <= 0) {
playing = false;
updateCounterBtn();
}
}
}

requestAnimationFrame(loop);
}
function loop(timestamp) {
const diff = timestamp - previousTick;
previousTick = timestamp;
loopCounter += diff;

if (loopCounter >= 1000) {
loopCounter = 0;

if (playing) {
decreaseCounter(timeInMs);

if (timeInMs <= 0) {
playing = false;
updateCounterBtn();
}
}
}

requestAnimationFrame(loop);
}
This is the basic structure, it's from here: https://codepen.io/D10f/pen/VwQVLNg
Joao
Joaoโ€ข10mo ago
It's basically a recursive function that calls itself as fast as the refresh rate of the screen allows it (making it ideal for smooth animations as the name suggests, also games). You can control how often the actual counter updates by dividing by 1000 (every second) 100 (every hundredth), etc. And here's a raw implementation in React. It's not on separate files because of Codepen, but ideally that's how you'd structure. https://codepen.io/D10f/pen/JjzWdYB
CDL
CDLโ€ข10mo ago
That's pretty neat! definitely feels a little nicer than the way we had it (even though it's not the exact code)
Joao
Joaoโ€ข10mo ago
Hope it helps!
CDL
CDLโ€ข10mo ago
Very much so, thanks for taking the time to respond ๐Ÿ™‚
Want results from more Discord servers?
Add your server