5
\$\begingroup\$

I wrote a Chrome extension that lists upcoming concerts for the next seven days, this month, and next month. I had trouble with the nextSeven() method, especially transitioning from dates like Dec. 31st to Jan. 1st, hence the really fugly if-statements. I was wondering if anyone had any advice to make the code more condensed. I would really appreciate any feedback!

$(document).ready(function() {
 var months = ['Jan. ', 'Feb. ', 'March ', 'April ', 'May ', 'June ', 'July ', 'Aug. ', 'Sept. ', 'Oct. ', 'Nov. ', 'Dec. '];
 var systemTime = new Date();
 $.ajax({
 url: 'http://nycmetalscene.com/',
 type: 'GET',
 success: function(data) {
 var parsedData = $.parseHTML(data);
 var parseResults = $(parsedData).find('p').text();
 var events = parseResults.split(/Sun\. |Mon\. |Tues\. |Wed\. |Thurs\. |Fri\. |Sat\. /g);
 events.splice(0, 1);
 var monthIndex = systemTime.getMonth();
 var day = systemTime.getDate();
 var testDay = systemTime.getDate();
 var week = [];
 var thisDay;
 getDays();
 $('#nextseven').on('click', function() {
 $('#concerts').empty();
 nextSeven();
 });
 $('#thismonth').on('click', function() {
 $('#concerts').empty();
 showMonth();
 });
 $('#nextmonth').on('click', function() {
 $('#concerts').empty();
 showNextMonth();
 });
 function getDays() {
 for (var i = 0; i < 7; i++) {
 if (day < 31) {
 week.push(day);
 day++;
 } else {
 week.push(day);
 day = 1;
 }
 }
 for (var i = 0; i < week.length; i++) {
 if (week[i] === 1 || week[i] === 31) {
 week[i] = ' ' + week[i] + 'st,';
 } else if (week[i] === 2 || week[i] === 22) {
 week[i] = ' ' + week[i] + 'nd,';
 } else if (week[i] === 3 || week[i] === 23) {
 week[i] = ' ' + week[i] + 'rd,';
 } else {
 week[i] = ' ' + week[i] + 'th,';
 }
 }
 }
 function nextSeven() {
 for (i = 0; i < week.length; i++) {
 thisDay = week[i];
 $.each(events, function(i, item) {
 var eventsstr = JSON.stringify(item);
 if (eventsstr.match(thisDay)) {
 if (monthIndex === 11 && testDay >= 26) {
 if (eventsstr.match(months[monthIndex]) || eventsstr.match(months[0])) {
 var concert = JSON.parse(eventsstr);
 $('#concerts').append('<p>' + concert + '</p>');
 }
 } else if (monthIndex < 11 && testDay >= 26) {
 if (eventsstr.match(months[monthIndex]) || eventsstr.match(months[monthIndex + 1])) {
 var concert = JSON.parse(eventsstr);
 $('#concerts').append('<p>' + concert + '</p>');
 }
 } else {
 if (eventsstr.match(months[monthIndex])) {
 var concert = JSON.parse(eventsstr);
 $('#concerts').append('<p>' + concert + '</p>');
 }
 }
 }
 });
 }
 }
 function showMonth() {
 $.each(events, function(i, item) {
 var eventsstr = JSON.stringify(item);
 if (eventsstr.match(months[monthIndex])) {
 var concert = JSON.parse(eventsstr);
 $('#concerts').append('<p>' + concert + '</p>');
 }
 });
 }
 function showNextMonth() {
 $.each(events, function(i, item) {
 var eventsstr = JSON.stringify(item);
 if (eventsstr.match(months[monthIndex + 1] || months[0])) {
 var concert = JSON.parse(eventsstr);
 $('#concerts').append('<p>' + concert + '</p>');
 }
 });
 }
 }
 });
});
<!DOCTYPE html>
<html>
<head>
 <meta http-equiv="content-type" content="text/html; charset=utf-8 ;">
 <script src=jquery-3.1.1.min.js></script>
 <script type="text/javascript" src="Scrape.js"></script>
