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.
1 Answer 1
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.
-
\$\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\$BeeBee8– BeeBee82019年02月05日 05:18:47 +00:00Commented Feb 5, 2019 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– VLAZ2019年02月05日 05:41:25 +00:00Commented Feb 5, 2019 at 5:41