patterntypescriptMinor
Classify the urgency by comparing the current time to two timestamps
Viewed 0 times
thecomparingurgencytimetwocurrentclassifytimestamps
Problem
I'm currently working on a typescript-class with this method:
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?
/**
* 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:
This might result in (slightly) easier to read code such as:
- Is
CardStatean enum? If not, this function doesn't return aCardStateobject, 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
currentTimea good variable name? This method does nothing to ensure the passedDateobject represents a time that is "current". Why name this variable*Timewhen it holds aDateobject?
- Consider less verbose
determinePackageStatevs.determineStateOfPackagepackagevs.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.