2
\$\begingroup\$

This is implementation of a Datepicker by our outsource colleagues.

I want to get an objective criticism of code style and approaches used.

I see:

  • Global variables with popular names
  • Styles inside JS
  • Not allowed attributes like cellpadding

Please help me find other problems. I believe that this code is not ready for production, and it can lead to unpredictable behavior. Am I biased?

// calendar.js
var date = new Date();
var currdate = new Date();
var colorclear;
var input;

function insertNodeText(obj, text) {
    var txtNode = document.createTextNode(text);
    obj.appendChild(txtNode);
    return obj;
}

function insertButton(obj, text, flag) {
    var buttons = Array (2);
    for (var i = 0; i < buttons.length; i++) {
        var button = document.createElement('Button');
        button.setAttribute('type', 'button');
        var caption = (i == flag)? text + text : text;
        button = insertNodeText(button, caption);
        button.onclick = controlButton;
        obj.appendChild(button);
    }
}

function getCoords(elem) {
    var box = elem.getBoundingClientRect();
    var scrollTop = window.pageYOffset || document.documentElement.scrollTop || document.body.scrollTop;
    var scrollLeft = window.pageXOffset || document.documentElement.scrollLeft || document.body.scrollLeft;
    var clientTop = document.documentElement.clientTop || document.body.clientTop || 0;
    var clientLeft = document.documentElement.clientLeft || document.body.clientLeft || 0;
    var top  = box.top +  scrollTop - clientTop;
    var left = box.left + scrollLeft - clientLeft;
    return { top: Math.round(top) + elem.offsetHeight + 'px', left: Math.round(left) + 'px'}
}

function getCountDay(index) {
    var months = new Array(31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31);
    if (date.getFullYear() % 4 == 0)
        months[1]++;
    return months[index];
}

function getCountWeek(date) {
    date.setDate(1);
    return Math.ceil((getCountDay(date.getMonth()) + getNumberDay(date)) / 7);
}

function getNumberDay(date) {
    var days = new Array(6, 0, 1, 2, 3, 4, 5);
    return days[date.getDay()];
}

function getNumberFirstDay(date) {
    date.setDate(1);
    return getNumberDay(date);
}

function controlButton() {
    if (this.lastChild.nodeValue.length == 1) {
        var m = (this.lastChild.nodeValue == '<')? date.getTime() - (24 * 60 * 60 * 1000 * date.getDate()) : date.getTime() + (24 * 60 * 60 * 1000 * (getCountDay(date.getMonth()) - date.getDate() + 1));
            date.setTime(m);
    }
    /*else {
        var Y = (this.lastChild.nodeValue == '<<')? date.getFullYear() - 1 : date.getFullYear() + 1;
        date.setFullYear(Y);
    }*/
    generateCalendar();
}

function createTable() {
    var days = new Array('Пн', 'Вт', 'Ср', 'Чт', 'Пт','Сб','Вс');

    var body = document.querySelector('.days');
    if(body){
        var div = document.createElement('div');
        body.appendChild(div);
        div.setAttribute('id', 'calendar', 0);
        div.className = 'calendar-wrapper';

        var tbl = document.createElement("table");
        div.appendChild(tbl);

        tbl.setAttribute('align', 'center', 0);
        tbl.setAttribute('cellpadding', '0', 0);
        tbl.setAttribute('cellspacing', '0', 0);    

        var row = tbl.insertRow(-1);
        row.setAttribute('id', 'first-child', 0);   

        // first cell
        var cell = row.insertCell(-1);
        cell.setAttribute('colspan', '2', 0);
        cell.className = 'lalign';
        cell = insertButton(cell, '<', 0);

        // second cell
        var cell = row.insertCell(-1);
        cell.setAttribute('colspan', '3', 0);   

        // third cell
        cell = row.insertCell(-1);
        cell.setAttribute('colspan', '2', 0);   
        cell.className = 'ralign';
        cell = insertButton(cell, '>', 1);

        row = tbl.insertRow(-1);
        row.className = 'day';
        for (var i = 0; i < 7; i++) {
            cell = row.insertCell(-1);
            cell = insertNodeText(cell, days[i]);                       
        }
    }
}

function generateCalendar() {
    var wrapper = document.getElementById('calendar');
    var tbl = wrapper.getElementsByTagName('table')[0]; 

    var months = Array('Январь', 'Февраль', 'Март', 'Апрель', 'Май', 'Июнь', 'Июль', 'Август', 'Сентябрь', 'Октябрь', 'Ноябрь', 'Декабрь'); 

    tbl.rows[0].cells[1].innerHTML = months[date.getMonth()] + ' ' + date.getFullYear();

    while (tbl.rows.length > 2)
        tbl.deleteRow(tbl.rows.length - 1);

    var flag = false;
    var countday = 1;

    for (var i = 0; i < getCountWeek(date); i++) {
        var row = tbl.insertRow(-1);
        for (var j = 0; j < 7; j++) {
            var cell = row.insertCell(-1);
            if (j == getNumberFirstDay(date))
                flag = true;
            if (flag && countday <= getCountDay(date.getMonth())) {
                cell = insertNodeText(cell, countday);

                cell.onclick = function() {
                    date.setDate(this.lastChild.nodeValue);
                    var d = (date.getDate() < 10)? '0' + date.getDate() : date.getDate();                                   
                    var m = ((date.getMonth() + 1) < 10)? '0' + (date.getMonth() + 1) : (date.getMonth() + 1);
                    var period = input.getAttribute('data-time');
                    var value = input.value;
                    input.removeAttribute('placeholder');
                    if(period){
                        if(value!=''){
                            if(value.split('.').length>3 && value.split('.').length<=5){
                                var newInput = value.split('-');
                                var firstDate = newInput[0];
                                var secondDate = d + '.' + m + '.' + date.getFullYear();
                                var firstDateArray = firstDate.split('.');
                                var secondDateArray = secondDate.split('.');
                                if(firstDateArray[2]<=secondDateArray[2]){
                                    if(firstDateArray[1]<=secondDateArray[1]){
                                        if(firstDateArray[0]<=secondDateArray[0]){
                                            input.value = newInput[0] +'-'+ d + '.' + m + '.' + date.getFullYear();
                                        }
                                    }
                                }
                            } else{
                                input.value = input.value +'-'+ d + '.' + m + '.' + date.getFullYear();
                            }                                                   
                        } else{
                            input.value = d + '.' + m + '.' + date.getFullYear();
                        }
                    }
                    else{
                        input.value = d + '.' + m + '.' + date.getFullYear();
                    }
                    document.getElementById('calendar').style.display = 'none';                             
                };
                cell.onmouseover = function() {colorclear = this.className; this.className = 'Select';};
                cell.onmouseout = function() {this.className = colorclear;};
                if (j == 6 || j==5)
                    cell.className = 'Sunday';
                else
                    cell.className = 'DefaultDay';
                if (currdate.getFullYear() == date.getFullYear() && currdate.getMonth() == date.getMonth() && currdate.getDate() == countday)
                    cell.className = 'CurrDay';

                countday++;
            }
            else {
                cell.style.border = 'none';
                cell = insertNodeText(cell, ' ');
            }
        }
    }
}

