0
\$\begingroup\$

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.

\$\endgroup\$
4
  • 1
    \$\begingroup\$ This doesn't really have enough code to review. What's f, and where does it come from? (And why such a short name?) There are some other things that don't seem to be defined (unless your language/library provides them?) \$\endgroup\$ Mar 13, 2023 at 16:27
  • \$\begingroup\$ Thanks, Toby it's my first time creating a question here, Should I put all my file? \$\endgroup\$ Mar 13, 2023 at 16:46
  • 1
    \$\begingroup\$ If you want all of it to be reviewed, then yes (that's what I'd recommend - and it can help reviewers if they are able to reproduce your results). If you only want part of it reviewed for some reason, then at least provide enough context that we can understand it. We have a very generous question size limit here, so full code is usually preferred. \$\endgroup\$ Mar 13, 2023 at 17:02
  • \$\begingroup\$ Ok @TobySpeight thanks for your recommendation. I am going to edit better my question. Thanks for your suggestions. \$\endgroup\$ Mar 14, 2023 at 9:11

1 Answer 1

1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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