3
\$\begingroup\$

The first part of my code is a datepicker for a "From" and "To", showing the date in MMDDYYYY format in a textbox and having the value in the From field 7 days prior to the current date.

Second portion are a lot of form functionality such as show and hide sections and making certain fields required or not and how the page submits to the next page.

Then the third section is how we show values selected from a select and show them in a textbox when certain buttons are chosen.

I am looking to see if its at all possible to get away from the repetition in my code - embracing "DRY" (Do not repeat yourself).

For example, I use datepicker a lot ... basically I am new to coding and looking for expert advice of how I should/can shorten my code or simplify my code to be more efficient and run smoother.

JSFiddle

function resetDatePickers(){
      var today   = new Date();
      var weekAgo = new Date();
      var $from   = $("#StartDate");
      var $to     = $("#EndDate");

      weekAgo.setDate(today.getDate() - 7);

      $from.datepicker({  
        changeMonth: true,
        maxDate: today,
        onSelect: function(dateText) {
          $to.datepicker("option", "minDate", dateText);
        }
      }).datepicker('setDate', weekAgo);

      $to.datepicker({
        changeMonth: true,
        maxDate: today,
        minDate: weekAgo,
        onSelect: function(dateText) {
          $from.datepicker("option", "maxDate", dateText);
        }
      }).datepicker('setDate', today);
    }


    $(document).on('change', '#reporttype', function() {
        var value = $(this).val();

        var location             = $("#location");
        var loc                  = $("#loc");
        var employeelist         = $("#employeelist");
        var chosendates          = $("#chosendates");
        var formattype           = $("#formattype");
        var submitbtn            = $("#submitbtn");
        var locationbtns         = $("#locationbtns");
        var locationtextarea     = $("#locationtextarea");
        var EmployeeName         = $("#EmployeeName");
        var employeelistbtns     = $("#employeelistbtns");
        var employeelisttextarea = $("#employeelisttextarea");
        var generatereportform   = $("form[name=generatereport]");

        if (value === "checklistreports") {
            generatereportform[0].reset();
            location.show();
            locationbtns.show();
            locationtextarea.show();
            loc.prop('required',true);
            employeelistbtns.show();
            employeelisttextarea.show();
            employeelist.show();
            chosendates.show();
            formattype.show();
            submitbtn.show();
            generatereportform.attr("action", "index.cfm?content=reportsassociate");
            EmployeeName.prop('required',true);
        } else if (value === "locationreports") {
            generatereportform[0].reset();
            location.show();
            locationbtns.show();
            locationtextarea.show();
            loc.prop('required',true);
            employeelistbtns.hide();
            employeelisttextarea.hide();
            employeelist.hide();
            chosendates.show();
            formattype.show();
            submitbtn.show();
            generatereportform.attr("action", "index.cfm?content=reportslocation");
            EmployeeName.prop('required',false);
        } else {
            generatereportform[0].reset();
            location.hide();
            locationbtns.hide();
            locationtextarea.hide();
            loc.prop('required',false);
            employeelistbtns.hide();
            employeelisttextarea.hide();
            employeelist.hide();
            chosendates.hide();
            formattype.hide();
            submitbtn.hide();
            generatereportform.attr("action", "#");
            EmployeeName.prop('required',false);
        }

    resetDatePickers()
        });

 // JS for Showing Chosen Locations in textarea

    // Listens to the changes in #loc. Prepares the selected elements, and sets the result in
    // #selected.
    $("#loc").change(function() {
      var selected = [];

      $(this).find("option:selected").each(function() {
        selected.push($(this).text());
      });

      $("#selected").val(selected.join("\n"));
    });

    // Selects all the options, and manually triggers the change() of #loc
    $("#add").click(function() {
      var loc = $("#loc");
      loc.find("option").prop("selected", true);
      loc.change();
    });

    // Deselects all the options, and manually triggers the change() of #loc
    $("#rem").click(function() {
      var loc = $("#loc");
      loc.find("option").prop("selected", false);
      loc.change();
    });
    // End JS for Showing Chosen Locations in textarea

    // JS for Showing Chosen Associates in textarea

    // Listens to the changes in #EmployeeName. Prepares the selected elements, and sets the 
    // result in #selected1.
    $("#EmployeeName").change(function() {
      var selected = [];

      $(this).find("option:selected").each(function() {
        selected.push($(this).text());
      });

      $("#selected1").val(selected.join("\n"));
    });

    // Selects all the options, and manually triggers the change() of #EmployeeName
    $("#add1").click(function() {
      var emps = $("#EmployeeName");
      emps.find("option").prop("selected", true);
      emps.change();
    });

    // Deselects all the options, and manually triggers the change() of #EmployeeName
    $("#rem1").click(function() {
      var emps = $("#EmployeeName");
      emps.find("option").prop("selected", false);
      emps.change();
    });
    // End JS for Showing Chosen Associates in textarea
\$\endgroup\$
  • \$\begingroup\$ What's the point of the <textarea id="selected">? It looks confusingly similar to a dual multiselect — but isn't. \$\endgroup\$ – 200_success Sep 22 '17 at 21:56
4
\$\begingroup\$

Before getting into the code, I would suggest reading this article Stop writing slow Javascript. It has some good techniques for reducing time needed for DOM/page interaction. And while it is over 9 years old, JavaScript Event Delegation is Easier than You Think explains the benefits of event delegation as far as memory goes.

resetDatePickers

The configuration for the two date pickers is very similar. The date selected handler and the min date on the to date are the only real difference. A date selected handler can be used for both fields, depending on the id attribute of the selected date picker.

function dateSelectHandler(dateText) {
  if (this.id == 'StartDate') {
    $to.datepicker("option", "minDate", dateText);
  } else {
    $from.datepicker("option", "maxDate", dateText);
  }
}

