5
\$\begingroup\$

I would like a code review for my simple pomodoro utility web page because I want to improve my coding ability.

<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">
<html>
<head>
    <meta http-equiv="refresh" content="55">
    <meta http-equiv="Content-type" content="text/html; charset=UTF-8">
    <meta name="viewport" content="width=device-width,initial-scale=1">
    <title>Send Pomodoro alarm</title>
</head>

<body>
<h1>Pomodoro work life balance</h1>
            
<h2>Enter a start time and stop time...</h2>
<p> Enter a start time in 24-hour format like "8:00" for 8:00 AM: <input type="text" id="startTime" size="5" value="8:00" onclick="startTimeInput()">
<p> Enter a stop time in 24-hour format like "17:00" for 5:00 PM: <input type="text" id="stopTime" size="5" value="17:00" onclick="stopTimeInput()">
<br>
<br>    
<h2>...add your work and break time intervals...</h2>   
<p> Enter work interval minutes: <input type="number" id="workInterval" size="1" value="25" min="1" max="99"> 
<p> Enter break interval minutes: <input type="number" id="breakInterval" size="1" value="5" min="1" max="99">
<br>
<br>
<input type="button" value="Add work and break intervals" onclick="addBreakResumeTimes()">
<br> 
<br>            
<input type="button" value="Show break times" onclick="showBreakTimes()"> <input type="button" value="Show resume times" onclick="showResumeTimes()">
<br> 
<br> 
<p>Note:<br> When the first entered time arrives, a 'pop-up is blocked' notification will appear on the right corner of the address bar. Click on the notification and select 'Always allow..'. Click the Ok button. The break and resume time pop-up notification alert message will be displayed.             every time a break or resume time arrives.
<br>
<br>
You may also add your break and resume to work times one by one in a 12-hour format like "8:25 AM".<br>
Or you may also add multiple break and resume times in a 12-hour format separated by commas like "8:25 AM, 8:55 AM, 9:25 AM, ... 8:30 AM, 9:00 AM, 9:30 AM, ...,"
</p>            
<h3> Break from work time:</h3>
<input type="text" id="breakInputField" size="70" value="8:25 AM, 8:55 AM, 9:25 AM, ...," onclick="breakInputFieldClick()"> 
<input type="button" value="Add" onclick="addBreakFromWorkTimes()">
            
<h3> Resume to work time:</h3>
<input type="text" id="backToWorkInputField" size="70" value="8:30 AM, 9:00 AM, 9:30 AM, ...," onclick="backToWorkInputFieldClick()"> 
<input type="button" value="Add" onclick="addbackToWorkTimes()">
<br> 
<br>
<input type="button" value="Show break times" onclick="showBreakTimes()"> <input type="button" value="Show resume times" onclick="showResumeTimes()">       
<br>
<br>
            
<h4> Delete an unneeded 'break from work' time:</h4> Enter the number of nth break time that you want to delete starting from the top left in the break times list. <input type="button" value="Show break times" onclick="showBreakTimes()">
<br>
<br>
<input type="number" id="breakTimeIndex" size="1" onclick="breakTimeIndexClick()" min="1" max="99">
<input type="button" value="Delete" onclick="deletePomodoroTime()">
<br>
<br>
<h4> Delete an unneeded 'resume to work' time:</h4> Enter the number of nth resume time that you want to delete starting from the top left in the resume times list. <input type="button" value="Show resume times" onclick="showResumeTimes()">
<br>
<br> 
<input type="number" id="resumeTimeIndex" size="1" onclick="resumeTimeIndexClick()" min="1" max="99">
<input type="button" value="Delete" onclick="deleteBackToWorkTime()">
<br> 
<br> 
<h4>Clear all break and resume times:</h4> <input type="button" value="Clear break times" onclick="clearBreakTimes()"> <input type="button" value="Clear resume times" onclick="clearResumeTimes()">
    
<script type="text/javascript">         
        
var breakFromWorkArray = [];
var backToWorkArray = [];
var breakFromWorkTimes; // Break from work times input  
var backToWorkTimes; //  Resume to work times input
var breakTimeString; // Convert breakTime to string to compare to currentTime
var resumeTimeString; // Convert resumeTime to string to compare to currentTime
var currentTime;

