Repository navigation
Glasgow | 26-Jul-SDC | Shreef Ibrahim | Sprint 2 | Analyse Refactor Functions - #239
shreefAhmedM wants to merge 5 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
shreefAhmedM
added
the
Needs Review
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
removed
the
Needs Review
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
shreefAhmedM
changed the title
shreefAhmedM
added
the
Needs Review
illicitonion
left a comment
illicitonion
left a comment
There was a problem hiding this comment.
This generally looks good, but has a lot of lack of attention to detail - please take a look and fix up the comments
| */ | ||
| export function calculateSumAndProduct(numbers) { | ||
| let sum = 0; | ||
| let product =1 |
There was a problem hiding this comment.
This code isn't well formatted - it uses inconsistent spacing, and lines you added are missing trailing ; characters. Please fix.
There was a problem hiding this comment.
You haven't explained the complexity of the before or after code
| * @returns {Array} Array containing unique common items | ||
| * | ||
| * Time Complexity: O(n * m) | ||
| * Space Complexity: O(u) |
|
|
||
| console.log(findCommonItems([2, 5, 7], [5, 6, 7])); | ||
|
|
||
| // Time:O(n \ m) filter() visits each item in the first array, and includes() may scan the second array each time. |
There was a problem hiding this comment.
What is
n \ m?
I updated the complexity; I did not intend to use k and u t, as it is different from my local update I pushed the changes. The new implementation improves the average time complexity from O(n × m) to O(n + m) by using a Set for faster lookups. The space complexity is O(n + m) because the code stores the second array’s items in a Set and creates the filtered results.
| console.log(findCommonItems([2, 5, 7], [5, 6, 7])); | ||
|
|
||
| // Time:O(n \ m) filter() visits each item in the first array, and includes() may scan the second array each time. | ||
| // Sace: O(k) The Set stores unique common items, and the resulting array contains those items. |
There was a problem hiding this comment.
What is k and how does it relate to the u you used above?
| * Optimal Time Complexity: | ||
| * | ||
| * Time Complexity: O(N^2) | ||
| // Space Complexity: O(1) |
There was a problem hiding this comment.
Is the space complexity O(1) both before and after your change?
There was a problem hiding this comment.
Is the space complexity O(1) both before and after your change?
The original uses O(1) space and O(n²) time. The updated version uses O(n) space but improves the time to O(n) on average by using a Set for faster lookups.
illicitonion
added
Reviewed
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
shreefAhmedM
changed the title
This comment has been minimized.
This comment has been minimized.
shreefAhmedM
added
the
Needs Review
2 participants
Learners, PR Template
Self checklist
Task code
CYF-1173
Changelist
This PR analyses and refactors functions to improve efficiency, reduce time complexity, and follow best practices. It documents the original and optimised time and space complexities for each function.