let datePickerConfig = {
    changeMonth: true,
    maxDate: today,
    onSelect: dateSelectHandler
  };

Then that config can be used for both, and add the min date to the $to picker using Object.assign():

$from.datepicker(datePickerConfig).datepicker('setDate', weekAgo);
$to.datepicker(Object.assign(datePickerConfig, {
    minDate: weekAgo
})).datepicker('setDate', today);

Though Object.assign() isn't (currently) supported by any version of IE, so one could either add a polyfill or just modify the original date picker configuration slightly:

let toDatePickerConfig = datePickerConfig;
toDatePickerConfig.minDate = weekAgo;
$to.datepicker(toDatePickerConfig).datepicker('setDate', today);

See this branched fiddle for IE 11.

Hiding/Showing fields

The form fields can be grouped into two groups selected by jQuery when the DOM is ready (i.e. by passing a function to $()- like the deprecated .ready()):

$(function() { //DOM-loaded - like deprecated $(document).ready()
    //these don't change
    const reportFields = $('#location, #locationbtns, #locationtextarea, #chosendates, #submitbtn, #formattype');
    const employeeFields = $('#employeelist, #employeelistbtns, #employeelisttextarea');

Then when the report type changes, those fields can be toggeled like below:

if (value === "checklistreports") {
    reportFields.show();
    employeeFields.show();
} else if (value === "locationreports") {
    generatereportform[0].reset();
    employeeFields.show();
}

And the other DOM queries can be added to that DOM ready function:

const loc = $("#loc");
const EmployeeName = $("#EmployeeName");
const selectedElement = $('#selected');
const selected1Element = $('#selected1');
const reportType = $('#reporttype');
const generatereportform = $("form[name=generatereport]");

And then those can be used in the click/change handlers instead of querying the DOM each time- that is expensive. For example:

Instead of:

$("#add").click(function() {
    var loc = $("#loc");
    loc.find("option").prop("selected", true);

we can utilize that loc defined earlier, reducing the need for var loc

Event delegation

Then using event delegation, we can add two event handlers to the page and based on the id of the event target call various functions:

$(function() { //DOM-loaded - like deprecated $(document).ready()
    $(document).click(function(clickEvent) {
        switch (clickEvent.target.id) {
            //case statements
        }
    }
    $(document).change(function(changeEvent) {
       switch (changeEvent.target.id) {
           //case statements
       }
}

See forked fiddle here.

Note:

const and let are not supported by older browsers - e.g. IE 10 or earlier. Refer to the Browser Compatibility sections for let and const.

See this edited version of the forked fiddle. Also you mentioned it not working in IE 11 - I tried the original version and noticed errors related to jQuery. I tried porting it to jsbin. I noticed there were still errors with jQuery and updated the jQuery version to 3.1.1. That seems to help.

\$\endgroup\$
  • \$\begingroup\$ Yep, these are some other good examples of fighting coding repetition. \$\endgroup\$ – Igor Soloydenko Sep 22 '17 at 21:13
  • \$\begingroup\$ I am getting an error with the code from the fiddle. Saying there is a syntax error on this line let weekAgo = new Date(); \$\endgroup\$ – David Brierton Sep 25 '17 at 14:41
  • \$\begingroup\$ This works in chrome but not IE11. Is there a reason the browser would not allow the fiddle to work properly? and that the dates are not showing up right for me? \$\endgroup\$ – David Brierton Sep 25 '17 at 15:12
  • \$\begingroup\$ @SamOnela The "to" date is not showing up for me. It is in you fiddle. But in my app the from shows up with the date but not the "to" any idea what would cause that? \$\endgroup\$ – David Brierton Sep 25 '17 at 15:24
  • \$\begingroup\$ I tried updating the jQuery version from 1.10 to 3.1.1 and I believe that helps...Oh and for the to - that isn't showing up because IE has no support for Object.assign() \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Sep 25 '17 at 15:26
3
\$\begingroup\$

Fight the repetitive code. Your change/reporttype handler can be roughly 2.5 times shorter.

$(document).on('change', '#reporttype', function() {
  const value = $(this).val();

  const generatereportform = $("form[name=generatereport]");
  generatereportform[0].reset();

  const isChecklistOrLocation = value === "checklistreports" || value === "locationreports";
  const isChecklist = value === "checklistreports";
  const isLocation = value === "locationreports";

  const conditionallyDisplay = (targetElement, condition) =>
      condition ? targetElement.show() : targetElement.hide();

  conditionallyDisplay($("#location"), isChecklistOrLocation);
  conditionallyDisplay($("#locationbtns"), isChecklistOrLocation);
  conditionallyDisplay($("#locationtextarea"), isChecklistOrLocation);
  $("#loc").prop('required', isChecklistOrLocation);
  conditionallyDisplay($("#chosendates"), isChecklistOrLocation);
  conditionallyDisplay($("#formattype"), isChecklistOrLocation);
  conditionallyDisplay($("#submitbtn"), isChecklistOrLocation);
  conditionallyDisplay($("#employeelistbtns"), isChecklist);
  conditionallyDisplay($("#employeelisttextarea"), isChecklist);
  conditionallyDisplay($("#employeelist"), isChecklist);
  $("#EmployeeName").prop('required',isChecklist);

  if (isChecklist)
    generatereportform.attr("action", "index.cfm?content=reportsassociate");
  else if (isLocation)
    generatereportform.attr("action", "index.cfm?content=reportslocation");
  else
    generatereportform.attr("action", "#");

  resetDatePickers();
});

Also, some of the elements that you access via jQuery, are static. There's no point in searching for them many times like $("loc"). These instances can be hold in constants, so that there is a bit less code and a bit less calculation at run time.

\$\endgroup\$

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.