//Convert time to string in hour and minute.        
currentTime = String(new Date().toLocaleTimeString("en-US", {
    hour12: true,
    hour: "numeric",
    minute: "numeric"
}));

if (sessionStorage.getItem("breakFromWorkTimes") != null) {
    breakFromWorkArray = JSON.parse(sessionStorage.getItem("breakFromWorkTimes"));
}
if (sessionStorage.getItem("backToWorkTimes") != null) {
    backToWorkArray = JSON.parse(sessionStorage.getItem("backToWorkTimes"));
}

loopArray();

function toTimeString(dateObj) {
    return dateObj.toLocaleTimeString("en-US", {
        hour12: true,
        hour: "numeric",
        minute: "numeric"
    });
}

function startTimeInput() {
    document.getElementById("startTime").value = "";
}

function stopTimeInput() {
    document.getElementById("stopTime").value = "";
}

function addBreakResumeTimes() {
    var resumeInterval = document.getElementById("breakInterval").value;
    var resumeDuration = parseFloat(resumeInterval);
    var workInterval = document.getElementById("workInterval").value;
    var breakDuration = parseFloat(workInterval);
    var breakTime = new Date();
    var breakHours;
    var breakMinutes;
    var resumeTime = new Date();
    var resumeHours;
    var resumeMinutes;
    var startTime;
    var startTimeObject = new Date();
    var stopTime;
    var stopTimeObject = new Date();

    startTime = document.getElementById("startTime").value;
    stopTime = document.getElementById("stopTime").value;

    if (startTime == "" || stopTime == "" || resumeInterval == "" || workInterval == "") {
        alert("Please enter a start time, a stop time, a break time interval and a resume to work time interval.");
        return;
    }

    startTime = startTime.substring(0, 5);
    startTime = startTime.split(":");
    startTimeObject.setHours(startTime[0], startTime[1]);
    breakMinutes = startTimeObject.getMinutes();
    breakHours = startTimeObject.getHours();
    breakTime.setHours(breakHours, breakMinutes);
    resumeHours = startTimeObject.getHours();
    resumeMinutes = startTimeObject.getMinutes();
    resumeTime.setHours(resumeHours, resumeMinutes);
    stopTime = stopTime.substring(0, 5);
    stopTime = stopTime.split(":");
    stopTimeObject.setHours(stopTime[0], stopTime[1]);

    if (breakTime > stopTimeObject && resumeTime > stopTimeObject) {
        var newStartTimeObject = new Date();

        newStartTimeObject.setHours(1, 0);
        breakMinutes = newStartTimeObject.getMinutes();
        breakHours = newStartTimeObject.getHours();
        breakTime.setHours(breakHours, breakMinutes);
        resumeHours = newStartTimeObject.getHours();
        resumeMinutes = newStartTimeObject.getMinutes();
        resumeTime.setHours(resumeHours, resumeMinutes);
        while (breakTime < stopTimeObject && resumeTime < stopTimeObject) {
            breakMinutes = resumeMinutes + breakDuration;

            /* if (breakMinutes == 0)
            {
                breakMinutes = resumeMinutes + breakDuration;                   
            }    */
            breakTime.setHours(breakHours, breakMinutes);

            resumeMinutes = breakMinutes + resumeDuration;
            resumeTime.setHours(resumeHours, resumeMinutes);

            breakTimeString = toTimeString(breakTime);
            breakFromWorkTimes = breakTimeString;
            breakFromWorkArray.push(breakFromWorkTimes);

            resumeTimeString = toTimeString(resumeTime);
            backToWorkTimes = resumeTimeString;
            backToWorkArray.push(backToWorkTimes);
        }
        sessionStorage.setItem("breakFromWorkTimes", JSON.stringify(breakFromWorkArray));
        sessionStorage.setItem("backToWorkTimes", JSON.stringify(backToWorkArray));
        alert("Break and resume time intervals were added.");

        breakMinutes = startTimeObject.getMinutes();
        breakHours = startTimeObject.getHours();
        breakTime.setHours(breakHours, breakMinutes);
        resumeHours = startTimeObject.getHours();
        resumeMinutes = startTimeObject.getMinutes();
        resumeTime.setHours(resumeHours, resumeMinutes);
        stopTimeObject.setHours(23, 59);
        while (breakTime < stopTimeObject && resumeTime < stopTimeObject) {
            breakMinutes = resumeMinutes + breakDuration;
            /* if (breakMinutes == 0)
                {
                    breakMinutes = resumeMinutes + breakDuration;                   
                }    */
            breakTime.setHours(breakHours, breakMinutes);

            resumeMinutes = breakMinutes + resumeDuration;
            resumeTime.setHours(resumeHours, resumeMinutes);

            breakTimeString = toTimeString(breakTime);
            breakFromWorkTimes = breakTimeString;
            breakFromWorkArray.push(breakFromWorkTimes);

            resumeTimeString = toTimeString(resumeTime);
            backToWorkTimes = resumeTimeString;
            backToWorkArray.push(backToWorkTimes);
        }
        sessionStorage.setItem("breakFromWorkTimes", JSON.stringify(breakFromWorkArray));
        sessionStorage.setItem("backToWorkTimes", JSON.stringify(backToWorkArray));
        alert("Break and resume time intervals were added.");
    } else {
        while (breakTime < stopTimeObject && resumeTime < stopTimeObject) {
            breakMinutes = resumeMinutes + breakDuration;

            /* if (breakMinutes == 0)
            {
                breakMinutes = resumeMinutes + breakDuration;                   
            }    */
            breakTime.setHours(breakHours, breakMinutes);

            resumeMinutes = breakMinutes + resumeDuration;
            resumeTime.setHours(resumeHours, resumeMinutes);

            breakTimeString = toTimeString(breakTime);
            breakFromWorkTimes = breakTimeString;
            breakFromWorkArray.push(breakFromWorkTimes);

            resumeTimeString = toTimeString(resumeTime);
            backToWorkTimes = resumeTimeString;
            backToWorkArray.push(backToWorkTimes);
        }
        sessionStorage.setItem("breakFromWorkTimes", JSON.stringify(breakFromWorkArray));
        sessionStorage.setItem("backToWorkTimes", JSON.stringify(backToWorkArray));
        alert("Break and resume time intervals were added.");
    }
}

