3
\$\begingroup\$

I created a simple clock & calendar system using a fictional system: everything from the number of seconds in a minute, down to the number of months in a year could be different from our accepted norms (i.e. the Gregorian Calendar). (For the purposes of testing the system I have declared that there are 2 seconds in a minute and the calendar uses Elder Scrolls naming for months and days of the week.)

Eventually I'd like to incorporate this into a game, so the function setInterval that currently increments the seconds is a simple stand-in for a more sophisticated kind of game loop.

The full code (with visual output) can be found at this JSFiddle.

I'm particularly interested in feedback on this method, which handles the incrementation of time:

function incrementOneSecond() { secondIndex++; currentSecond = secondIndex; if (secondIndex > secondsPerMinute) { secondIndex = 0; minuteIndex++; currentMinute = twoDigitFormat(minuteIndex); if(minuteIndex > minutesPerHour) { minuteIndex = 0; hourIndex++; currentHour = twoDigitFormat(hourIndex); if (hourIndex > hoursPerDay) { hourIndex = 0; dayIndex++; currentDay = weekdays[dayIndex]; if (dayIndex >= weekdays.length) { dayIndex = 0; dateNumberIndex++; currentDateNumber = dateNumberIndex; if (dateNumberIndex > monthMaxDays) { dateNumberIndex = 1; monthIndex++; currentMonth = months[monthIndex]; if (monthIndex >= months.length) { monthIndex = 0; yearIndex++; currentYear = yearIndex; } } } } } } updateTime(currentMinute, currentHour, currentDay, currentMonth, currentDateNumber, currentYear); }

There's a lot of repetition, so it feels like I should be able to DRY it up, but because each if statement differs slightly, I haven't been able to come up with a way of rewriting the method in a way that is more efficient but also still readable.

Other methods used:

function twoDigitFormat(input) { formatted = ("0" + input).slice(-2) return formatted; } function updateTime(currentMinute, currentHour, currentDay, currentMonth, currentDateNumber, currentYear) { timeDisplay.innerHTML = currentHour + ':' + currentMinute; dateDisplay.innerHTML = currentDay + '<br/>' + currentMonth + ' ' + currentDateNumber + ', ' + currentYear; }
\$\endgroup\$
2
  • \$\begingroup\$ "morning star" means satan and 88 means heil hitler... the first date being 'Morning Star 1, 1888' hmmmm \$\endgroup\$ Commented Dec 4, 2017 at 20:48
  • \$\begingroup\$ Yikes! That wasn't what I was going for at all. Morning Star is part of the Elder Scrolls calendar and 1888 was chosen because the game I'm making has a Victorian steampunk/horror theme, and that is the year of the Jack the Ripper murders. \$\endgroup\$ Commented Dec 5, 2017 at 9:29

3 Answers 3

Reset to default
1
\$\begingroup\$

Encapsulation

When your projects get bigger, a clear separation between individual parts of your code becomes more important. Reduce interdependency, limit the scope of your variables, keep things local and well contained. All this facilitates reasoning about and understanding of your code as well as debugging, testing individual parts, reuse and more.

The properties and methods needed to store and update a custom date are a good fit for a custom object type, a class.

You would start with modeling a simple date. A concise representation of a date is given by the seconds passed since 1888, similar to a Unix timestamp which counts the seconds passed since 1970. All other variables such as minute, hour, day, month or year can be computed from that timestamp, given we know how many seconds there are in a minute, hour, day and so on.

To incrementing such a date by one second, you would simply increment the timestamp.

It is a good idea to hide the logic which derives these dependent variables from the timestamp behind getter and setter methods. This would allow you to e.g. introduce leap seconds later on without having to substantially change the interface of your date class and thereby the code which interacts with these dates.

It is equally helpful to separate your model from the way it will later be represented on the screen. Formatting a date depends on the user's language and locale. Integrating a formatted date string into the user interface should be part of the view layer of your application.

Here is one possible way of encapsulating a custom date within a class and some formatting functions:

class CustomDate { constructor(timestamp = 0, secondsPerMinute = 2, minutesPerHour = 60, hoursPerDay = 24, daysPerWeek = 7, daysPerMonth = 30, monthsPerYear = 12) { this._secondsPerMinute = secondsPerMinute; this._secondsPerHour = this._secondsPerMinute * minutesPerHour; this._secondsPerDay = this._secondsPerHour * hoursPerDay; this._secondsPerWeek = this._secondsPerDay * daysPerWeek; this._secondsPerMonth = this._secondsPerDay * daysPerMonth; this._secondsPerYear = this._secondsPerMonth * monthsPerYear; this.timestamp = timestamp; } set timestamp(timestamp) { this._timestamp = timestamp; } get timestamp() { return this._timestamp; } get second() { return this._timestamp % this._secondsPerMinute; } get minute() { return Math.floor(this._timestamp % this._secondsPerHour / this._secondsPerMinute); } get hour() { return Math.floor(this._timestamp % this._secondsPerDay / this._secondsPerHour); } get day() { return Math.floor(this._timestamp % this._secondsPerWeek / this._secondsPerDay); } get date() { return Math.floor(this._timestamp % this._secondsPerMonth / this._secondsPerDay + 1); } get month() { return Math.floor(this._timestamp % this._secondsPerYear / this._secondsPerMonth + 1); } get year() { return Math.floor(this._timestamp / this._secondsPerYear + 1888); } } function formatTime(customDate) { const hour = customDate.hour.toString().padStart(2, '0'); const minute = customDate.minute.toString().padStart(2, '0'); return `${hour}:${minute}`; } function formatDate(customDate) { const days = [ 'Morndas', 'Tirdas', 'Middas', 'Turdas', 'Fredas', 'Loredas', 'Sundas' ]; const months = [ 'Morning Star', 'Sun\'s Dawn', 'First Seed', 'Rain\'s Hand', 'Second Seed', 'Midyear', 'Sun\'s Height', 'Last Seed', 'Hearthfire', 'Frostfall', 'Sun\'s Dusk', 'Evening Star' ]; const day = days[customDate.day]; const month = months[customDate.month - 1]; return `${day} <br> ${month} ${customDate.date}, ${customDate.year}`; } // Example: const date = new CustomDate(); console.log(formatTime(date), formatDate(date)); // Increment by two seconds: date.timestamp += 2; console.log(formatTime(date), formatDate(date));

