Pomodoro Clock Feedback Required, Please Rate from 1 - 10

Hey There Guys,

I just finished working on Pomodoro Clock and am now looking for some healthy feedback/criticism. Please provide your valuable feedback and feel free to rate this project from 1 - 10.

Pomodoro Clock Links

Features

  • Responsive
  • Plays a relaxing music when break time starts
  • Music can be turned off with a button
  • Reset Button
  • Used Flex Box for layout.
  • No Bootstrap
  • Vanilla Javascript

I Used SCSS in the making for the first time ever and I must say that I am never going back to plain CSS3 ever again if not necessary.

Hi.

First thing that caught my attention is that most of the code is duplicated.

function decrement(targetId) {
    var currentVal, newVal;
    if (targetId === "session-time") {
        targetId = document.getElementById(targetId);
        currentVal = parseInt(targetId.value);
        if (currentVal <= 0) {
            return false;
        } else {
            newVal = (currentVal - 1).toString();
            targetId.value = newVal;
            document.getElementById('clock').innerHTML = formatTime(parseInt(newVal) * 60);
            sessionInSeconds = parseInt(document.getElementById('session-time').value) * 60;
            breakinSeconds = parseInt(document.getElementById('break-time').value) * 60;
        }
    } else {
        targetId = document.getElementById(targetId);
        currentVal = parseInt(targetId.value);
        if (currentVal <= 0) {
            return false;
        } else {
            newVal = (currentVal - 1).toString();
            targetId.value = newVal;
            sessionInSeconds = parseInt(document.getElementById('session-time').value) * 60;
            breakinSeconds = parseInt(document.getElementById('break-time').value) * 60;
        }
    }
}

Can be refactored to

function decrement(targetId) {
    var currentVal, newVal;
    targetId = document.getElementById(targetId);
    currentVal = parseInt(targetId.value);
    if (currentVal <= 0) {
        return false;
    } else {
        newVal = (currentVal - 1).toString();
        targetId.value = newVal;
    }

    if (targetId === "session-time") {
      document.getElementById('clock').innerHTML = formatTime(parseInt(newVal) * 60);
    }
}

You can do likewise with other methods too.
So see if you have the same code repeated in if/else blocks and if so, move common code out of the block.

Hope this helps.

1 Like

Hi,

Thank you so much for the time you spent for this thorough review mate. I really appreciate your effort. Actually I know I repeated a lot of code as I was making this app in a hurry. I will refactor my code now as you suggested. It makes the code more readable as well.

EDIT: Some of the code needs repetition as required for desired functionality i.e sessionInSeconds & breakinSeconds

Any other suggestion?

Thanks again.

This is what I should have done according to your suggestion.

