Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce valid KpiResult scores #30

Merged
merged 3 commits into from
Oct 28, 2024
Merged

Enforce valid KpiResult scores #30

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a2445db
feat: check the score of all KpiCalculationResults to guarantee that …
janniclas Oct 28, 2024
7ce6c3d
test: added rangeCheck test for KpiCalculationResults without a score
janniclas Oct 28, 2024
5f85f43
chore: renamed validity check function and added javadoc
janniclas Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 44 additions & 1 deletion core/src/main/kotlin/de/fraunhofer/iem/spha/core/strategy/KpiCalculationStrategy.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import de.fraunhofer.iem.spha.core.hierarchy.KpiHierarchyEdge
import de.fraunhofer.iem.spha.model.kpi.KpiStrategyId
import de.fraunhofer.iem.spha.model.kpi.hierarchy.KpiCalculationResult
import de.fraunhofer.iem.spha.model.kpi.hierarchy.KpiNode
import io.github.oshai.kotlinlogging.KotlinLogging

internal fun getKpiCalculationStrategy(strategyId: KpiStrategyId): KpiCalculationStrategy {
return when (strategyId) {
Expand Down Expand Up @@ -60,6 +61,8 @@ internal interface KpiCalculationStrategy {
fun isValid(node: KpiNode, strict: Boolean = false): Boolean
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intented to be on global scope? How does the private modifier work in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the logging library says to use it this way, I didn't check why and trusted them.
private makes it accessible in the whole file and it should be initialized first.

private val logger = KotlinLogging.logger {}

internal abstract class BaseKpiCalculationStrategy : KpiCalculationStrategy {

abstract val kpiStrategyId: KpiStrategyId
Expand All @@ -82,7 +85,7 @@ internal abstract class BaseKpiCalculationStrategy : KpiCalculationStrategy {

updateEdgeWeights(edges = hierarchyEdges, strict)

val result = internalCalculateKpi(hierarchyEdges)
val result = getResultInValidRange(internalCalculateKpi(hierarchyEdges))

if (
hierarchyEdges.any { it.to.result !is KpiCalculationResult.Success } &&
Expand Down Expand Up @@ -149,4 +152,44 @@ internal abstract class BaseKpiCalculationStrategy : KpiCalculationStrategy {
}

protected abstract fun internalIsValid(node: KpiNode, strict: Boolean): Boolean

companion object {
/**
* Checks if `result.score` is in a valid range (0..100). If the score is lower 0 or higher
* than 100, we return 0 or 100 respectively.
*
* @param result
*/
fun getResultInValidRange(result: KpiCalculationResult): KpiCalculationResult {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this a point where we should throw in strict mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in strict mode we would fail even earlier as we then enforce the validity of the given KPI hierarchy and a valid hierarchy should not result in invalid values.
This function should mainly act as a failsafe if we somehow messed up and somewhere get invalid range results.

return when (result) {
is KpiCalculationResult.Success ->
createValidScore(result, result.score) { score ->
KpiCalculationResult.Success(score)
}
is KpiCalculationResult.Incomplete ->
createValidScore(result, result.score) { score ->
KpiCalculationResult.Incomplete(score, result.reason)
}
else -> result
}
}

private fun <T : KpiCalculationResult> createValidScore(
result: T,
score: Int,
createResult: (Int) -> T,
): T {
return when {
score < 0 -> {
logger.warn { "Calculation result score $result is out of bounds." }
createResult(0)
}
score > 100 -> {
logger.warn { "Calculation result score $result is out of bounds." }
createResult(100)
}
else -> result
}
}
}
}
59 changes: 59 additions & 0 deletions core/src/test/kotlin/de/fraunhofer/iem/spha/core/strategy/AbstractKpiCalculationTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import de.fraunhofer.iem.spha.model.kpi.RawValueKpi
import de.fraunhofer.iem.spha.model.kpi.hierarchy.KpiCalculationResult
import de.fraunhofer.iem.spha.model.kpi.hierarchy.KpiEdge
import de.fraunhofer.iem.spha.model.kpi.hierarchy.KpiNode
import kotlin.random.Random
import kotlin.test.fail
import org.junit.jupiter.api.Assertions.assertEquals
import org.junit.jupiter.api.Test
Expand Down Expand Up @@ -173,4 +174,62 @@ class AbstractKpiCalculationTest {
assertEquals(0.5, incompleteNode.hierarchyEdges.last().plannedWeight)
assertEquals(1.0, incompleteNode.hierarchyEdges.last().actualWeight)
}

@Test
fun isValidResultRange() {
(0..100).forEach { score ->
val successResult = KpiCalculationResult.Success(score)
val incompleteResult = KpiCalculationResult.Incomplete(score, "Incomplete")
assertEquals(
successResult,
BaseKpiCalculationStrategy.getResultInValidRange(successResult),
)
assertEquals(
incompleteResult,
BaseKpiCalculationStrategy.getResultInValidRange(incompleteResult),
)
}
}

@Test
fun isInvalidResultRange() {

val smallerThanZero = List(10) { Random.nextInt(-100, 0) }
val largerThanHundred = List(10) { Random.nextInt(101, 200) }

smallerThanZero.forEach { score ->
val successResult = KpiCalculationResult.Success(score)
val incompleteResult = KpiCalculationResult.Incomplete(score, "Incomplete")
assertEquals(
KpiCalculationResult.Success(0),
BaseKpiCalculationStrategy.getResultInValidRange(successResult),
)
assertEquals(
KpiCalculationResult.Incomplete(0, "Incomplete"),
BaseKpiCalculationStrategy.getResultInValidRange(incompleteResult),
)
}

largerThanHundred.forEach { score ->
val successResult = KpiCalculationResult.Success(score)
val incompleteResult = KpiCalculationResult.Incomplete(score, "Incomplete")
assertEquals(
KpiCalculationResult.Success(100),
BaseKpiCalculationStrategy.getResultInValidRange(successResult),
)
assertEquals(
KpiCalculationResult.Incomplete(100, "Incomplete"),
BaseKpiCalculationStrategy.getResultInValidRange(incompleteResult),
)
}
}

@Test
fun rangeCheckForNoScoreResults() {
val errorResult = KpiCalculationResult.Error("Error")
val emptyResult = KpiCalculationResult.Empty()

assertEquals(emptyResult, BaseKpiCalculationStrategy.getResultInValidRange(emptyResult))
assertEquals(errorResult, BaseKpiCalculationStrategy.getResultInValidRange(errorResult))
}
}