function showcalendar(input_date) {
    input = input_date;
    var wrapper = document.getElementById('calendar');

    if (input.value != '') {
        // Split given date

        // Array of parameters (Day, Month, Year)
        // To Date constructor send in different order: Year, Month, Day 
        // ar[2] - Year
        // ar[1] - Month
        // ar[0] - Day
        // In JavaScript months starts from 0 to 11, therefore decrement month


        if(input.getAttribute('data-time')=='period'){
            var ar = input.value.split('.');
            var day = ar[2].split('-');
            if(day.length>1){
                date = new Date(ar[4], ar[3]-1, day[1]);
            } else{
                date = new Date (ar[2], ar[1] - 1, ar[0]);
            }
        } else{
            var ar = input.value.split('.');
            date = new Date (ar[2], ar[1] - 1, ar[0]);
        }
    }

    generateCalendar(); 

    //wrapper.style.left = getCoords(input).left ;
    //wrapper.style.top = getCoords(input).top;
    wrapper.style.display = (wrapper.style.display=="block")? "none" : "block";
}

if (window.addEventListener)
    window.addEventListener('load', createTable, false);
else if (window.attachEvent)
    window.attachEvent('onload', createTable);
\$\endgroup\$
  • 1
    \$\begingroup\$ Can you tell us what the code is meant to do and from there we can suggest if other code smells exist \$\endgroup\$ – Siobhan Oct 19 '16 at 17:59
  • \$\begingroup\$ @TolaniJaiye-Tikolo This code used to show datepicker under input field \$\endgroup\$ – 4ell Oct 19 '16 at 18:06
  • 3
    \$\begingroup\$ Feel free to edit your post to include any relevant information. \$\endgroup\$ – Mathieu Guindon Oct 19 '16 at 18:10
  • 1
    \$\begingroup\$ @Mat'sMug I must maintain this code in the future. For that reason I seeking for review of code. I need an independent view. \$\endgroup\$ – 4ell Oct 19 '16 at 19:33
  • 5
    \$\begingroup\$ To everyone closing the question: code being maintained by the OP is on-topic. Take a look at the help center. \$\endgroup\$ – Mast Oct 19 '16 at 19:50
3
\$\begingroup\$

There is not a whole lot to like about this code (other than it may work).

  • The display is much too heavily coupled to the business logic. You are hard-coding things like class names, element properties, CSS styles, localization strings, etc. into the code, which doesn't make a lot of sense to me and limits flexibility of use for this code.
  • There is no modularity to this code. What if you need multiple date pickers on a single page? Right now you can only attach this behavior to the first element with days specified as class. What if you have two date fields on a form?
  • It does really odd things like call generateCalendar() every time someone wants to select a date. Why does this need to be re-generated every time?
  • This code gives no flexibility to change event-based behavior. Event handlers are hard-coded into the methods as opposed to taking the approach of allowing event callbacks to be passed into the functions.
  • There are a lot of hard-coded date behaviors that really don't need to be here. If you know the start date which you want to make the basis of your calendar, you should easily be able to increment the date of the Date object to generate the calendar as needed. There is no reason for you to store how many days are in a month, what the days of the week are, etc.
  • The code is horrendously hard to read. Long lines of code, heavily nested conditionals, minimal meaningful comments, methods that do WAY too much, commented-out code, etc. This is really a mess.
  • There is no input validation.
  • This is all operating against global scope (at least from what is shown). There is no reason for this. I would like to see this logic potentially put into a more proper class type of structure that would allow for multiple instances of the date time picker which have common methods, but which could allow instantiations of multiple date pickers, each with their own date values and bindings to the view and event handlers and each operating within their own proper scope.
  • Really there are any number of datepicker implementations out there on the web. I don't know why one would want to re-invent the wheel, especially if you are going to end up with an inferior result.
\$\endgroup\$
  • \$\begingroup\$ "The code is horrendously hard to read. Long lines of code, heavily nested conditionals, minimal meaningful comments, methods that do WAY too much, commented-out code, etc. This is really a mess." I second this comment by Mike; pretty much sums it up. No dev would enjoy maintaining this code. Use a library instead because the author's work is not production ready. \$\endgroup\$ – SoEzPz Oct 20 '16 at 21:26

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.