function decrement(targetId) {
var currentVal, newVal;
targetId = document.getElementById(targetId);
currentVal = parseInt(targetId.value);

if (currentVal <= 0) {
    return false;
} else {
    newVal = (currentVal - 1).toString();
    targetId.value = newVal;
    sessionInSeconds = parseInt(document.getElementById('session-time').value) * 60;
    breakinSeconds = parseInt(document.getElementById('break-time').value) * 60;
}

if (targetId === "session-time") {
    document.getElementById('clock').innerHTML = formatTime(parseInt(newVal) * 60);
}

Yeah, sorry I left out some code as I’m on my phone and it’s not easy to see lines of code.
Refactor looks good now.

If you look closer you’ll realise increment looks very similar to decrement, save a check on currentVal so you can probably merge them to shrink your code further.

Have a go at it, I’ll be back tomorrow when I’m on my computer.

Have fun!

1 Like

increment & decrement are being called via HTML on seperate buttons. I am not sure if they can be merged.

I am refactoring all my code now and will leave a post here so that you can recheck my code again if you like.

I have refactored my code and re-positioned all important variable declarations to the top. Provision of comments that explain my code are still remaining which will be done sometime later & I think my code is quite self explanatory as well. Please review again so I can learn more.

OK, how’s this:

/**
  * @param {string} targetId
  * @param {integer} increment_value can be 1 for increasing or -1 for decreasing
  */
function updateCounter(targetId, increment_value) {
    var currentVal, newVal;
    var target = document.getElementById(targetId);
    currentVal = parseInt(target.value);

    if (currentVal <= 0 || currentVal >= 60) {
        return false;
    }

    newVal = currentVal + increment_value; // no need to parseString here
    target.value = newVal;

    sessionInSeconds = parseInt(sessionTime.value) * 60;
    breakinSeconds = parseInt(breakTime.value) * 60;

    if (targetId === "session-time") {
        clock.innerHTML = formatTime(newVal * 60); 
    }
}

Then in HTML:

            <div class="session-time-control">
                <h2>Session Time</h2>
                <div class="btn-container">
                    <button type="button" class="left-border control-btns black" onclick="updateCounter('session-time', -1)">-</button>
                    <input id="session-time" type="text" value="25">
                    <button type="button" class="right-border control-btns black" onclick="updateCounter('session-time', 1)">+</button>
                </div>
            </div>

So if your counter is at 12 and you call updateCounter('session-time', -1) it will do 12 + -1 effectively decreasing by 1.
We just shrunk code by almost 50%.

And to tweak further you could have a function to update sessionInSeconds and breakinSeconds as it’s repeated in three places too:

function updateSessionAndBreak() {
    sessionInSeconds = parseInt(sessionTime.value) * 60;
    breakinSeconds = parseInt(breakTime.value) * 60;
}

If you find yourself copy-pasting then move code to a function.

1 Like

You just unlocked some doors in my brain with this sentence.

“If you find yourself copy-pasting then move code to a function.”

I had previously used this technique but didn’t actually memorize it and your sentence has made me to remember it for eternity. And yeah Hats off to you for that update counter function although I had used this method before as well but couldn’t figure out that -1 || +1 idea of yours. I guess I need some getting used to using more functions in my code.

Any learning resources you could point me towards? I would love to be in contact with you as well if you don’t mind can I have any social media link of yours

Thanks a lot mate, There are very limited number of people who take the time to provide such a thorough review.

Well aside from programming aspect what was your overall experience with this app?

Thanks for the kind words. Glad to be able to pass some of my knowledge to someone.

As for resources, you can find lots of free books on JavaScript on https://devfreebooks.github.io/javascript/
You can’t avoid spending time coding, looking at others’ code, checking other programming languages. Acquiring programming techniques and problem solving skills takes time. Working through challenges on this site should prove highly useful.

Then there’s the famous mantra: make it work, make it right, make it fast
Once you have your code to do what it should, it’s time to refactor, just like we did. To do that safely though I would advise to backup your code with tests. It’s a safety net. If your original version passes 10 tests then so should your refactored code. It makes the whole refactor process a lot smoother.

You can contact me using FCC PM feature for any questions. :slight_smile:

Finally, your app was fine. Problem is I like to listen to music as I work so it gets in the way of your sound notification!

1 Like

Thank you for this amazing link. And yeah I do look at others code as it definitely teaches lots of new things & best practices. And for other languages I believe I should first learn programming concepts deeply using one language (JS feels like the best choice because of it’s versatility) and then move on to learning some other languages so that I can make my self familiar with different syntax and concepts.

You taught this mantra to me very well for which I am really thankful. Well I have not gone that far to do TDD (Test driven development) but that’s on my learning list and will be doing that soon. I will sure be in contact with you and will be asking you out to review my Tic Tac Toe project that I will be working on by tonight.

And finally the mute button is there for a reason. :wink: :smile:

EDIT: clean-code-javascript this link tells all the things you told me. Thanks again for the link.

TBH, out of all the books in the link I pasted I have only started reading Chapter 1 of You Don’t Know JS a couple of months ago and then forgot about it. Silly me. I should get back to reading it!

Aight, looking forward to reviewing your Tic Tac Toe challenge.

Have fun!

1 Like