May I have feedback for my pomodoro clock?

Hey everyone. I’ve more or less finished my pomodoro clock. It passes all tests, and all that’s left on my to-do list is to write some media queries to make it look better on small screens, and switch from text to icons on the buttons. I focused on modularizing my source code and on getting some practice with Redux. Would love to know what you think.

Source code: https://github.com/spblewis/pomodoro/tree/main/src

Live: https://spblewis.github.io/pomodoro/

Looking through the code for anything that catches my eye…


in src/index.js

const mapStateToProps = (state) => ({
    appState: state,
});

const mapDispatchToProps = (dispatch) => ({
  
});

const App = connect(mapStateToProps, mapDispatchToProps)(Pomodoro);

First of all, you don’t usually map all the state. That kind of defeats the point of breaking your state up into different sections. That may not be apparent on a small app, but it’s a good habit to get into. I would expect something like:

const mapStateToProps = (state) => ({
  session: state.session,
  breakLength: state.breakLength,
});

And if you don’t have anything for mdtp, just leave it out.

Also, I wouldn’t do this at the root level of your app. Just do it where it is needed - that is one of the big values of redux - that you don’t need to keep state in your root component and pass it down everywhere. Just use mstp and mdtp where you need them. If you have a child that is a dumb component and you want to handle that in the parent, that can be cool, just don’t overdo it.


In src/components/break-control.js

    const decrementBreak = () => {
        store.dispatch(breakDecrement());
    }

This is what mdtp is for.


In src/components/timer.js

    useEffect(() => {
        if (appState.timeLeft === 0) {
            currentAudio.play();
        }
    });

You don’t have any dependencies for your useEffect hooks. Doesn’t that mean that they will run every render? It seems like unnecessary overhead for something that could just be run in the body of the component.

    useEffect(() => {
        let timer;
        if (appState.running) {
            timer = setTimeout(() => {
                store.dispatch(tick())
            }, 1000);
        } 
        return () => clearTimeout(timer);
    })

Isn’t this going to create a new timer on every render? Maybe I’m wrong.


In src/reducers/reducer.js

            return Object.assign({}, state, {
                running: true,
            });

Most people would write that as:

            return { ...state, running: true };

I see that you added in thunk here. Did you use a thunk? I didn’t see one. I wouldn’t expect there to be thunks here because of the lack of async operations. I mean, even redux is kind of overkill here, but it makes sense as a learning exercise.


All in all, it looks pretty good. Keep in mind that I’m a very picky code reviewer.

I think some of your formatting is unusual, but not atrociously so. If you’re developing locally, I might suggest a linter. They are a pain in the ass at first, but they teach you to be consistent in your formatting which makes code easier to read which makes it easier to write and debug.

But still, good work. Have fun with data visualization.

Thanks very much for this. This is very useful feedback. Particularly with attention to the hooks. The FCC curriculum doesn’t cover hooks, and I knew what I wanted to do if the component was a class component, but I wanted to figure out how hooks worked. Once it functioned, I was pretty pleased, but I’m afraid I still don’t understand very well. I also don’t understand very well what triggers a component to re-render, and how to optimize for that. I’ll look into it while refactoring.

I’ll take out the thunk, too. Early in the process, my idea was that I’d use react, redux, and all related tools, to get some practice. I never thought to take thunk back out when it wasn’t necessary.

I might suggest a linter. They are a pain in the ass at first, but they teach you to be consistent in your formatting which makes code easier to read which makes it easier to write and debug.

This is excellent advice. I got eslint set up for my d3 projects, and it’s been very helpful so far. It was indeed a bit of a headache to get working, but it’s worth it. I’ve seen a lot of guides where eslint is set up alongside Prettier, but they sound like they have a similar function. Do you have any thoughts on whether it is worthwhile to use them together, or whether one is sufficient?

Right. I expect that hooks will be in the future. But yeah, anything that you can do with a class and lifecycle methods you can do with a FC and hooks. The hooks don’t always line up exactly with how lifecycle methods work, but there is a way to do it.

I’ll take out the thunk, too. Early in the process, my idea was that I’d use react, redux, and all related tools, to get some practice. I never thought to take thunk back out when it wasn’t necessary.

Yeah, I get that. If you had to make some async call that would populate the redux store when done, that would have made sense.

got eslint set up for my d3 projects, and it’s been very helpful so far

Cool. Yeah, it can be a pain. (Whoever woke up and though, “Do I need more nagging in my life?”) But they also do a great job training you and if you are working on a project with other people they can save a lot of hassle.

As to Prettier, I haven’t used it much, but I think there is some overlap there. But prettier is a code formatter. So, prettier will actually (intelligently) format your code. Eslint won’t change your code for you (unless you tell it to) but it will point out problem areas. They can work together no problem, but you need to make sure that the settings for each are compatible. I’m sure you can find guides online.

This topic was automatically closed 182 days after the last reply. New replies are no longer allowed.