0
\$\begingroup\$

I have this logic that I wanted to see if there is any better way to build this select control. I am using the includes to look for data in the array and then I have a if statement to build the selected flag.

 $.each(guidelineMeetingDates, function (index, value) {
    var b = selectedMeetingDates.includes(value.MeetingDateId);
        if (b === true) {
         meetingdatesAppend += (`<option value="${value.MeetingDateId}" selected>${value.StartDate}</option>`);
    } else {
       meetingdatesAppend += (`<option value="${value.MeetingDateId}">${value.StartDate}</option>`);
    }
});

Sample https://jsfiddle.net/tjmcdevitt/L6qog1de/17/

Update I had to add a null check

 var b = (selectedMeetingDates === null ? false : selectedMeetingDates.includes(value.MeetingDateId));
\$\endgroup\$
1
  • \$\begingroup\$ Why did you add 'null' check? Your code doesn't guarantee that selectedMeetingDates exist? In what cases it can be null. \$\endgroup\$
    – Erasus
    May 26, 2022 at 1:51

1 Answer 1

2
\$\begingroup\$

There are a couple of ways that I know how you could improve the code, although the benefit ultimately is that the code is cleaner, for the size of your code it doesn't really matter that much, but it's always a good idea to don't repeat yourself and separate responsibilities of the code.

So here are my suggestions how I would do it and why.

I would replace this code:

if (b === true) {
         meetingdatesAppend += (`<option value="${value.MeetingDateId}" selected>${value.StartDate}</option>`);
    } else {
       meetingdatesAppend += (`<option value="${value.MeetingDateId}">${value.StartDate}</option>`);
    }

To this:

meetingdatesAppend += (`<option value="${value.MeetingDateId}" ${b ? "selected" : ""}>${value.StartDate}</option>`);

The idea here is to have one line of code with only one string, because you check if 'b' is true and if so the only thing that changes is 'selected' atrribute, so it's just simplier to use one line of code and only check inside a string if 'b' is true.

Since it's guaranteed that in your code b always returns a boolean so you don't really need to do strict checking with '===' operator.

Also if you want to separate that line of code, you can create a function like this:

meetingdatesAppend += createOption(value.MeetingDateId, value.StartDate, 

b);

And define a function for creating an option - 'createOption':

function createOption(value, text, selected = false) {
    return (`<option value="${value}" ${selected ? "selected" : ""}>${text}</option>`);
}

The benefit here is that you abstract your string building for select option in a function, and just by passing certain arguments you can create a string. And you can also use that function anywhere in case you need it, so you don't need copy and paste that '<option...>' string and such, you just call a function, it's way more efficient.

That's just how I would do it, as there are about only a few lines of code in your given example so there's nothing more to optimize unless we know your 'business' requirements.

\$\endgroup\$

Your Answer

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

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