Classify the urgency by comparing the current time to two timestamps

Question

I'm currently working on a typescript-class with this method:

  /**
   * Gets the state of a package. The state is determined based on two timestamps stored as fields in the {@link
    * Package}. As the current time equals or passes a timestamp, the state changes.
   *
   * @param currentTime The time to compare the timestamps with
   * @param sourcePackage The package containing the two timestamps to compare the currentTime with
   * @returns {CardState} The calculated state of the package
   */
  static determineStateOfPackage(currentTime: Date, sourcePackage: Package): CardState {
    if (sourcePackage.targetTime.getTime() <= currentTime.getTime()) {
      return CardState.URGENT;
    }
    if (sourcePackage.warningTime.getTime() <= currentTime.getTime()) {
      return CardState.WARNING;
    }
    return CardState.FRIENDLY;
  }

It bugs me that the two ifs are repetitive but I can't really figure out how to improve this. How can I improve this code especially to avoid calling the two ifs with nearly the same content twice?

Answer 1

A couple notes:

• Is CardState an enum? If not, this function doesn't return a CardState object, but rather what appears to be values described within CardState class constants. Your signature should reflect this.
• I honestly don't think it is that problematic to have multiple if conditions as you are doing, as your usage is not increasing cyclomatic complexity of the method. Now, if you started introducing new states, then it may get unwieldy and be good reason for refactoring. As your code is now, it is very straightforward and easy to understand. Why over-complicate?
• Is currentTime a good variable name? This method does nothing to ensure the passed Date object represents a time that is "current". Why name this variable *Time when it holds a Date object?
• Consider less verbose determinePackageState vs. determineStateOfPackage package vs. sourcePackage.
• You could possibly set getTime() value to constant in order to remove redundant calls (though this is probably splitting hairs).

This might result in (slightly) easier to read code such as:

  static determinePackageState(date: Date, package: Package): CardState {
    const timestamp: number = date.getTime();
    if (package.targetTime.getTime() <= timestamp) {
      return CardState.URGENT;
    }
    if (package.warningTime.getTime() <= timestamp) {
      return CardState.WARNING;
    }
    return CardState.FRIENDLY;
  }

Answer 2

I would even go one step further than @IgorSoloydenko, namely

class CardStateSelector {
    static determineStateOfPackage(currentTime : Date, sourcePackage : Package) : CardState {
        return CardStateSelector.getCriticalState(currentTime, sourcePackage.targetTime, CardState.URGENT)
            || CardStateSelector.getCriticalState(currentTime, sourcePackage.warningTime, CardState.WARNING)
            || CardState.FRIENDLY;
    }

    private static getCriticalState(currentTime : Date, criticalTime : Date, state : CardState) : CardState {
        return criticalTime.getTime() <= currentTime.getTime() ? state : undefined;
    }
}

This would require all CardState enum values to be truthy.

Answer 3

Step 1: method extractions

At the very minimum, you can always extract the reusable(repeated) part of the code into an independent method and give it a proper self-explanatory name. This will not necessarily reduce the number of lines of resulting code, however, it will isolate (encapsulate) the specific logic in a single place which is a good achievement on its own

export class CardStateSelector {
  static determineStateOfPackage(currentTime: Date, sourcePackage: Package): CardState {
    if (CardStateSelector.isTimeCritical(currentTime, sourcePackage.targetTime)) {
      return CardState.URGENT;
    }
    if (CardStateSelector.isTimeCritical(currentTime, sourcePackage.warningTime)) {
      return CardState.WARNING;
    }
    return CardState.FRIENDLY;
  }

  private static isTimeCritical(currentTime: Date, baseTime: Date): boolean {
    return baseTime.getTime() <= currentTime.getTime()
  }
}

Step 2: applying the Strategy pattern [optional]

In your particular case applying the Strategy design pattern would be an overkill. However, if your code had more branching, or had more complex branch selection logic, or had heavier logic in branch(es), or a combination of those, I'd consider using Strategy.

This answer may be a good illustration of the pattern application in a situation similar to yours.

Update 1

ifs are just very nasty code smell. They should be avoided when the things start being complicated. There's even a small anti-if movement.

P.S.

Let me know if you want to see some pseudo-code or more real code for the Step 2.