\$\endgroup\$
1
\$\begingroup\$

Feedback

The code seems to work okay, though it appears that the counting is off because of the comparison operators used (i.e. >) and the initial value of the indexes (e.g. secondIndex = 0). Use Greater than Or equal to (i.e. >=) in order to accurately count with the correct number of seconds per minute, minutes per hour, etc.

Your code doesn't display the value of currentSecond but I added it (see code snippet below) and noticed it was going between values of 1, 2 and 3.

Suggestions

Pre-increment instead of post-increment and use immediately after

Instead of incrementing each value using the post-increment operator and then using it, use the pre-increment operator.

So lines like this:

secondIndex++; currentSecond = secondIndex;

Can be converted to a single line like this:

currentSecond = ++secondIndex;

Parameter passing

function incrementOneSecond() references all variables defined with scope outside of it (e.g. currentSecond, currentMinute, etc.), so why pass those to updateTime()? For consistency, either pass them all to incrementOneSecond() when calling it, or just reference them the same way within updateTime().

var dateDisplay = document.getElementById('dateDisplay'); var btnNext = document.getElementById('next'); var weekdays = [ 'Morndas', 'Tirdas', 'Middas', 'Turdas', 'Fredas', 'Loredas', 'Sundas' ]; var months = [ 'Morning Star', 'Sun\'s Dawn', 'First Seed', 'Rain\'s Hand', 'Second Seed', 'Midyear', 'Sun\'s Height', 'Last Seed', 'Hearthfire', 'Frostfall', 'Sun\'s Dusk', 'Evening Star' ]; var monthMaxDays = 30; var yearStart = 1888; var secondsPerMinute = 2; var minutesPerHour = 30; var hoursPerDay = 24; var currentSecond = currentSecond || 0; var currentMinute = currentMinute || twoDigitFormat(0); var currentHour = currentHour || twoDigitFormat(0); var currentDay = currentDay || weekdays[0]; var currentDateNumber = currentDateNumber || 1; var currentMonth = currentMonth || months[0]; var currentYear = currentYear || yearStart; var secondIndex = currentSecond; var minuteIndex = currentMinute; var hourIndex = currentHour; var dayIndex = parseInt(weekdays.indexOf(currentDay), 10); var dateNumberIndex = currentDateNumber; var monthIndex = parseInt(months.indexOf(currentMonth), 10); var yearIndex = currentYear; var secondLoop = setInterval(incrementOneSecond, 500); function twoDigitFormat(input) { return ("0" + input).slice(-2) } function incrementOneSecond() { currentSecond = ++secondIndex; if (secondIndex >= secondsPerMinute) { secondIndex = 0; currentMinute = twoDigitFormat(++minuteIndex); if (minuteIndex >= minutesPerHour) { minuteIndex = 0; currentHour = twoDigitFormat(++hourIndex); if (hourIndex >= hoursPerDay) { hourIndex = 0; currentDay = weekdays[++dayIndex]; if (dayIndex >= weekdays.length) { dayIndex = 0; currentDateNumber = ++dateNumberIndex; if (dateNumberIndex > monthMaxDays) { dateNumberIndex = 1; currentMonth = months[++monthIndex]; if (monthIndex >= months.length) { monthIndex = 0; currentYear = ++yearIndex; } } } } } } updateTime(); } function updateTime() { timeDisplay.innerHTML = currentHour + ':' + currentMinute + ':' + currentSecond; dateDisplay.innerHTML = currentDay + '<br/>' + currentMonth + ' ' + currentDateNumber + ', ' + currentYear; }
html, body { font-family: sans-serif; font-size: 1rem; line-height: 1.2rem; }
<span id='timeDisplay'></span> <br/> <span id='dateDisplay'></span>

\$\endgroup\$
1
\$\begingroup\$ 
secondIndex++; currentSecond = secondIndex; if (secondIndex > secondsPerMinute) { secondIndex = 0; /*...*/ }

First of all, as Sam said, use >= instead of >, or it will count one higher than intended.

Secondly, you are setting the display value before you check if it should be reset. So your currentMinute shows 1-30 instead of 0-29, and it doesn't follow the minuteIndex. It's even worse for the week/month days, as you get those from an array. After Sundas it shows undefined, and then goes to Tirdas, skipping Morndas.

Update the display value after the following if, when you are done updating the index value.

secondIndex++; if (secondIndex >= secondsPerMinute) { secondIndex = 0; minuteIndex++; if (minuteIndex >= minutesPerHour) { minuteIndex = 0; hourIndex++; if (hourIndex >= hoursPerDay) { hourIndex = 0; dayIndex++; if (dayIndex >= weekdays.length) { dayIndex = 0; dateNumberIndex++; if (dateNumberIndex >= monthMaxDays) { dateNumberIndex = 1; monthIndex++; if (monthIndex >= months.length) { monthIndex = 0; yearIndex++; currentYear = yearIndex; } currentMonth = months[monthIndex]; } currentDateNumber = dateNumberIndex; } currentDay = weekdays[dayIndex]; } currentHour = twoDigitFormat(hourIndex); } currentMinute = twoDigitFormat(minuteIndex); } currentSecond = secondIndex;
\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.