</head>
<body>
 <div>
 <h1>NYC Concerts</h1>
 <div id="alltabs">
 <ul class="tab">
 <center>
 <a href="#" onclick="return false;" class="tablinks" id="nextseven">Next 7 Days</a>
 <a href="#" onclick="return false;" class="tablinks" id="thismonth">This Month</a>
 <a href="#" onclick="return false;" class="tablinks" id="nextmonth">Next Month</a>
 </center>
 </ul>
 <h2><div id="concerts"></div></h2>
 </div>
 <link rel="stylesheet" type="text/css" href="popup.css"></link>
</body>
</html>

asked Dec 9, 2016 at 23:59
\$\endgroup\$
1

1 Answer 1

3
\$\begingroup\$

What doesn't work properly

There are two severe issues in the way you evaluate when to go from a given month to the next one.

When defining the set of days in a week

var systemTime = new Date();
var day = systemTime.getDate();
// ...
for (var i = 0; i < 7; i++) {
 if (day < 31) {
 week.push(day);
 day++;
 } else {
 week.push(day);
 day = 1;
 }
}

In the above code you work as if all months had 31 days!
Instead you should take account of those having only 30 days, and of February with its 28 or 29 days depending on bisextile years.

Obviously doing so becomes a much more complex task, so I'd suggest using a totally different way, based on the Date object internal capabilities:

var systemTime = new Date();
// ...
for (var i = 0; i < 7; i++) {
 var day = systemTime.getDate();
 week.push(day);
 systemTime.setDate(day + 1);
}

This way, not only we ensure to get correct days in any case but the code is even more reduced than before.

When looking for an involved month in the content of an event

The same kind of error appears in the nextSeven() function, when you test monthIndex < 11 && begDay >= 26: in fact, testing begDay >= 26 is appropriate only for months having 31 days... you know the next.

But there is also another issue here:

  • for each event you select it if it's matching thisDay
  • then you look for the given monthIndex or monthIndex + 1, independently from the current day
  • so you may accept wrong days, having the required number but belonging to another month!

Again I'll suggest to work based on full dates rather than separate days and months, but it'll be more clear to explain it later (see "The week case" below).

How to simplify and reduce code

Your code can be improved in several ways.

Formatting day

Once defined the set of days, here is how you finally format them:

for (var i = 0; i < week.length; i++) {
 if (week[i] === 1 || week[i] === 31) {
 week[i] = ' ' + week[i] + 'st,';
 } else if (week[i] === 2 || week[i] === 22) {
 week[i] = ' ' + week[i] + 'nd,';
 } else if (week[i] === 3 || week[i] === 23) {
 week[i] = ' ' + week[i] + 'rd,';
 } else {
 week[i] = ' ' + week[i] + 'th,';
 }
}

First I'd suggest using map() instead of a for() loop.
A second improvement is to factorize what's common to all cases, starting by defining the proper suffix, then building the result at once. And finally define suffix in a more compact way, for example using a predefined object registering suffixes.

This way, the whole code sequence above can be replaced by this:

const suffixes = {'1': 'st', '2': 'nd', '3': 'rd'};
var systemTime = new Date();
// ...
week = week.map(function (day) {
 return ' ' + day + (suffixes[day % 10] || 'th') + ',';
});

Defining days at once

In fact, we now see that the two partial tasks examined above can be joined, so the whole getDays() function becomes much more simple:

const suffixes = {'1': 'st', '2': 'nd', '3': 'rd'};
var systemTime = new Date();
// ...
function getDays() {
 for (var i = 0; i < 7; i++) {
 day = systemTime.getDate();
 week.push(' ' + day + (suffixes[day % 10] || 'th') + ',');
 systemTime.setDate(day + 1);
 }
}

Reorganization

It must be observed that this getDays() function only serves to populate weeks, which itself is used only in the nextSeven() context.

From that:

  1. The getDays(); call should'nt appear in the general context of success but in its own nextSeven()context.
  2. In this context, we see that the whole job is wrapped in a for() loop iterating week, to work on thisDay, which is merely week[i].
  3. So it'll be more efficient to abandon the getDays() function and integrate its job to nextSeven().

