Skip to content

Commit

Permalink
Refactor/2318/prepare migration of metric data services (#2514)
Browse files Browse the repository at this point in the history
refactor: move calculation of metric data into selectors

So that we can easily access them from Angular components to come during migration.

The only "real" change is that on onAttributeTypesChanged nodeMetricData and edgeMetricData aren't recalculated anymore, as they are independent of attributeType.

refs: #2318
Co-authored-by: Torsten Knauf <Torsten.Knauf@maibornwolff.de>
Co-authored-by: Ruben Bridgewater <ruben@bridgewater.de>
  • Loading branch information
3 people committed Nov 18, 2021
1 parent a09ebd1 commit d0b8567
Show file tree
Hide file tree
Showing 52 changed files with 369 additions and 601 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/)

## [unreleased] (Added 🚀 | Changed | Removed 🗑 | Fixed 🐞 | Chore 👨‍💻 👩‍💻)

### Chore 👨‍💻 👩‍💻

- Refactor where metric data are calculated ([#2514](https://github.com/MaibornWolff/codecharta/pull/2514)).

### Changed

- Improved the UI and usability of the Suspicious Metrics Feature ([#2376](https://github.com/MaibornWolff/codecharta/pull/2494)) <br>
Expand Down
1 change: 0 additions & 1 deletion visualization/app/codeCharta/codeCharta.model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,6 @@ export interface State {
treeMap: TreeMapSettings
files: FileState[]
lookUp: LookUp
metricData: MetricData
appStatus: AppStatus
}

Expand Down
11 changes: 8 additions & 3 deletions visualization/app/codeCharta/codeCharta.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,14 @@ import { ExportBlacklistType, ExportCCFile } from "./codeCharta.api.model"
import { getCCFiles, isSingleState } from "./model/files/files.helper"
import { DialogService } from "./ui/dialog/dialog.service"
import { CCValidationResult, ERROR_MESSAGES } from "./util/fileValidator"
import { setNodeMetricData } from "./state/store/metricData/nodeMetricData/nodeMetricData.actions"
import packageJson from "../../package.json"
import { clone } from "./util/clone"
import { nodeMetricDataSelector } from "./state/selectors/accumulatedData/metricData/nodeMetricData.selector"

const mockedNodeMetricDataSelector = nodeMetricDataSelector as unknown as jest.Mock
jest.mock("./state/selectors/accumulatedData/metricData/nodeMetricData.selector", () => ({
nodeMetricDataSelector: jest.fn()
}))

describe("codeChartaService", () => {
let codeChartaService: CodeChartaService
Expand Down Expand Up @@ -179,7 +184,7 @@ describe("codeChartaService", () => {
})

it("should load the default scenario after loading a valid file", () => {
storeService.dispatch(setNodeMetricData(metricData))
mockedNodeMetricDataSelector.mockImplementation(() => metricData)

codeChartaService.loadFiles([
{
Expand All @@ -196,7 +201,7 @@ describe("codeChartaService", () => {

it("should not load the default scenario after loading a valid file, that does not have the required metrics", () => {
metricData.pop()
storeService.dispatch(setNodeMetricData(metricData))
mockedNodeMetricDataSelector.mockImplementation(() => metricData)

codeChartaService.loadFiles([
{
Expand Down
3 changes: 2 additions & 1 deletion visualization/app/codeCharta/codeCharta.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { setIsLoadingFile } from "./state/store/appSettings/isLoadingFile/isLoad
import { FileSelectionState, FileState } from "./model/files/files"
import { getCCFile } from "./util/fileHelper"
import { setRecentFiles } from "./state/store/dynamicSettings/recentFiles/recentFiles.actions"
import { nodeMetricDataSelector } from "./state/selectors/accumulatedData/metricData/nodeMetricData.selector"

export class CodeChartaService {
static ROOT_NAME = "root"
Expand Down Expand Up @@ -72,7 +73,7 @@ export class CodeChartaService {
private setDefaultScenario() {
const { areaMetric, heightMetric, colorMetric } = ScenarioHelper.getDefaultScenarioSetting().dynamicSettings
const names = [areaMetric, heightMetric, colorMetric]
const metricNames = new Set(this.storeService.getState().metricData.nodeMetricData.map(x => x.name))
const metricNames = new Set(nodeMetricDataSelector(this.storeService.getState()).map(x => x.name))

if (names.every(metric => metricNames.has(metric))) {
this.storeService.dispatch(setState(ScenarioHelper.getDefaultScenarioSetting()))
Expand Down
Original file line number Diff line number Diff line change
@@ -1,35 +1,35 @@
import { CodeMapNode, FileMeta } from "../../../codeCharta.model"
import { FileState } from "../../../model/files/files"
import { fileStatesAvailable, getVisibleFileStates, isSingleState, isPartialState, isDeltaState } from "../../../model/files/files.helper"
import { fileStatesAvailable, isSingleState, isPartialState, isDeltaState } from "../../../model/files/files.helper"
import { AggregationGenerator } from "../../../util/aggregationGenerator"
import { clone } from "../../../util/clone"
import { NodeDecorator } from "../../../util/nodeDecorator"
import { createSelector } from "../../angular-redux/store"
import { filesSelector } from "../../store/files/files.selector"
import { metricDataSelector } from "../../store/metricData/metricData.selector"
import { CcState } from "../../store/store"
import { metricNamesSelector } from "./metricNames.selector"
import { metricNamesSelector } from "./metricData/metricNames.selector"
import { getDeltaFile } from "./utils/getDeltaFile"
import { addEdgeMetricsForLeaves } from "./utils/addEdgeMetricsForLeaves"
import { blacklistSelector } from "../../store/fileSettings/blacklist/blacklist.selector"
import { attributeTypesSelector } from "../../store/fileSettings/attributeTypes/attributeTypes.selector"
import { visibleFileStatesSelector } from "../visibleFileStates.selector"
import { metricDataSelector } from "./metricData/metricData.selector"

const accumulatedDataFallback = Object.freeze({
unifiedMapNode: undefined,
unifiedFileMeta: undefined
})

export const accumulatedDataSelector: (state: CcState) => { unifiedMapNode: CodeMapNode; unifiedFileMeta: FileMeta } = createSelector(
[metricDataSelector, filesSelector, attributeTypesSelector, blacklistSelector, metricNamesSelector],
(metricData, files, attributeTypes, blacklist, metricNames) => {
if (!fileStatesAvailable(files) || !metricData.nodeMetricData) return accumulatedDataFallback
[metricDataSelector, visibleFileStatesSelector, attributeTypesSelector, blacklistSelector, metricNamesSelector],
(metricData, fileStates, attributeTypes, blacklist, metricNames) => {
if (!fileStatesAvailable(fileStates) || !metricData.nodeMetricData) return accumulatedDataFallback

const data = getUndecoratedAccumulatedData(files)
const data = getUndecoratedAccumulatedData(fileStates)
if (!data || !data.map) return accumulatedDataFallback

NodeDecorator.decorateMap(data.map, metricData, blacklist)
addEdgeMetricsForLeaves(data.map, metricNames)
NodeDecorator.decorateParentNodesWithAggregatedAttributes(data.map, isDeltaState(files), attributeTypes)
NodeDecorator.decorateParentNodesWithAggregatedAttributes(data.map, isDeltaState(fileStates), attributeTypes)

return {
unifiedMapNode: data.map,
Expand All @@ -38,18 +38,18 @@ export const accumulatedDataSelector: (state: CcState) => { unifiedMapNode: Code
}
)

const getUndecoratedAccumulatedData = (files: FileState[]) => {
const getUndecoratedAccumulatedData = (fileStates: FileState[]) => {
// TODO this cloning shouldn't be necessary. When migrating to NgRx
// we should find and eliminate the responsible side effects
const visibleFileStates = clone(getVisibleFileStates(files))
const visibleFileStates = clone(fileStates)

if (isSingleState(files)) {
if (isSingleState(fileStates)) {
return visibleFileStates[0].file
}
if (isPartialState(files)) {
if (isPartialState(fileStates)) {
return AggregationGenerator.getAggregationFile(visibleFileStates.map(x => x.file))
}
if (isDeltaState(files)) {
if (isDeltaState(fileStates)) {
const [reference, comparison] = visibleFileStates
if (comparison && reference.file.map.name !== comparison.file.map.name) {
return AggregationGenerator.getAggregationFile(visibleFileStates.map(x => x.file))
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import { calculateEdgeMetricData, nodeEdgeMetricsMap } from "./edgeMetricData.selector"
import { FILE_STATES, VALID_NODE_WITH_PATH } from "../../../../util/dataMocks"
import { FileState } from "../../../../model/files/files"
import { clone } from "../../../../util/clone"
import { EdgeMetricDataService } from "../../../store/metricData/edgeMetricData/edgeMetricData.service"

describe("edgeMetricDataSelector", () => {
let fileStates: FileState[]

beforeEach(() => {
fileStates = clone(FILE_STATES)
fileStates[0].file.map = clone(VALID_NODE_WITH_PATH)
})

it("should create correct edge Metrics", () => {
const result = calculateEdgeMetricData(fileStates, [])

expect(result.map(x => x.name)).toContain("pairingRate")
expect(result.map(x => x.name)).toContain("otherMetric")
})

it("should calculate correct maximum value for edge Metrics", () => {
const result = calculateEdgeMetricData(fileStates, [])

expect(result.find(x => x.name === "pairingRate").maxValue).toEqual(2)
expect(result.find(x => x.name === "otherMetric").maxValue).toEqual(1)
})

it("should contain the None metric once", () => {
const result = calculateEdgeMetricData(fileStates, [])

expect(result.filter(x => x.name === EdgeMetricDataService.NONE_METRIC)).toHaveLength(1)
})

it("should sort the metrics after calculating them", () => {
const result = calculateEdgeMetricData(fileStates, [])

expect(result).toHaveLength(4)
expect(result[0].name).toBe("avgCommits")
expect(result[1].name).toBe("None")
expect(result[2].name).toBe("otherMetric")
expect(result[3].name).toBe("pairingRate")
})

it("metrics Map should contain correct entries", () => {
calculateEdgeMetricData(fileStates, [])

const pairingRateMapKeys = [...nodeEdgeMetricsMap.get("pairingRate").keys()]

expect(pairingRateMapKeys[0]).toEqual("/root/big leaf")
expect(pairingRateMapKeys[1]).toEqual("/root/Parent Leaf/small leaf")
expect(pairingRateMapKeys[2]).toEqual("/root/Parent Leaf/other small leaf")
})
})
Original file line number Diff line number Diff line change
@@ -1,41 +1,32 @@
import { EdgeMetricDataAction, EdgeMetricDataActions, setEdgeMetricData } from "./edgeMetricData.actions"
import { hierarchy } from "d3-hierarchy"
import { BlacklistItem, BlacklistType, Edge, EdgeMetricCount, EdgeMetricData } from "../../../../codeCharta.model"
import { getVisibleFileStates } from "../../../../model/files/files.helper"
import { isPathBlacklisted } from "../../../../util/codeMapHelper"
import { FileState } from "../../../../model/files/files"
import { EdgeMetricDataService } from "./edgeMetricData.service"
import { sortByMetricName } from "../metricData.reducer"
import { hierarchy } from "d3-hierarchy"
import { isPathBlacklisted } from "../../../../util/codeMapHelper"
import { createSelector } from "../../../angular-redux/store"
import { blacklistSelector } from "../../../store/fileSettings/blacklist/blacklist.selector"
import { EdgeMetricDataService } from "../../../store/metricData/edgeMetricData/edgeMetricData.service"
import { visibleFileStatesSelector } from "../../visibleFileStates.selector"
import { sortByMetricName } from "./sortByMetricName"

export type EdgeMetricCountMap = Map<string, EdgeMetricCount>
export type NodeEdgeMetricsMap = Map<string, EdgeMetricCountMap>
// Required for performance improvements
// TODO move this into store or create a selector for it, to prevent random access / non unidirectional data flow
// TODO move this as soon as edgeMetricData.service is deleted, to prevent random access / non unidirectional data flow
export let nodeEdgeMetricsMap: NodeEdgeMetricsMap = new Map()

export function edgeMetricData(state = setEdgeMetricData().payload, action: EdgeMetricDataAction) {
switch (action.type) {
case EdgeMetricDataActions.SET_EDGE_METRIC_DATA:
return action.payload
case EdgeMetricDataActions.CALCULATE_NEW_EDGE_METRIC_DATA:
return calculateMetrics(action.payload.fileStates, action.payload.blacklist)
default:
return state
}
}
export const edgeMetricDataSelector = createSelector([visibleFileStatesSelector, blacklistSelector], calculateEdgeMetricData)

function calculateMetrics(fileStates: FileState[], blacklist: BlacklistItem[]) {
export function calculateEdgeMetricData(visibleFileStates: FileState[], blacklist: BlacklistItem[]) {
nodeEdgeMetricsMap = new Map()
const allVisibleFileStates = getVisibleFileStates(fileStates)
const allFilePaths: Set<string> = new Set()

for (const { file } of allVisibleFileStates) {
for (const { file } of visibleFileStates) {
for (const { data } of hierarchy(file.map)) {
allFilePaths.add(data.path)
}
}

for (const fileState of allVisibleFileStates) {
for (const fileState of visibleFileStates) {
for (const edge of fileState.file.settings.fileSettings.edges) {
if (bothNodesAssociatedAreVisible(edge, allFilePaths, blacklist)) {
// TODO: We likely only need the attributes once per file.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { createSelector } from "../../../angular-redux/createSelector"
import { edgeMetricDataSelector } from "./edgeMetricData.selector"
import { nodeMetricDataSelector } from "./nodeMetricData.selector"

export const metricDataSelector = createSelector([nodeMetricDataSelector, edgeMetricDataSelector], (nodeMetricData, edgeMetricData) => ({
nodeMetricData,
edgeMetricData
}))
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { CcState } from "../../../store/store"
import { edgeMetricDataSelector } from "./edgeMetricData.selector"
import { metricNamesSelector } from "./metricNames.selector"

const mockedEdgeMetricDataSelector = edgeMetricDataSelector as unknown as jest.Mock
jest.mock("./edgeMetricData.selector", () => ({
edgeMetricDataSelector: jest.fn()
}))

describe("metricNamesSelector", () => {
it("should get the names of the metrics", () => {
mockedEdgeMetricDataSelector.mockImplementationOnce(() => [{ name: "metricOfTruth" }, { name: "otherMetric" }])

expect(metricNamesSelector({} as unknown as CcState)).toEqual(["metricOfTruth", "otherMetric"])
})
})
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import { createSelector } from "../../../angular-redux/createSelector"
import { edgeMetricDataSelector } from "./edgeMetricData.selector"

export const metricNamesSelector = createSelector([edgeMetricDataSelector], edgeMetricData => edgeMetricData.map(x => x.name))
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
import { TEST_DELTA_MAP_A, VALID_NODE_WITH_ROOT_UNARY } from "../../../../util/dataMocks"
import { FileSelectionState, FileState } from "../../../../model/files/files"
import { BlacklistType } from "../../../../codeCharta.model"
import { NodeDecorator } from "../../../../util/nodeDecorator"
import { clone } from "../../../../util/clone"
import { NodeMetricDataService } from "../../../store/metricData/nodeMetricData/nodeMetricData.service"
import { calculateNodeMetricData } from "./nodeMetricData.selector"

describe("nodeMetricDataSelector", () => {
let fileState: FileState

beforeEach(() => {
const file = clone(TEST_DELTA_MAP_A)
NodeDecorator.decorateMapWithPathAttribute(file)
fileState = {
file,
selectedAs: FileSelectionState.Single
}
})

it("should return a sorted array of metricData sorted by name calculated from visibleFileStates", () => {
const expected = [
{ maxValue: 1000, minValue: 10, name: "functions" },
{ maxValue: 100, minValue: 1, name: "mcc" },
{ maxValue: 100, minValue: 30, name: "rloc" },
{ maxValue: 1, minValue: 1, name: NodeMetricDataService.UNARY_METRIC }
]

const result = calculateNodeMetricData([fileState], [])

expect(result).toEqual(expected)
})

it("should ignore blacklisted nodes", () => {
const expected = [
{ maxValue: 1000, minValue: 100, name: "functions" },
{ maxValue: 100, minValue: 10, name: "mcc" },
{ maxValue: 70, minValue: 30, name: "rloc" },
{ maxValue: 1, minValue: 1, name: NodeMetricDataService.UNARY_METRIC }
]

const result = calculateNodeMetricData([fileState], [{ path: "root/big leaf", type: BlacklistType.exclude }])

expect(result).toEqual(expected)
})

it("should always add unary metric if it's not included yet", () => {
const result = calculateNodeMetricData([fileState], [])

expect(result.filter(x => x.name === NodeMetricDataService.UNARY_METRIC)).toHaveLength(1)
})

it("should not add unary metric a second time if the cc.json already contains unary", () => {
fileState.file.map = VALID_NODE_WITH_ROOT_UNARY

const result = calculateNodeMetricData([fileState], [])

expect(result.filter(x => x.name === NodeMetricDataService.UNARY_METRIC).length).toBe(1)
})
})
Original file line number Diff line number Diff line change
@@ -1,28 +1,19 @@
import { NodeMetricDataAction, NodeMetricDataActions, setNodeMetricData } from "./nodeMetricData.actions"
import { hierarchy } from "d3-hierarchy"

import { BlacklistItem, BlacklistType, NodeMetricData } from "../../../../codeCharta.model"
import { getVisibleFileStates } from "../../../../model/files/files.helper"
import { FileState } from "../../../../model/files/files"
import { isLeaf, isPathBlacklisted } from "../../../../util/codeMapHelper"
import { hierarchy } from "d3-hierarchy"
import { NodeMetricDataService } from "./nodeMetricData.service"
import { sortByMetricName } from "../metricData.reducer"

export function nodeMetricData(state = setNodeMetricData().payload, action: NodeMetricDataAction) {
switch (action.type) {
case NodeMetricDataActions.SET_NODE_METRIC_DATA:
return action.payload
case NodeMetricDataActions.CALCULATE_NEW_NODE_METRIC_DATA:
return setNewMetricData(action.payload.fileStates, action.payload.blacklist)
default:
return state
}
}
import { createSelector } from "../../../angular-redux/store"
import { blacklistSelector } from "../../../store/fileSettings/blacklist/blacklist.selector"
import { NodeMetricDataService } from "../../../store/metricData/nodeMetricData/nodeMetricData.service"
import { visibleFileStatesSelector } from "../../visibleFileStates.selector"
import { sortByMetricName } from "./sortByMetricName"

function setNewMetricData(fileStates: FileState[], blacklist: BlacklistItem[]) {
export const calculateNodeMetricData = (visibleFileStates: FileState[], blacklist: BlacklistItem[]) => {
const metricMaxValues: Map<string, number> = new Map()
const metricMinValues: Map<string, number> = new Map()

for (const { file } of getVisibleFileStates(fileStates)) {
for (const { file } of visibleFileStates) {
for (const node of hierarchy(file.map)) {
if (isLeaf(node) && node.data.path && !isPathBlacklisted(node.data.path, blacklist, BlacklistType.exclude)) {
for (const metric of Object.keys(node.data.attributes)) {
Expand Down Expand Up @@ -58,3 +49,5 @@ function setNewMetricData(fileStates: FileState[], blacklist: BlacklistItem[]) {
sortByMetricName(metricData)
return metricData
}

export const nodeMetricDataSelector = createSelector([visibleFileStatesSelector, blacklistSelector], calculateNodeMetricData)
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
export const sortByMetricName = (metricData: { name: string }[]) => {
metricData.sort((a, b) => {
const aLower = a.name.toLowerCase()
const bLower = b.name.toLowerCase()
return aLower > bLower ? 1 : bLower > aLower ? -1 : 0
})
}

0 comments on commit d0b8567

Please sign in to comment.