function addBreakFromWorkTimes() {
    breakFromWorkTimes = document.getElementById("breakInputField").value;
    if (breakFromWorkTimes == "") {
        alert("Please enter a break time in 0:00 AM or PM format.");
    } else {
        var ctr = 0;
        breakFromWorkTimes = breakFromWorkTimes.split(", ");
        while (ctr < breakFromWorkTimes.length) {
            breakFromWorkArray.push(breakFromWorkTimes[ctr]);
            ctr = ctr + 1;
        }

        if (ctr == 1)
            alert("Break time was added.");
        else if (ctr > 1)
            alert("Break times were added.");
        sessionStorage.setItem("breakFromWorkTimes", JSON.stringify(breakFromWorkArray));
        //alert("Break time(s) was/were added.");
    }
}

function addbackToWorkTimes() {
    backToWorkTimes = document.getElementById("backToWorkInputField").value;
    if (backToWorkTimes == "") {
        alert("Please enter a break time in 0:00 AM or PM format.");
    } else {
        var ctr = 0;
        backToWorkTimes = backToWorkTimes.split(", ");
        while (ctr < backToWorkTimes.length) {
            backToWorkArray.push(backToWorkTimes[ctr]);
            ctr = ctr + 1;
        }

        if (ctr > 1)
            alert("Resume times were added.");
        else if (ctr == 1)
            alert("Resume time was added.");
        sessionStorage.setItem("backToWorkTimes", JSON.stringify(backToWorkArray));
        //alert("Resume time(s) was/were added.");
    }
}

function showBreakTimes() {
    if (breakFromWorkArray.length == 0) {
        alert("Break times is empty. Please add break times.");
    } else {
        alert(breakFromWorkArray);
    }
}

function showResumeTimes() {
    if (backToWorkArray.length == 0) {
        alert("Resume times is empty. Please add resume times");
    } else {
        alert(backToWorkArray);
    }
}