Current organization schema:

getDays();
// ...
function getDays() {
 for (var i = 0; i < 7; i++) {
 var day = systemTime.getDate();
 week.push(' ' + day + (suffixes[day % 10] || 'th') + ',');
 systemTime.setDate(day + 1);
 }
}
// ...
function nextSeven() {
 for (i = 0; i < week.length; i++) {
 thisDay = week[i];
 // ...
 }
}

Suggested organization schema:

function nextSeven() {
 for (var i = 0; i < 7; i++) {
 var day = systemTime.getDate();
 thisDay = ' ' + day + (suffixes[day % 10] || 'th') + ',';
 // ...
 systemTime.setDate(day + 1);
 }
}

(note: this way, the week variable doesn't exist any more)

Compacting some sequences

There are cases where using several statements to build a desired result can be simplified to a unique statement, so avoiding temporary variables not used elsewhere, though becoming more readable at the same time.
At the opposite, yet for readability, it may be better to extract constants from the sequence.

Here is an example:

var parsedData = $.parseHTML(data);
var parseResults = $(parsedData).find('p').text();
var events = parseResults.split(/Sun\. |Mon\. |Tues\. |Wed\. |Thurs\. |Fri\. |Sat\. /g);
events.splice(0, 1);

It can be replaced by:

const regexDays = /Sun\. |Mon\. |Tues\. |Wed\. |Thurs\. |Fri\. |Sat\. /g;
// ...
var events = $($.parseHTML(data)).find('p').text().split(regexDays).splice(0, 1);

Factorization

The most obviously visible case where factorization can take place is the one of the showMonth() and showNextMonth() functions.
What's done in their body differ only by one using monthIndex and the other using monthIndex + 1, so they can be replaced by a unique function accepting monthIndex as an argument:

function showMonth(monthIndex) {
 $.each(events, function(i, item) {
 var eventsstr = JSON.stringify(item);
 if (eventsstr.match(months[monthIndex] || months[0])) {
 var concert = JSON.parse(eventsstr);
 $('#concerts').append('<p>' + concert + '</p>');
 }
 });
}

And where they were called:

  • showMonth(); becomes showMonth(monthIndex);
  • showNextMonth(); becomes showMonth(monthIndex + 1);

The week case

Now it's time to bo back to the issue mentioned above, while at the same time simplifying the nextSeven() process.

The revised showMonth() function currently takes care of the given month: we can ask it to also take care of the given day, so it will select only those events which satisfy both, like this:

function showMonth(monthIndex, day) {
 $.each(events, function(i, item) {
 var eventsstr = JSON.stringify(item);
 if (
 eventsstr.match(months[monthIndex] || months[0])
 &&
 eventsstr.match(day)
 ) {
 var concert = JSON.parse(eventsstr);
 $('#concerts').append('<p>' + concert + '</p>');
 }
 });
}

Now we can very simply call it from nextSeven():

function nextSeven() {
 for (var i = 0; i < 7; i++) {
 var day = systemTime.getDate();
 var thisDay = ' ' + day + (suffixes[day % 10] || 'th') + ',';
 var thisMonth = systemTime.getMonth();
 showMonth(thisMonth);
 systemTime.setDate(day + 1);
 }
}

But so we observe that showMonth() doesn't work properly for the two other cases (month, nextMonth), so we must finally add what's needed for that (i.e. don't care of day if undefined):

function showMonth(monthIndex, day) {
 $.each(events, function(i, item) {
 var eventsstr = JSON.stringify(item);
 if (
 eventsstr.match(months[monthIndex] || months[0])
 &&
 (!day || eventsstr.match(day))
 ) {
 var concert = JSON.parse(eventsstr);
 $('#concerts').append('<p>' + concert + '</p>');
 }
 });
}

NOTE: since I don't know the exact structure and format of the events content, I don't go further on this, but it's likely possible to be more directly precise and efficient, depending on how month and day are presented.

answered Dec 17, 2016 at 20:06
\$\endgroup\$
0

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.