-
Notifications
You must be signed in to change notification settings - Fork 1
Feat/ahp #18
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.
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
Feat/ahp #18
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds AHP (Analytic Hierarchy Process) functionality to the application, implementing a multi-criteria decision analysis tool that can compute weighted rankings of alternatives based on criteria comparisons.
Changes:
- Added mathjs library dependency for matrix operations
- Implemented AHP computation utilities including matrix validation, priority vector calculation, and consistency ratio computation
- Created API endpoint
/api/v1/ahpfor data-driven AHP calculations
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/validateMatrix.js | Matrix validation for reciprocal matrices used in AHP |
| src/utils/pairwiseMatrix.js | Builds pairwise comparison matrices from raw data |
| src/utils/computePriorityVector.js | Computes priority vectors using geometric mean method |
| src/utils/computeConsistancyRatio.js | Calculates consistency ratio for pairwise matrices |
| src/utils/ahs.js | Core AHP computation logic |
| src/controllers/ahpController.js | HTTP controller for AHP endpoint |
| src/routes/ahpRoutes.js | Express router for AHP routes |
| server.js | Registers AHP routes in the application |
| package.json | Adds mathjs dependency |
| package-lock.json | Lock file updates for mathjs and its dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { create, all } from "mathjs"; | ||
|
|
||
| const math = create(all, {}); |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mathjs instance is created but never used in this file. The math.multiply function is called on line 14, but since mathjs is imported and configured, you should verify if this configuration is necessary. If the multiply function works without the custom math instance, consider removing the unused constant.
| import { create, all } from "mathjs"; | ||
| import matrixValidation from "./validateMatrix.js"; | ||
| import weightVectorGeometricMean from "./computePriorityVector.js"; | ||
| import { consistencyRatio } from "./computeConsistancyRatio.js"; | ||
|
|
||
| const math = create(all, {}); | ||
|
|
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mathjs instance is created but never used in this file. Since no mathjs operations are performed directly in this module, consider removing this unused import and constant.
| import { create, all } from "mathjs"; | |
| import matrixValidation from "./validateMatrix.js"; | |
| import weightVectorGeometricMean from "./computePriorityVector.js"; | |
| import { consistencyRatio } from "./computeConsistancyRatio.js"; | |
| const math = create(all, {}); | |
| import matrixValidation from "./validateMatrix.js"; | |
| import weightVectorGeometricMean from "./computePriorityVector.js"; | |
| import { consistencyRatio } from "./computeConsistancyRatio.js"; |
| console.log("Validating matrix:", matrix); | ||
|
|
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debug console.log statement should be removed from production code. This could log potentially large matrices and cause performance issues. If logging is needed, consider using a proper logging library with configurable log levels.
| console.log("Validating matrix:", matrix); | |
| // debug: log matrix shapes (removed after diagnosing issues) | ||
| try { | ||
| // avoid large dumps; log dimensions and a sample | ||
| const altKeys = Object.keys(alternativeMatrices); | ||
| const altShapes = altKeys.map(k => ({ key: k, size: alternativeMatrices[k]?.length ?? 0 })); | ||
| console.log('computeDataDrivenAHP: criteriaMatrix size=', criteriaMatrix.length, 'alternativeMatrices=', altShapes); | ||
| } catch (e) { | ||
| console.error('computeDataDrivenAHP debug error', e); | ||
| } | ||
|
|
||
|
|
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debug console.log statement should be removed from production code. The comment on line 51 mentions 'removed after diagnosing issues' but the log is still present. Consider using a proper logging library with configurable log levels or remove this debug code.
| // debug: log matrix shapes (removed after diagnosing issues) | |
| try { | |
| // avoid large dumps; log dimensions and a sample | |
| const altKeys = Object.keys(alternativeMatrices); | |
| const altShapes = altKeys.map(k => ({ key: k, size: alternativeMatrices[k]?.length ?? 0 })); | |
| console.log('computeDataDrivenAHP: criteriaMatrix size=', criteriaMatrix.length, 'alternativeMatrices=', altShapes); | |
| } catch (e) { | |
| console.error('computeDataDrivenAHP debug error', e); | |
| } | |
| } | ||
|
|
||
| try { | ||
| console.log("Start running AHP computation"); |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debug console.log statement should be removed from production code. Consider using a proper logging library consistent with the rest of the application, or remove this debug statement.
| console.log("Start running AHP computation"); |
| const { criteriaNames, alternativeNames, dataMatrix, benefitFlags } = req.body || {}; | ||
|
|
||
| if (!req.body) { | ||
| return res.status(400).json({ error: 'Request body is required. Please send JSON with dataMatrix, criteriaNames, alternativeNames, and benefitFlags.' }); | ||
| } | ||
|
|
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check on line 6 is redundant since req.body is already accessed on line 4 with a fallback to an empty object. If req.body is null/undefined, the destructuring on line 4 will use {} and the variables will be undefined. The check should be moved before the destructuring, or specific parameter validation should be added instead.
| const { criteriaNames, alternativeNames, dataMatrix, benefitFlags } = req.body || {}; | |
| if (!req.body) { | |
| return res.status(400).json({ error: 'Request body is required. Please send JSON with dataMatrix, criteriaNames, alternativeNames, and benefitFlags.' }); | |
| } | |
| const body = req.body; | |
| if (!body || typeof body !== 'object') { | |
| return res.status(400).json({ error: 'Request body is required. Please send JSON with dataMatrix, criteriaNames, alternativeNames, and benefitFlags.' }); | |
| } | |
| const { criteriaNames, alternativeNames, dataMatrix, benefitFlags } = body; |
| if (!Array.isArray(dataMatrix)) { | ||
| return res.status(400).json({ error: 'dataMatrix must be an array.' }); | ||
| } | ||
|
|
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incomplete input validation. The controller only validates that dataMatrix is an array but doesn't validate other required parameters (criteriaNames, alternativeNames). These are required by computeDataDrivenAHP and will cause unclear error messages if missing. Add validation for all required parameters.
| if (!Array.isArray(criteriaNames) || criteriaNames.length === 0) { | |
| return res.status(400).json({ error: 'criteriaNames must be a non-empty array.' }); | |
| } | |
| if (!Array.isArray(alternativeNames) || alternativeNames.length === 0) { | |
| return res.status(400).json({ error: 'alternativeNames must be a non-empty array.' }); | |
| } |
| @@ -0,0 +1,30 @@ | |||
| import { create, all } from "mathjs"; | |||
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filename has a typo: 'Consistancy' should be 'Consistency'. The file is named 'computeConsistancyRatio.js' but should be 'computeConsistencyRatio.js'.
| @@ -0,0 +1,28 @@ | |||
| import { computeDataDrivenAHP } from "../utils/pairwiseMatrix.js"; | |||
|
|
|||
| export const dataDrivenAHP = (req, res) => { | |||
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new AHP controller lacks test coverage. All other controllers in the codebase (branchController, messageController, thirdPartiesController, etc.) have corresponding test files in src/tests/. Consider adding comprehensive tests for the AHP controller covering successful computation, validation errors, and error handling.
| import express from 'express'; | ||
| import { dataDrivenAHP } from '../controllers/ahpController.js'; | ||
|
|
||
| const router = express.Router(); |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import style is inconsistent with other route files in the codebase. Other route files use destructured imports like import { Router } from 'express' (see branch.js line 2). Consider using the same pattern for consistency.
| import express from 'express'; | |
| import { dataDrivenAHP } from '../controllers/ahpController.js'; | |
| const router = express.Router(); | |
| import { Router } from 'express'; | |
| import { dataDrivenAHP } from '../controllers/ahpController.js'; | |
| const router = Router(); |
vindyakodithuwakku02
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI fixed and checks passed.
No description provided.