function deletePomodoroTime() {
    var ctr = document.getElementById("breakTimeIndex").value;

    if (breakFromWorkArray.length == 0) {
        alert("Break times is empty");
        return;
    }

    if (ctr == "") {
        alert("Enter the number of nth resume time starting from the top left in the resume times list.");
    } else {
        var index = parseFloat(ctr);
        breakFromWorkArray.splice(index - 1, 1);
        breakFromWorkTimes = breakFromWorkArray;
        sessionStorage.setItem("breakFromWorkTimes", JSON.stringify(breakFromWorkArray));
        alert("Break time was cleared.");
    }
}

function deleteBackToWorkTime() {
    var ctr = document.getElementById("resumeTimeIndex").value;

    if (backToWorkArray.length == 0) {
        alert("Break times is empty");
        return;
    }

    if (ctr == "") {
        alert("Enter the number of nth resume time starting from the top left in the resume times list.");
    } else {
        var index = parseFloat(ctr);
        backToWorkArray.splice(index - 1, 1);
        backToWorkTimes = backToWorkArray;
        sessionStorage.setItem("backToWorkTimes", JSON.stringify(backToWorkArray));
        alert("Resume time was cleared.");
    }

}

function clearBreakTimes() {
    while (breakFromWorkArray.length) {
        while (breakFromWorkArray.length > 0) {
            sessionStorage.removeItem("breakFromWorkTimes");
            breakFromWorkArray = [];
        }

        alert("Break times were cleared.");
        return;
    }

    if (breakFromWorkArray.length == 0) {
        alert("Break times is empty. Please add resume times.");
    }
}

function clearResumeTimes() {
    while (backToWorkArray.length) {
        while (backToWorkArray.length > 0) {
            sessionStorage.removeItem("backToWorkTimes");
            backToWorkArray = [];
        }

        alert("Resume times were cleared.");
        return;
    }

    if (backToWorkArray.length == 0) {
        alert("Resume times is empty. Please add resume times.");
    }
}

function loopArray() {
    var alarmWindow;
    var breakAlertArray;
    var resumeAlertArray;
    var breakMessage;
    var resumeMessage;
    var ctr = 0;

    //Loop the arrays while breakFromWorkArray is not equal to currentTime or backToWorkArray is not equal to currentTime
    while (breakFromWorkArray[ctr] != currentTime || backToWorkArray[ctr] != currentTime) {
        // If pomodoro time is equal to current time, alarm for pomodoro with pop-up message
        //alert(breakFromWorkArray[ctr]);
        //alert(currentTime);
        if (breakFromWorkArray[ctr] == currentTime) {
            //alert(breakFromWorkArray[ctr]);
            alarmWindow = window.open("", "BreakTime", "width=350,height=50");
            alarmWindow.focus();
            breakAlertArray = breakFromWorkArray[ctr] + "." + "</p>";
            breakMessage = "<p>Time for a break because it is now ";
            alarmWindow.document.write(breakMessage + breakAlertArray);
            setTimeout(function() {
                alarmWindow.close();
            }, 50000);
            break;
        }

        // If back to work time is equal to current time, alarm for back to work with pop-up message                
        if (backToWorkArray[ctr] == currentTime) {
            alarmWindow = window.open("", "ResumeToWork", "width=375,height=50");
            alarmWindow.focus();
            resumeAlertArray = backToWorkArray[ctr] + "." + "</p>";
            resumeMessage = "<p>Time to resume to work because it is now ";
            alarmWindow.document.write(resumeMessage + resumeAlertArray);
            setTimeout(function() {
                alarmWindow.close();
            }, 50000);
            break;
        }

        //break from the ctr if the iteration reach the last elements of the pomodoro and back to work time arrays
        if (breakFromWorkArray[ctr] == breakFromWorkArray[breakFromWorkArray.length] && backToWorkArray[ctr] === backToWorkArray[backToWorkArray.length]) {
            break;
        }

        //increment ctr count
        ctr = ctr + 1;
    }
}

</script>
</body>
</html>
\$\endgroup\$
1
  • \$\begingroup\$ Anything in particular you want to focus on? It's a big piece of code, so it might be a long review you get back as well :) \$\endgroup\$
    – Icepickle
    Aug 1, 2020 at 0:36

1 Answer 1

7
\$\begingroup\$

UI

