4
\$\begingroup\$

I've made a calendar for the year of 2015 using Firebase for real-time updates when something changes in it. I'm quite happy with it but I'm interested in getting some feedback on my Javascripting regarding best-practices, what to avoid and anything in general that I can improve in this code. Even the tiniest of things is important so throw it at me. Also let me know what I did well so that I know it for future projects!

Unfortunately I can't include a JSfiddle or the actual site since I don't want random people to spam my database. Hope you understand.

//The database reference
var myDataRef = new Firebase('https://event-calender.firebaseio.com/events'); 

myDataRef.on('child_added', function(snapshot) { 
    var info = snapshot.val();
    calendar.showAddedEvent(info.eventType, info.eventTime, info.eventLocation, info.eventAddress, info.eventDay); 
});

var theParent; 
var container = document.getElementById('container'); 

//An object containing any raw data
var dataModel = {

    months: ['January', 'February', 'Marsch', 'April', 'May', 'June',
             'July', 'August', 'September', 'October', 'November', 'December'
            ],
    daysInMonth: [31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31], 
    years: ['2015'],
    buttonTexts: ['Add', 'Delete'],

    //All the red days of the year of 2015 (in Sweden)
    redDays: ['1Jan', '4Jan', '6Jan', '11Jan', '18Jan', '25Jan',
              '1Feb', '8Feb', '15Feb', '22Feb',
              '1Mar', '8Mar', '15Mar', '22Mar', '29Mar',
              '3Apr', '5Apr', '6Apr', '12Apr', '19Apr', '26Apr',
              '1May', '3May', '10May', '14May', '17May', '24May', '31May',
              '6Jun', '7Jun', '14Jun', '20Jun', '21Jun', '28Jun',
              '5Jul', '12Jul', '19Jul', '26Jul',
              '2Aug', '9Aug', '16Aug', '23Aug', '30Aug', 
              '6Sep', '13Sep', '20Sep', '27Sep',
              '4Oct', '11Oct', '18Oct', '25Oct', '31Oct',
              '1Nov', '8Nov', '15Nov', '22Nov', '29Nov',
              '6Dec', '13Dec', '20Dec', '25Dec', '26Dec', '27Dec'
             ]
}

