Filtering visible map layers based on multiple conditions

Question

I am working on a webapp using OpenLayers. I have written a filter function that I can use to tell whether a given layer is visible.

Since there are a bunch of conditions for a layer to be visible, I condensed them all into a single switch (true) block, and I was wondering if it was a good thing, since:

• switch (boolean) has obviously only two branches
• I think Java forbids switch (true) so it must be really bad somehow
• A case "falling through" is most of the time an error (forgot to break), so as a rule of thumb we forbid such case statements
• The language structure whose purpose is conditions is the if statement, which can be made shorter (less bytes of code) than the switch equivalent

But here I wrote a switch (true) version and I think this pattern is powerful since it's really easy to add/remove conditions, keep track of them, describe them, etc. After playing around with this method, I also wrote a version which uses an array of expressions instead of the switch.

So now I have three versions of the method. Which one do you think is the best and why?

/*global map */
// Sample usage
var visibleLayers = map.getLayers().getArray().filter(isVisible);

---

/*global map */
function isVisible(layer) {
 // Layer is not visible if it is hidden, has no data, or is transparent
 if (!layer || !layer.getVisible() || !layer.getSource() ||
 !layer.getOpacity()) {
 return false;
 }
 // Resolution limits
 var minRes = layer.getMinResolution() || 0;
 var maxRes = layer.getMaxResolution() || Infinity;
 var curRes = map.getView().getResolution();
 // Layer is not visible if the current resolution is too low or high
 if (curRes < minRes || maxRes < curRes) {
 return false;
 }
 // Layer is visible in our app only if its type matches
 if (['WMS', 'Vector'].indexOf(layer.getType()) !== -1) {
 return true;
 }
 return false;
}

---

/*global map */
function isVisible(layer) {
 if (!layer) {
 return false;
 }
 // Resolution limits
 var minRes = layer.getMinResolution() || 0;
 var maxRes = layer.getMaxResolution() || Infinity;
 var curRes = map.getView().getResolution();
 switch (true) {
 // Layer is hidden when any of these is true:
 case !layer.getVisible(): // - is hidden
 case !layer.getSource(): // - has no data
 case !layer.getOpacity(): // - is completely transparent
 case curRes < minRes: // - too low resolution
 case maxRes < curRes: // - too high resolution
 return false;
 // Layer is hidden *in our app* if none of these is true:
 case layer.getType() === 'WMS':
 case layer.getType() === 'Vector':
 return true;
 }
 return false;
}

---

/*global map */
function isVisible(layer) {
 if (!layer) {
 return false;
 }
 // Resolution limits
 var minRes = layer.getMinResolution() || 0;
 var maxRes = layer.getMaxResolution() || Infinity;
 var curRes = map.getView().getResolution();
 // Layer is hidden when any of these is true:
 var hideOn = [
 !layer.getVisible(), // - is hidden
 !layer.getSource(), // - has no data
 !layer.getOpacity(), // - is completely transparent
 curRes < minRes, // - too low resolution
 maxRes < curRes, // - too high resolution
 ];
 // Layer type must match
 var typeMatch = ['WMS', 'Vector'].indexOf(layer.getType()) !== -1;
 return typeMatch && !hideOn.some(Boolean);
}

(PS: layer.getType() is not an OpenLayers method.)

• GnxR: I agree that the new title is more appropriate since this is what the method does, but I think the question itself is more generic since it's only about JS coding style, the concept of "map layer" is trivial (though I'm new to this site so I may be wrong about the conventions here)

Answer 1

Okay, so here is how I decided (yes we can say a lot of things about 20 little lines):

Performance

• The first code (with a few if) is probably the best, since the || operator stops whenever a condition is evaluated to true.
• The second one (with the big switch) is a very lil bit slower, since we compute minRes, maxRes and curRes before evaluating the first exit conditions where they are unused.
  Otherwise, the switch statement stops whenever a truthy case is found.
• The third one (with the Array) is the most wrong approach if I want speed. We compute the variables, store the N conditions in an array of size N, so they're all evaluated, and then we loop through said array (.some() method) to tell whether it contains a truthy value.

However, regarding this specific app I'm working on, code legibility and future features are more important than a few microseconds.

Legibility

My problem here is that legibility is kinda subjective. I follow specific coding style and guidelines, and it's easier for me to read code which follows these guidelines that code which doesn't.

Sadly, on this project, there are no defined conventions and there's an interesting diversity of whitespace habits.

• The first code is the most legible for most people coming from C, but anyone who would come after me would be very tempted to write nested if, since there are loads of nested code in the repo.
• The second one is still quite legible for C pals, and honestly the individual comments on each condition are nice. Also I believe it is less tempting to nest conditions, since we would have to change the structure first.
• The last one is the most "eye-candy": it has the advantages of the switch, while being less heavy (case, break). However, it is trickier: "What on Earth is this !hideOn.some(Boolean) thing? Damn, I'll have to check the docs."

Actually, reading the doc is nice I think. Learning about .some() does not take long, and it's better if maintainers make sure they know some of the language before, well, maintaining.

Extensibility

Extensibility is also important to me. I know this code will not be reviewed now, but in a few months someone will probably ask: "what if we also check this and that?".

In fact, what's likely to happen is "what if we make these conditions conditional?", ie, add a parameter somewhere, and if this parameter has some value, then we'll do some more things. This is an interesting way to check extensibility. Now I'll insert some code since there's been too much text.

// First code with ifs; nice but we end up nesting conditions
if (configuration === 'wow') {
 doSomething();
 // Comment both conditions
 if (newCond1 || newCond2) {
 return false;
 }
} // Continue
// Second code with switch; I had higher expectations for you
switch () {
case configuration === 'wow':
 doSomething();
 // Comment both conditions
 if (newCond1 || newCond2) {
 return false;
 }
 // Non-breaking case!!
case ...
}
// Other possibility; we may end up with very long lines,
// or multiline conditions (I limit my lines at 80 chars)
switch () {
case configuration === 'wow':
 doSomething();
 // Non-breaking case!!
case configuration === 'wow' && newCond1: // Comment condition 1
case configuration === 'wow' && newCond2: // Comment condition 2
case ...
}
// Last code with Array; free candy
if (configuration === 'wow') {
 doSomething();
 hideOn.concat([
 newCond1, // Comment condition 1
 newCond2 // Comment condition 2
 ]);
}

In this particular situation, I think the last one is the most elegant. Given it's also the most legible in my opinion, and that I'm clearly biased, and that I want maintainers to read at least ES5 docs once in a while, my choice has been to use the third versions, with the array of booleans.