Skip to content

Commit

Permalink
fix agg metrics, don't use "for .. in .." on JS arrays
Browse files Browse the repository at this point in the history
Agg metrics were broken because we iterated over an array using "for .. in ..", which has unexpected consequences.

"for .. in .." iterates over all the keys in an object. Arrays are a type of object, where the elements of the array are keyed by index.
But they can have other properties too (like their methods) so when you use "for .. in .." you get everything.

In general arrays should be iterated with "for .. of .." or ".forEach", but in this specific case I noticed we are basically just copying an array with some values substituted, so I opted for ".map".

Note: There is repeated code across these two files that should obviously be refactored later, but for now I just want to get this bug fixed.
  • Loading branch information
JGreenlee committed Aug 19, 2024
1 parent 90595e9 commit e6aea4e
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 14 deletions.
13 changes: 6 additions & 7 deletions www/js/metrics/CarbonFootprintCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -131,15 +131,14 @@ const CarbonFootprintCard = ({ userMetrics, aggMetrics }: Props) => {

// Issue 422:
// https://github.com/e-mission/e-mission-docs/issues/422
let aggCarbonData: MetricsSummary[] = [];
for (let i in aggThisWeekSummary) {
aggCarbonData.push(aggThisWeekSummary[i]);
if (isNaN(aggCarbonData[i].values)) {
let aggCarbonData: MetricsSummary[] = aggThisWeekSummary.map((summaryEntry) => {

Check warning on line 134 in www/js/metrics/CarbonFootprintCard.tsx

View check run for this annotation

Codecov / codecov/patch

www/js/metrics/CarbonFootprintCard.tsx#L134

Added line #L134 was not covered by tests
if (isNaN(summaryEntry.values)) {
logWarn(`WARNING in calculating groupCarbonRecords: value is NaN for mode
${aggCarbonData[i].key}, changing to 0`);
aggCarbonData[i].values = 0;
${summaryEntry.key}, changing to 0`);
summaryEntry.values = 0;

Check warning on line 138 in www/js/metrics/CarbonFootprintCard.tsx

View check run for this annotation

Codecov / codecov/patch

www/js/metrics/CarbonFootprintCard.tsx#L138

Added line #L138 was not covered by tests
}
}
return summaryEntry;

Check warning on line 140 in www/js/metrics/CarbonFootprintCard.tsx

View check run for this annotation

Codecov / codecov/patch

www/js/metrics/CarbonFootprintCard.tsx#L140

Added line #L140 was not covered by tests
});

let groupRecords: { label: string; x: number | string; y: number | string }[] = [];

Expand Down
13 changes: 6 additions & 7 deletions www/js/metrics/CarbonTextCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -107,15 +107,14 @@ const CarbonTextCard = ({ userMetrics, aggMetrics }: Props) => {

// Issue 422:
// https://github.com/e-mission/e-mission-docs/issues/422
let aggCarbonData: MetricsSummary[] = [];
for (let i in aggThisWeekSummary) {
aggCarbonData.push(aggThisWeekSummary[i]);
if (isNaN(aggCarbonData[i].values)) {
let aggCarbonData: MetricsSummary[] = aggThisWeekSummary.map((summaryEntry) => {

Check warning on line 110 in www/js/metrics/CarbonTextCard.tsx

View check run for this annotation

Codecov / codecov/patch

www/js/metrics/CarbonTextCard.tsx#L110

Added line #L110 was not covered by tests
if (isNaN(summaryEntry.values)) {
logWarn(`WARNING in calculating groupCarbonRecords: value is NaN for mode
${aggCarbonData[i].key}, changing to 0`);
aggCarbonData[i].values = 0;
${summaryEntry.key}, changing to 0`);
summaryEntry.values = 0;

Check warning on line 114 in www/js/metrics/CarbonTextCard.tsx

View check run for this annotation

Codecov / codecov/patch

www/js/metrics/CarbonTextCard.tsx#L114

Added line #L114 was not covered by tests
}
}
return summaryEntry;

Check warning on line 116 in www/js/metrics/CarbonTextCard.tsx

View check run for this annotation

Codecov / codecov/patch

www/js/metrics/CarbonTextCard.tsx#L116

Added line #L116 was not covered by tests
});

let groupText: { label: string; value: string }[] = [];

Expand Down

0 comments on commit e6aea4e

Please sign in to comment.