First thing to look at would probably the ease of use of the webpage itself. You could start by using typed input controls, like time, ie:

<input type="time" step="00:30" value="10:00" id="startTime" />

Those would still render as text inputs for any browser that might not be able to render it, but it would give an easier way of inputting the data from a user perspective.

On the subject of user experience, I found it really weird that my previous inputs got erased once I clicked on the fields. I assume you were going for a placeholder effect, but for me, it felt a bit weird.

Furthermore, I have no idea what it is exactly that I have to do on the page. I seem to be able to add work an break times, but nothing that indicates visually to me, if I have started it or not.

So, I would suggest you have a way to visualize the work times, it would definitely make it easier for the user to interact with a visual interpretation of the data than just saying the n-th interval.

I could also add a "resume to work time" that was set before a "break time", which seems contradictory.

All in all, a lot of the problems that I would have with the setup is:

  • Not seeing a visual representation of break / resume times
  • Not seeing if I started the process
  • inputs that reset when clicking them (honestly I even had an input reset, when I was trying to correct a mistake, for me sometimes it goes faster to go back with the mouse)
  • You have a button that adds all breaks and resume times, but no button that erases both (but you do have two separate buttons for that)
  • After I added the automatic break and resume times, I can add an extra resume time in between
  • Mixture of time formats between start / stop time and add manual break / resume times
  • Use required attribute, indicating a user automatically that something must be set before you can continue

Personally, I would also prefer the <button /> element over <input type="button" />

Alerts

All interactions with the page seem to go over alerts, that feels off. I could have added to the above list, but I think this illustrates the best what is wrong and that is that the user doesn't see something but the input fields.

Javascript

As a rule, don't mix JavaScript and html elements. Currently, input every element and every button has a html onclick attribute. This makes it a very tight coupling between html and the code behind it.

For example, the text fields all have a "clear" event on them. Although I am not in favor of it, you could have gone with a single clear function for all input elements.

function clearElement( e ) {
  e.target.value = '';
}

Array.from( document.querySelectorAll('input[type=text]') )
  .forEach( element => element.addEventListener('click', clearElement ) );
<input type="text" value="a" />
<input type="text" value="b" />
<input type="text" value="c" />

Global variables

The start of your code defines several global variables. For the current usage this might be fine, but can be handled differently. Global variables are like magic, especially for people who didn't write the code themselves, or for yourself when you might look at your code at a later time.

So define variables where you need them explicitly.

Local storage

You read and write data from and to the local store. That's a good place to store the data you need, but as you have also seen, you need to transform the data upon each read and write. You can make it a lot easier for yourself if you would abstract the localstorage away in a utility class or utility functions.

  • it would allow you to exchange the localstorage for another option in the future
  • you can leave the data manipulation over to the abstraction
  • it will be easier to test

loopArray() function

For some reason you are using a while loop, but you should be using Array.prototype.find here instead. It would allow you to instantly get the currentTime in an array or undefined.

Instead of using the <meta http-equiv="refresh" content="55" />, you could use setInterval here, with adding loopArray()

setInterval( loopArray, 55000 );

It would run the loopArray function every 55 seconds and you wouldn't have unintended page reloads.

Declaration

var is fine, but let and const are generally better

D(ont) R(epeat) Y(ourself)

The loopArray function calls two "different" windows and sets an interval to close them, write a method and parametrize the things that are different, then call the single method with the different arguments

