2
\$\begingroup\$

I have an array of ranges as you can see in the snippet. when user select certain number from dropdown I want to remove that number from the range and return new ranges,

Everything is working as per my expectation but I think, I have over coded and used too many if else statements in my code. I was wondering if there is any better way to do it?

If yes please let me know, if Javascript has any methods to make this simple and reduce the amount of code.

var ranges = [
  {start: 1, end: 100},
  {start: 150, end: 300},
  {start: 400, end: 500},
  {start: 550, end: 650},
];


function onSelect(event) {
  let newRange = [];
  let HMID = parseInt(document.getElementById("mySelect").value);
  
  
  for (let range of ranges) {
      
      if ((HMID > range.start) && (HMID < range.end)) {

          newRange.push({start: range.start, end: (HMID - 1)});
          newRange.push({start: (HMID + 1), end: range.end});
      } else if (HMID === range.start) {

          newRange.push({start: (range.start + 1), end: range.end});
      } else if (HMID === range.end) {

          newRange.push({start: range.start, end: (range.end - 1)});
      } else {

          newRange.push(range);
      }
  }
  console.log(JSON.stringify(newRange));
  
}

function onSelectAdd() {

  let newRange = [];
  let HMID = parseInt(document.getElementById("mySelectAdd").value);
  
  
  for (let i = 0; i < (ranges.length); i++) {

  let start, end;

  if (HMID == (ranges[i].start -1)) {
      start = HMID;
      end = ranges[i].end;

      newRange.push({start: start, end: end});
  } else if (HMID == (ranges[i].end + 1)) {
      start = ranges[i].start;

      if (ranges.length === (i + 1)) {
          end = HMID;
      } else {

          if (HMID == (ranges[i+1].start - 1)) {
              end = ranges[i+1].end;
              i++;

          } else {
              end = ranges[i].end;
          }
      }

      newRange.push({start: start, end: end});

  } else {
      newRange.push({start: ranges[i].start, end: ranges[i].end});
  }

  }
  
  console.log(JSON.stringify(newRange));
}
<p>Remove item</p>
<select id="mySelect" onchange="onSelect()">
  <option value="1">1</option>
  <option value="150">150</option>
  <option value="450">450</option>
  <option value="650">650</option>
</select>

<p>Add removed item</p>
<select id="mySelectAdd" onchange="onSelectAdd()">
  <option value="1">1</option>
  <option value="150">150</option>
  <option value="450">450</option>
  <option value="650">650</option>
</select>

EDIT: I am looking only for logical and functional code review. Please don't comment on syntax and variable names. Thanks.

\$\endgroup\$
1
\$\begingroup\$

I'll go over some of the basics

let HMID = parseInt(document.getElementById("mySelect").value);

what should HMID be? Aside from being a number, I don't know what it is. And the only reason I know it's a number is because it's the value of parseInt. Looking at how it's used doesn't shed any light. You should name the variable something more descriptive.

Speaking of naming things more descriprive - mySelect is a really bad name, too. Part of why I don't know what HMID is, is because it's the value of something with the non-descriptive ID of mySelect. The myX naming convention is fine when prototyping but you really need to go with descriptive names upon finishing your code.

Echoing the same "descriptive names needed" sentiment for onSelect and onSelectAdd. What do they do? One is executed on select which...doesn't tell me much. The other...adds stuff on select? It's not clear. Somewhat better names would be

  • onSelect -> removeItemFromRange
  • onSelectAdd -> addRemovedToRange

I'm not great at naming things but I'd err on the side of verbose but descriptive rather than short but unclear. If you can have something shorter, feel free to use that. As it stands, the names aren't good.

You are better off adding the event listeners via element.addEventListener rather than in the HTML. That way, you keep the HTML clear of logic - it's all contained in your JavaScript. As an added benefit, you can attach more event listeners that do other stuff without changing these functions. Here is an example

let button = document.getElementById("buttonWithMultipleClickListenersExample"); //told you I'm not good at naming things...

button.addEventListener("click", sayFoo);
button.addEventListener("click", sayBar);

function sayFoo() {
  console.log("foo");
}

function sayBar() {
  console.log("bar");
}
<button id="buttonWithMultipleClickListenersExample">Click Me</button>

Two different event listeners are attached for the click event and they can do two completely separate things. If sayBar() is altered or even a sayBaz() is added, we don't need to change anything else, whereas if we define the click handler in the HTML, we have to change that function with every alteration needed.

\$\endgroup\$
  • \$\begingroup\$ Hello, Thanks for the input, but this is just a representational code and most your review comments are in place in actual code. I was looking for logical and functional review of my code. \$\endgroup\$ – Bhushan Babar Feb 5 at 5:18
  • 1
    \$\begingroup\$ It's hard to say if it is working, though. If it's a representation then maybe in the real code you don't just log the result. But I have no idea if in the real code you have the same problem as here where you never change ranges, so you aren't really ever merging ranges. And the unclear names are a real hindrance with reviewing the code. I opted to comment on naming because I have trouble following what's happening. \$\endgroup\$ – VLAZ Feb 5 at 5:41

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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