var calendar = {

    //Function that runs after the page is done loading
    init: function() {

        var addEventButton = document.getElementById('button'); 
            addEventButton.addEventListener('click', (function (e) { 

        var typeOfEvent = document.getElementById('event').value; 
        var timeOfEvent = document.getElementById('time').value;
        var locationOfEvent = document.getElementById('location').value;   
        var addressOfEvent = document.getElementById('address').value; 
        var dayOfEvent = theParent.id;

        var inputArray = ['type of event', 'location of the event', 'address of the event'];

        for (i = 0; i < inputArray.length; i++) {

            if (typeOfEvent.length > 21) {
                alert('The information you have entered for ' + inputArray[0]
                + ' is too long.');
                return;
            }
            else if (locationOfEvent.length > 21) {
                alert('The information you have entered for ' + inputArray[1]
                + ' is too long.');
                return;
            }
            else if (addressOfEvent.length > 21) {
                alert('The information you have entered for ' + inputArray[2]
                + ' is too long.');
                return;
            }
        }

        //Create a new JSON for each child added
        var newRef = myDataRef.child(dayOfEvent);
            newRef.set({eventType: typeOfEvent, eventTime: timeOfEvent, 
            eventLocation: locationOfEvent, eventAddress: addressOfEvent, eventDay: dayOfEvent});
        }))

        var deleteButton = document.getElementById('deleteButton');
            deleteButton.addEventListener('click', calendar.confirmDelete);

        var showButton = document.getElementById('showButton');
            showButton.addEventListener('click', calendar.scrollToMonth);

        var backToTopButton = document.getElementById('btt');
            backToTopButton.addEventListener('click', calendar.scrollToTop);

        var selectMonth = document.getElementById('month');
        var selectYear = document.getElementById('year');

        for (i = 0; i < dataModel.months.length; i++) {

            var option = document.createElement('option');
                option.innerHTML = dataModel.months[i];
                option.value = option.innerHTML;
                selectMonth.appendChild(option);
        }

        for (j = 0; j < dataModel.years.length; j++) {

            var option = document.createElement('option');
                option.innerHTML = dataModel.years[j];
                option.value = option.innerHTML;
                selectYear.appendChild(option);
        }

        calendar.createCalendar();
        calendar.markRedDays();
    },
    createCalendar: function() {

        var num = 1; 

        for (i = 0; i < dataModel.months.length; i++) {

            var h1 = document.createElement('h1');
                h1.setAttribute('id', dataModel.months[i]); 
                h1.innerHTML = dataModel.months[i] + ' ' + dataModel.years[0];
                container.appendChild(h1); 

            var table = document.createElement('table');
                container.appendChild(table);

            for (j = 0; j < 4; j++) {
                var tr = document.createElement('tr'); 
                    table.appendChild(tr);

                for (k = 0; k < 8; k++) {
                    var td = document.createElement('td');
                        td.setAttribute('id', num + dataModel.months[i].charAt(0) 
                        + dataModel.months[i].charAt(1)
                        + dataModel.months[i].charAt(2));

                    var span = document.createElement('span'); 
                        span.innerHTML = num;
                        td.appendChild(span);
                        num++;

                    if (dataModel.daysInMonth[i] === 31 && j === 3 && k > 6) {
                        td.style.display = 'none';
                    }
                    else if (dataModel.daysInMonth[i] === 30 && j === 3 && k > 5) {
                        td.style.display = 'none';
                    }
                    else if (dataModel.daysInMonth[i] === 29 && j === 3 && k > 4) { 
                        td.style.display = 'none';
                    }
                    else if (dataModel.daysInMonth[i] === 28 && j === 3 && k > 3) { 
                        td.style.display = 'none';
                    }

                    tr.appendChild(td); 

                    for (m = 0; m < 2; m++) { 

                        var button = document.createElement('button');
                            button.innerHTML = dataModel.buttonTexts[m];

                        if (button.innerHTML === 'Add') {
                            button.addEventListener('click', calendar.showEventAddWindow); 
                        }
                        else {
                            button.addEventListener('click', calendar.deleteEvent); 
                        }

                        td.appendChild(button);
                    }
                }
            }
            num = 1; 
        }
    },
    showEventAddWindow: function() {

        var parent = this.parentNode;

        var popup = document.getElementById('pop-up');
        //Grab all the inputs
        var typeOfEvent = document.getElementById('event');
        var timeOfEvent = document.getElementById('time');
        var locationOfEvent = document.getElementById('location');   
        var addressOfEvent = document.getElementById('address'); 

        typeOfEvent.value = '';
        timeOfEvent.value = '';
        locationOfEvent.value = '';
        addressOfEvent.value = '';

        if (popup.style.visibility === 'visible') {
            popup.style.visibility = 'hidden';
        }
        else {
            popup.style.visibility = 'visible'; 
        }

        theParent = parent; 
    },
    showAddedEvent: function(eventType, eventTime, eventLocation, eventAddress, dayOfEvent) {

        var eventInfo = [eventType, eventTime, eventLocation, eventAddress];

        var thisElement = document.getElementById(dayOfEvent); 

        var popup = document.getElementById('pop-up');
            popup.style.visibility = 'hidden'; 

        for (i = 0; i < 4; i++) {

            var p = document.createElement('p');
                p.innerHTML = eventInfo[i];
                thisElement.appendChild(p); 
        }
    },
    removeEvents: function(eventType, eventTime, eventLocation, eventAddress, dayOfEvent) {

        var p_list = document.getElementsByTagName('p');

        for (i = p_list.length-1; i>=0; i--) { 
            p_list[i].parentNode.removeChild(p_list[i]); 
        }
    },
    deleteEvent: function() {

        var parent = this.parentNode; 
        var parentId = parent.id; 

        var p_list = parent.getElementsByTagName('p');

        for (i = p_list.length-1; i>=0; i--) { 
            p_list[i].parentNode.removeChild(p_list[i]);
        }

        //Create a new Firebase reference that matches the parent ID
        var removeData = new Firebase('https://event-calender.firebaseio.com/events/' + parentId);
        removeData.remove(); //Remove the data at this location
    },
    deleteAll: function() { 
       myDataRef.remove();
    },
    markRedDays: function() {

        for (i = 0; i < dataModel.redDays.length; i++) {
            var currentDate = document.getElementById(dataModel.redDays[i]);
            var firstSpan = currentDate.getElementsByTagName('span')[0];
                firstSpan.style.color = 'red';
        }
    },
    confirmDelete: function() {

        var conf = confirm('You are about to delete all events, this cannot be undone. Are you sure you want to continue?');

        switch (conf) {

            case true: calendar.deleteAll();
                       calendar.removeEvents();
                       break;

            case false: return; 
                        break;
        }
    },
    scrollToMonth: function() {

        var m = document.getElementById('month'); 
        var selectedMonth = m.options[m.selectedIndex].value;

        $('html, body').animate({
            scrollTop: $('#'+selectedMonth).offset().top
        }, 'slow');
   },
    scrollToTop: function() {

        $("html, body").animate({ scrollTop: 0 }, "slow");
    }

} //End of calender object

