HiveBrain v1.2.0
Get Started
← Back to all entries
patterntypescriptMinor

Classify the urgency by comparing the current time to two timestamps

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thecomparingurgencytimetwocurrentclassifytimestamps

Problem

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?

Solution

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;
  }

Code Snippets

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;
  }

Context

StackExchange Code Review Q#161293, answer score: 3

Revisions (0)

No revisions yet.