Similarly, you have repeat the same work for getting hours and minutes from a string value. This is something that really should be refactored to a new function. By moving repeating code in their own little function, you also reduce problems in case you might have an error in that repeating code (you don't have to fix it all over the code base but just in one place).

For example, you could rewrite it to the following

const timeParser = {
  fromInput( elem ) {
    const parts = elem.value.split(':', 2).map( Number );
    return {
      hours: parts[0],
      minutes: parts[1]
    };
  }
}

const dateProvider = {
  fromTime( time ) {
    const date = new Date();
    date.setHours( time.hours );
    date.setMinutes( time.minutes );
    return date;
  },
  increaseWith( date, minutes ) {
    const result = new Date( date.valueOf() );
    result.setMinutes( date.getMinutes() + minutes );
    return result;
  },
  getTime( date ) {
    return {
      hours: date.getHours(),
      minutes: date.getMinutes()
    };
  }
}

function getIntervals( startDate, endDate, workInterval, breakInterval ) {
  // don't mutate input arguments when it's not expected
  // not making startsAt a new object would introduce side effects
  let currentTime = new Date( startDate.valueOf() );
  const endsAt = endDate;
  // when endDate is smaller than startDate, it's a night shift :)
  if (endsAt < currentTime) {
    endsAt.setDate( endsAt.getDate() + 1 );
  }
  
  const intervals = [];
  while (currentTime < endsAt) {
    const start = dateProvider.getTime( currentTime );
    currentTime = dateProvider.increaseWith( currentTime, workInterval );
    // if the work interval exceeds the endsAt, choose endsAt instead
    const end = currentTime > endsAt ? dateProvider.getTime( endsAt ) : dateProvider.getTime( currentTime );
    intervals.push( { start, end } );
    currentTime = dateProvider.increaseWith( currentTime, breakInterval );
  }
  return intervals;
}

function getElement( id ) {
  return document.getElementById( id );
}

document.getElementById('allbreaks').addEventListener('submit', e => {
  e.preventDefault();
  const startTime = timeParser.fromInput( getElement('startTime') );
  const endTime = timeParser.fromInput( getElement('endTime') );
  console.log( 
    getIntervals( 
      dateProvider.fromTime( startTime ), 
      dateProvider.fromTime( endTime ),
      parseInt( getElement( 'workInterval' ).value ),
      parseInt( getElement( 'breakInterval' ).value )
    )
  );    
});
<form id="allbreaks">
  <div>
    <label for="startTime">Start time</label>
    <input type="time" id="startTime" required />
  </div>
  <div>
    <label for="endTime">End time</label>
    <input type="time" id="endTime" required />
  </div>
  <div>
    <label for="workInterval">Work interval</label>
    <input type="number" min="5" max="720" step="5" value="25" id="workInterval" required />
  </div>
  <div>
    <label for="breakInterval">Break interval</label>
    <input type="number" min="5" max="720" step="5" value="5" id="breakInterval" required />
  </div>
  <button>Add work and break intervals</button>
</form>

Adding ranges to an array

If you want to add a range to an array, just spread the assignments, for example:

const test = 'a, b, c, d, e, f';
const arr = [];

arr.push(...test.split(',').map( t => t.trim() ) );
console.log( arr );

Use arguments

Currently none of your functions have arguments, you retrieve all data from the inputs and that functions uses the data. Having utility functions like the once in the above shown code, makes it easier to reuse the functionality

function length

The addBreakResumeTimes is a hefty function where you repeat similar code 3 times. It a function gets to long, it becomes harder to read. So refactor the code in to more manageable blocks. If you see the refactoring of the code, you can see I wrote it more concise and it covers all the use cases you have.

If one of those blocks has an error that is replicated in all 3 blocks, then you have to fix that error in all 3 of the blocks

User input

Validate user input. In the above example, I used string.prototype.trim to at least ensure I don't have any whitespace leftover after the split

Use objects to define your intervals

I would argue that a work interval is defined as a defined time from a certain start to a certain end. By putting start & end of an interval together, you have an easier way of manipulating the data afterwards.

For example, if I would like to erase a break, I could simply move the start of one object together with the end of the next object, say:

  • Interval 8:00-8:25 and 8:30-8:55
  • Erase the end time for the first interval, would give me 8:00-8:55
\$\endgroup\$
3
  • \$\begingroup\$ The succeeding comments are missing. I do not know what happened. I really appreciate your guidance. \$\endgroup\$ Aug 4, 2020 at 0:16
  • \$\begingroup\$ @cabpacedilla Comments are not first class citizens so they can be removed at any time. Once you completed your rework of the code, you can create a follow up question and link in this post. Most of the stuff is part of my answer anyhow, except for reworking how to access the local storage. I flagged the comments as no longer needed (or rather, 1 of them but all got removed) \$\endgroup\$
    – Icepickle
    Aug 4, 2020 at 10:54
  • \$\begingroup\$ Thank you very much for the clarification. I try my best with the improvement of my code from your suggestions. \$\endgroup\$ Aug 4, 2020 at 16:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.