window.onload = calendar.init;
\$\endgroup\$
2
\$\begingroup\$

From a once over;

I would write

//When a new child is added to the database, send a snapshot containing the data...
myDataRef.on('child_added', function(snapshot) { 
    var info = snapshot.val(); //And check the snapshot's data
calender.showAddedEvent(info.eventType, info.eventTime, info.eventLocation, info.eventAddress, info.eventDay); //And call the function to display it to the HTML
});

as

//Add new events to the calendar widget
myDataRef.on('child_added', function(snapshot) { 
    var event= snapshot.val(); 
    calender.showAddedEvent(event); 
});

The most important part are the comments, placing them on the right makes it harder to read. Also, they were not enlightening me. Your indenting was also off. The above is much clearer (to me). Also, since you call showAddedEvent only once in the whole code, and you pass only properties of event, it is easier to read/code/understand

var theParent; //Stores the current parent we're using
var container = document.getElementById('container'); //The container of the application

could be

var parent, //The current parent
    container = document.getElementById('container'); 

Again the comments were not too helpful, anybody could figure out after reading the variable name and id that the container was a container ;) Also it is considered better form to comma-chain your variable declarations.

I love dataModel, storing the month's ends is smart low tech.

You are using alert, that is considered bad form. Consider using (non-)modal forms.

You are using variables j and k, I am sure you could come up with something better.

I thought this part was funny:

//Prompts the user for a confirmation of the deletion of ALL events
var conf = confirm('You are about to delete all events, this cannot be undone. Are you sure you want to continue?');

switch (conf) {

    //If the answer is 'OK' go ahead with the procedure
    case true: calender.deleteAll();
               calender.removeEvents();
               break;

    //If the answer is 'Cancel' or 'X' the cancel the procedure
    case false: return; 
                break;

    //If something else happened during the request then alert an error message
    default: alert('There was an error processing your request');
             break;
}

confirm returns a boolean, I am not sure when default would ever be called ;)

\$\endgroup\$
  • \$\begingroup\$ Thanks for the response, ye I know the indention was a bit off but that happens when u copy from ur own file to here, could not be bothered to fix it for like 400 lines of code :P I'm just used to using i, j and k when having nested loops, makes it easy instead of coming up with names as variables, doesn't really add anything to the readability if u ask me. But I will think about it. The alerts is bad I know, but as I felt a bit lazy I just went with it instead of showing it next to the inputs. Ye haha I know about the default, I'm just so used to having it when using switches! Will remove. \$\endgroup\$ – Chrillewoodz Oct 29 '14 at 16:59
  • \$\begingroup\$ About the comments, they're meant to be understood by novice people since this is a school project. Most of the other people doesn't have a lot of knowledge so I had to "dumb it down" a bit. I don't need the obvious comments for myself. :) \$\endgroup\$ – Chrillewoodz Oct 29 '14 at 17:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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