0
\$\begingroup\$

I created this working sample and the code is below. Any advice on how to make this code; cleaner, more effective, just overall better! The data is coming from an API and I need to parse, select the value in location 0 set the title and then rebuild the drop down control.

can it be done without the JSON.parse(data) or the $.each?

https://dotnetfiddle.net/lfJhGi

      $("#ddlReleaseYears").change(function () {
            $('#toptitle').text('change event');
            //coming from API
            var data = "[2021,2020,2019,2018,2017,2016]";
            var arr = JSON.parse(data);
            $('#toptitle').text(arr[0]);
            var HTML = ""
            $.each(arr, function (index, value) {
                 HTML += "<option value='" +value + "' ";
                 HTML += ">" +value + "</option>"
            });
            $("#ddlReleaseYears").html("");
            $("#ddlReleaseYears").append(HTML);
        })
\$\endgroup\$
5
  • \$\begingroup\$ you could fill data dynamically based on the current year... (or you need to change your code in 7 months time... ) \$\endgroup\$ – Luuk May 28 at 16:13
  • \$\begingroup\$ can I do it without the JSON.parse(data) or the $.each \$\endgroup\$ – Jefferson May 28 at 16:16
  • \$\begingroup\$ Why not a simple for loop ? \$\endgroup\$ – Luuk May 28 at 18:15
  • \$\begingroup\$ If the data you are receiving from the API is JSON, you need to parse it. Why would you not want to use JSON.parse? \$\endgroup\$ – m-alorda May 28 at 20:15
  • \$\begingroup\$ I was just thinking I would need to do something like this to get the data out arr[0]. \$\endgroup\$ – Jefferson May 28 at 22:12
1
\$\begingroup\$

The two first things that come to my mind:

  1. Using template strings
  2. Modifying the dropdown HTML once:
$(document).ready(function () {
    $("#ddlReleaseYears").change(function () {
        var data = "[2021,2020,2019,2018,2017,2016]";
        var arr = JSON.parse(data);
        $('#toptitle').text(arr[0]);
        var HTML = ""
        $.each(arr, function (index, value) {
            HTML += `<option value="${value}">${value}</option>`
        });
        // No need to set the html to "" and then append
        $("#ddlReleaseYears").html(HTML);
     })
});
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the help anything I can do with setting the toptitle? ‘$('#toptitle').text(arr[0]);’ \$\endgroup\$ – Jefferson May 28 at 21:32
  • \$\begingroup\$ I mean, that is readable: you get the element whose id is toptitle and you set its text to the first element of the array arr. Nothing to say about that \$\endgroup\$ – m-alorda May 29 at 7:25
2
\$\begingroup\$

Use the DOM API

Always try to avoid adding HTML to the page. Modern browsers come with all the APIs and interfaces to create and manipulate page content.

using he API's is much quicker than adding HTML, And as you will not need jQuery even faster still.

Element by id

If you have elements identified by their ids as follows...

<label for="dlReleaseYear" id="toptitle"></label>
<select id="dlReleaseYear"></select>

...you can reference the element directly in JavaScript. You must ensure that the elements ID is unique.

toptitle.textContent = "hello world";

Select HTMLSelectElement

The select element has an interface HTMLSelectElement to do all the common tasks.

  • To empty the select element

    dlReleaseYear.length = 0;
    
  • To add an option use HTMLSelectElement.add to add a HTMLOptionElement

    dlReleaseYear.add(Object.assign(document.createElement("option"), {value}))
    

Reusable

As DOM APIs are generally very verbose and too specific create utility functions to reduce the code noise. For example the function tag creates an element by tag name and assigns properties.

const tag = (tag, props = {}) => Object.assign(document.createElement(tag), props);

Thus adding an option becomes

dlReleaseYear.add(tag("option", {value: item, textContent: item}));

Adding options to a select element is a frequent task so rather than create a custom function, create a general function that adds options to a Select element so that it can be reused.

const tag = (tag, props = {}) => Object.assign(document.createElement(tag), props);
const selectOptions = (selEl, ...data) => (
    selEl.length = 0,
    data.forEach(value => { selEl.add(tag("option", {value, textContent: value})) },
    selEl
)

Events

To add event Listeners using the DOM API call the elements EventTarget.addEventListener function with the event name and the listener.

To add the change event

dlReleaseYear.addEventListener("change", listenerFunction);

Rewrite

Using all of the above you no long need to use jQuery with all its overheads and there is no need to build markup strings and then have the browser have to parse them.

const tag = (tag, props = {}) => Object.assign(document.createElement(tag), props);
const selectOptions = (selEl, ...data) => (
  selEl.length = 0,
  data.forEach(value => { selEl.add(tag("option", {value, textContent: value})) }),
  selEl
);

ddlReleaseYears.addEventListener("change", updateYearOptions);
function updateYearOptions() {
  const data = [2002,2003,2004,2005,2006];
  toptitle.textContent = data[0];
  selectOptions(ddlReleaseYears, ...data);
}
<label for="ddlReleaseYears" id="toptitle">Select option to test</label>
<select id="ddlReleaseYears">
    <option>Test</option>
    <option>Years</option>

</select>

\$\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.