7
\$\begingroup\$

I made a date selector in HTML, which accounts for leap years and the number of days in each month. I am relatively new to javascript / HTML so please critique it as much as possible.

<!DOCTYPE html>
<html>
    <head>
        <title>Date Selector</title>
    </head>

    <body>
        <script>
            function clear(select) {
                for(var i = select.options.length - 1; i >= 0; i--) {
                    select.remove(i);
                }
            }

            function createMonths() {
                var months = ["Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"];
                var select = document.getElementById("month");
                for(var i = 0; i < 12; i++) {
                    var option = document.createElement("option");
                    option.text = months[i];
                    select.add(option);
                }
            }

            function createDays(days) {
                var select = document.getElementById("day");

                var index = select.options.selectedIndex;
                if(index < 0) index = 0;
                if(index >= days) index = days - 1;

                clear(select);
                for(var day = 1; day <= days; day++) {
                    var option = document.createElement("option");
                    option.text = day;
                    select.add(option);
                }

                select.options.selectedIndex = index;
            }

            function createYears(start, end) {
                var select = document.getElementById("year");
                for(var year = end; year >= start; year--) {
                    var option = document.createElement("option");
                    option.text = year;
                    select.add(option);
                }
            }

            function leapYear(year) {
                if(year % 400 == 0) return true;
                if(year % 100 == 0) return false;
                return year % 4 == 0;
            }

            function update() {
                var month = document.getElementById("month").options.selectedIndex + 1;

                if(month == 1 || month == 3 || month == 5 || month == 7 || month == 8 || month == 10 || month == 12) {
                    createDays(31);
                }
                else if(month == 4 || month == 6 || month == 9 || month == 11) {
                    createDays(30);
                }
                else {
                    var options = document.getElementById("year").options;
                    var year = options[options.selectedIndex].text;

                    if(leapYear(year)) {
                        createDays(29);
                    }
                    else {
                        createDays(28);
                    }
                }
            }

            window.onload = function() {
                var today = new Date();
                var year = today.getFullYear();

                createMonths();
                createDays(31);
                createYears(year - 100, year);
            };
        </script>

        <select id="month" onchange="update()"></select>
        <select id="day"></select>
        <select id="year" onchange="update()"></select>
    </body>
</html>

Improve this question
\$\endgroup\$
3
  • \$\begingroup\$ I'm not gonna wrote an answer from my phone while I'm poopin but you should put these functions together on a constructor or class and separate the DOM from the logic so you can reuse this. \$\endgroup\$
    – I wrestled a bear once.
    Jul 28, 2017 at 19:27
  • 1
    \$\begingroup\$ @bstrauch24 This is incorrectly reporting February as having 29 days in 2001-3, 2005-7, 2009-11, 2013-15, and 2017 (that I've checked), and is also incorrectly reporting February as having 28 days in 2004, 2008, 2012 and 2016. Perhaps you meant return year % 4 === 0; in the leapYear method. \$\endgroup\$
    – Der Kommissar
    Jul 28, 2017 at 19:28
  • \$\begingroup\$ @Iwrestledabearonce. Can you elaborate on how you would separate this into a javascript class? I like the idea of reusability, but how would you start? \$\endgroup\$
    – Brian
    Jul 28, 2017 at 19:45

1 Answer 1

Reset to default
5
\$\begingroup\$

I didn't pay much attention to your logic, but focusing on this question:

Can you elaborate on how you would separate this into a javascript class? I like the idea of reusability, but how would you start?

I would start by abstracting the DOM references in the code. Maybe just pass it a single div or span and then generate the select boxes dynamically for maximum reusability.

Then I would combine the methods into a class or constructor of some sort because if you have a function in your code called clear, for example, later on you might have a hard time figuring out what that does. datepicker.clear() is much more descriptive than just clear()

Here's an example of how you might accomplish this:

function DatePicker(element, days, startyear, endyear) {
  // Create the markup
  this.monthElement = document.createElement("select");
  this.dayElement = document.createElement("select");
  this.yearElement = document.createElement("select");
  this.monthElement.onChange = this.update;
  this.yearElement.onChange = this.update;
  element.appendChild(this.monthElement);
  element.appendChild(this.dayElement);
  element.appendChild(this.yearElement);

  this.createMonths();
  this.createDays(days);
  this.createYears(startyear, endyear);
}

DatePicker.prototype.clear = function(select) {
  for (var i = select.options.length - 1; i >= 0; i--) {
    select.remove(i);
  }
};

DatePicker.prototype.createMonths = function() {
  var months = ["Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"];
  for (var i = 0; i < 12; i++) {
    var option = document.createElement("option");
    option.text = months[i];
    this.monthElement.add(option);
  }
};

DatePicker.prototype.createDays = function(days) {
  var index = this.dayElement.options.selectedIndex;
  if (index < 0) index = 0;
  if (index >= days) index = days - 1;

  this.clear(this.dayElement);
  for (var day = 1; day <= days; day++) {
    var option = document.createElement("option");
    option.text = day;
    this.dayElement.add(option);
  }

  this.dayElement.options.selectedIndex = index;
};

DatePicker.prototype.createYears = function(start, end) {
  for (var year = end; year >= start; year--) {
    var option = document.createElement("option");
    option.text = year;
    this.yearElement.add(option);
  }
};

DatePicker.prototype.leapYear = function(year) {
  if (year % 400 == 0) return true;
  if (year % 100 == 0) return false;
  return year % 4 == 0;
};

DatePicker.prototype.update = function() {
  var month = this.monthElement.options.selectedIndex + 1;

  if (month == 1 || month == 3 || month == 5 || month == 7 || month == 8 || month == 10 || month == 12) {
    this.createDays(31);
  } else if (month == 4 || month == 6 || month == 9 || month == 11) {
    this.createDays(30);
  } else {
    var options = this.yearElement.options;
    var year = options[options.selectedIndex].text;

    if (this.leapYear(year)) {
      this.createDays(29);
    } else {
      this.createDays(28);
    }
  }
};

Then you can call it like this

var today = new Date();
var year = today.getFullYear();

// Send it a reference to a single element and create the markup in the constructor, 
// That way you don't have to rely on the user creating the correct markup
var dp = document.getElementById('datePicker');
var dPicker = new DatePicker(dp, 31, year - 100, year);

Here's a working demo.

Improve this answer
\$\endgroup\$

Your Answer

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

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