Choose a marker for the map based on the alerts it has

Question

I need help to refactor this code, I think I can avoid the first two functions that filter the array. Also in the third part where I use the result of these two filters:

let currentWarnings = f.emplacementsForecastForAlerts.nextHoursVariableValues
  .filter((x) =>
    RiskService.getInstance()
      .listDojoVaribleAlertsToShowInMap.map((x) => x.id)
      .includes(x.variable)
  )
  .filter((x) => x.isCurrentHourWarning);

let icon = SvgStrings.SITE_ICON_BASE.replace("$width", iconWidth.toString())
  .replaceAll(
    "$currentAlert",
    currentAlerts.length > 0 || currentWarnings.length > 0
      ? SvgStrings.CIRCLES_CURRENT_ALERTS.replaceAll(
          "$colorCurrentBorderRisk",
          currentAlerts.length > 0
            ? this.calculateColorBoolean(true)
            : this.calculateColorBoolean(false)
        )
      : ""
  )
  .replace(
    "$alerts",
    f.emplacementsForecastForAlerts.hasAlerts > 0 ||
      f.emplacementsForecastForAlerts.hasWarnings > 0
      ? SvgStrings.CIRCLE_NUM_ALERTS.replace(
          "$colorBorderRisk",
          this.calculateColor(f.emplacementsForecastForAlerts.hasAlerts, true)
        ).replaceAll(
          "$colorBodyRisk",
          this.calculateColor(f.emplacementsForecastForAlerts.hasAlerts, false)
        )
      : ""
  );

    private calculateColor(valueAlert: number, isBorder: boolean = false) {
        if (valueAlert > 0) {
            return isBorder ? COLOR_RED_BORDER : COLOR_RED;
        } else {
            return isBorder ? COLOR_YELLOW_BORDER : COLOR_YELLOW;
        }
    }

    private calculateColorBoolean(hastAlert: boolean) {
        return hastAlert ? COLOR_RED_BORDER : COLOR_YELLOW_BORDER;
    }

I'd like to avoid those initial filters and find a more recursive way to get the proper color for the marker.

Answer 1

There are variables and references which are not resolved. Based on the given code this is my review:

Let's start with the private functions first, as they seem pretty standalone and straight forward.

1. calculateColor

Let's first talk about the signature, what is a valueAlert? I'm assuming this is some kind of threshold value, when I look at the method body. I'd suggest to rename this parameter to something like value or even threshold.

The second parameter in this signature is a default boolean flag, keep in mind passing boolean values as parameters is ugly. The boolean value never expresses anything by looking at the function. Consider your example:

this.calculateColor(42, true);

There is NO information whatsoever, what this flag expresses. I'd explicitly have to look at the signature, and most likely even into the implementation to understand it.

Therefore I'd introduce an object to express the intent, something along the lines like:

interface ColorAndBorderOptions {
  value: number;
  hasBorder: boolean;
}

private getColorAndBorder(options: ColorAndBorderOptions);

Inside the method, get rid of the redundant else keyword. This reduces indentation and therefore readability.

So the refactored method would look like:

interface ColorAndBorderOptions {
  value: number;
  hasBorder: boolean;
}

private getColorAndBorder(options: ColorAndBorderOptions) {
  const isAlert = options.value > 0;
  
  if (isAlert) {
    return options.hasBorder ? COLOR_RED_BORDER : COLOR_RED;
  }

  return options.hasBorder ? COLOR_YELLOW_BORDER : COLOR_YELLOW;
}

2. calculateColorBoolean

What is a calculateColorBoolean? What does it calculate? Why is there a boolean as a parameter?

Those are the questions that come to my mind when looking at the method signature. To begin with, I'd rename it, to something like: border(...) maybe even getBorder(...). In regards to the parameters of the function, renaming the existing one should be sufficient: isAlert: boolean.

Resulting in the refactored method:

private getBorder(isAlert: boolean) {
  return isAlert ? COLOR_RED_BORDER : COLOR_YELLOW_BORDER;
}

Lets move to the variables

3. let currentWarnings

The only thing here I can say is, that the second filter method might be unnecessary and could be incorporated into the first one like so:

let currentWarnings = f.emplacementsForecastForAlerts
  .nextHoursVariableValues
  .filter((x) => RiskService
      .getInstance()
      .listDojoVaribleAlertsToShowInMap.map((x) => x.id)
      .includes(x.variable) && x.isCurrentHourWarning
  );

4. let icon

What is happening here is basically the manipulation of any SVG. So I'm focussing on the use-case of the private methods described in (1 & 2).

The currentAlerts can be written without any ternary operator as the method parameter takes a boolean:

SvgStrings.CIRCLES_CURRENT_ALERTS.replaceAll(
  "$colorCurrentBorderRisk",
  this.getBorder(currentAlerts.length > 0)
)

The second SvgStrings replacement would be changed to this:

SvgStrings.CIRCLE_NUM_ALERTS.replace(
  "$colorBorderRisk",
  this.getColorAndBorder({ value: f.emplacementsForecastForAlerts.hasAlerts, hasBorder: true })
  )
.replaceAll(
  "$colorBodyRisk",
  this.getColorAndBorder({ value: f.emplacementsForecastForAlerts.hasAlerts, hasBorder: true })
)

I noticed something strange, while writing, f.emplacementsForecastForAlerts.hasAlerts is used as it contains a number. The name hasAlerts does suggest otherwise. If it is under your control I would change this to explicitly express that this is not a boolean but a number. Something like numberOfAlerts.