Layers - Which is better readable?

[westnordost]

I am contemplating about an interface and I'd like your opinion which style you find more readable.

The following code snippets do the same: From given map data, color footways green and color the left and right side of a road each depending whether there is a sidewalk, it is separate or is tagged at all. Non-roads are filtered out, i.e. not displayed.

isDisplayed + getStyle

private val filter by lazy { """
    ways with (
        highway ~ trunk|trunk_link|primary|primary_link|secondary|secondary_link|tertiary|tertiary_link|unclassified|residential|living_street|pedestrian
        or highway ~ footway|steps
        or highway ~ path|bridleway|cycleway and foot ~ yes|designated
      )
      and area != yes
""".toElementFilterExpression() }

private val footways = setOf("footway", "steps", "path", "bridleway", "cycleway")

/** return whether this element should be displayed at all */
override fun isDisplayed(element: Element) = filter.matches(element)

/** returns how to style the given element for display on the map. Called only after isDisplayed. */
override fun getStyle(element: Element): PolylineStyle {
    if (element.tags["highway"] in footways) {
        return PolylineStyle(color = "#33cc00")
    } else {
        val sidewalkSides = createSidewalkSides(element.tags)
        if (sidewalkSides == null && !expectSidewalkTagging(element.tags)) {
            return PolylineStyle(null)
        }

        return PolylineStyle(
            color = null,
            colorLeft = sidewalkSides?.left.color,
            colorRight = sidewalkSides?.right.color
        )
    }
}

private fun expectSidewalkTagging(tags: Map<String, String>): Boolean =
    tags["highway"] != "living_street" && tags["highway"] != "pedestrian"

// (or alternatively have a roadFilter and footwayFilter separately - few more LOC)

Notes

• less flexible (i.e. elements cannot be styled depending on other elements; I consider this a feature though as I don't really want unnecessary complexity to creep in)
• if more than one type of element is displayed (different filters), it could lead to that each filter is evaluated twice (once in isDisplayed and once in getStyle)

getStyledElements

/** return pairs of element to style for all elements in the map data that should be displayed */
override fun getStyledElements(mapData: MapDataWithGeometry) =
    mapData.filter("""
        ways with 
          highway ~ trunk|trunk_link|primary|primary_link|secondary|secondary_link|tertiary|tertiary_link|unclassified|residential|living_street|pedestrian
          and area != yes
    """).map { it to getSidewalkStyle(it) } +
    mapData.filter("""
        ways with (
            highway ~ footway|steps
            or highway ~ path|bridleway|cycleway and foot ~ yes|designated
          ) and area != yes
    """).map { it to PolylineStyle("#33cc00") }

private fun getSidewalkStyle(element: Element): PolylineStyle {
    val sidewalkSides = createSidewalkSides(element.tags)
    if (sidewalkSides == null && !expectSidewalkTagging(element.tags)) {
        return PolylineStyle(null)
    }

    return PolylineStyle(
        color = null,
        colorLeft = sidewalkSides?.left.color,
        colorRight = sidewalkSides?.right.color
    )
}

private fun expectSidewalkTagging(tags: Map<String, String>): Boolean =
    tags["highway"] != "living_street" && tags["highway"] != "pedestrian"

Notes

• maybe a little more consistent because similar to OsmElementQuestType::getHighlightedElements
• more performant: The same performance improvement can be utilized as for OsmElementQuestType::getHighlightedElements. (This alone should not be the reason to use this, as an performance improvement can also be achieved otherwise, albeit with a little more code like e.g. requiring to specify which element types are displayed at all up-front)

---

Pinging some regular and recent contributors: @matkoniecz @FloEdelmann @mnalis @arrival-spring @peternewman @Cj-Malone

[peternewman]

getStyledElements seems a lot better to me as there's far less duplication of the filtering aspects.

The other one lists the footways in the filter, then again in the footways set and finally some in expectSidewalkTagging. Whereas with getStyledElements, they are only listed twice. There's no risk of adding something to the filter but then forgetting to style it too (or vice versa).

[mnalis]

It's little over my head as I'm not fully grasping what/how the implementation would work (e.g. What are the elements fetched used for exactly? When does the quest-equivalent itself comes into play? What does createSidewalkSides() do - is it provided by SC or something layer-writer needs to provide?), but few notes:

• is the style mandatory? I.e. if in isDisplayed + getStyle the element matches but has no style specified, does SC provide some default style or is that a fatal error? If fatal error, then I dislike this solution, as it would likely result in many bugs (if I were writing the code, at least 😄).
  However, if that situation is just fine (elements selected, but no style specified), than I actually prefer that situation as one could provide non-default style only to subset of elements making the code simpler, and separation between element selection and its styling is clearly separate.

• the original idea of getStyledElements seems good to me: "one selection query - one simple style" if the style must always be specified (see above) as it also cleanly separates element selection / styling, but that advantage seems to fall apart when the style selected is not simple (e.g. calling getSidewalkStyle which does more element checking, maybe even more selections?). Alternatively, it also seems it could lead to repetition of parts of selection queries, which I think would not be ideal.

[westnordost]

It's used for something like this

Device-2022-04-22-133826-1.mp4