feat: Add GaleShapley in a new folder Greedy #1714
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1714 +/- ##
==========================================
+ Coverage 84.65% 84.71% +0.06%
==========================================
Files 378 379 +1
Lines 19744 19822 +78
Branches 2952 2957 +5
==========================================
+ Hits 16715 16793 +78
Misses 3029 3029 ☔ View full report in Codecov by Sentry. |
|
@raklaptudirm can you please review |
| import { expect, test } from 'vitest' | ||
| import { stableMatching } from '../GaleShapley' | ||
|
|
||
| test('Test Case 1', () => { |
There was a problem hiding this comment.
A test case structure like this isn't helpful. Please see https://github.com/TheAlgorithms/TypeScript/blob/master/CONTRIBUTING.md#writing-good-tests (it's for the TS repo, but translates to JS just as well).
TL;DR: Use a describe block for Gale-Shapley and it.each for individual test cases if you have no descriptive labels for them.
| * stableMatching(donorPref, recipientPref); // Output: [1, 2, 3, 0] | ||
| */ | ||
| function stableMatching(donorPref, recipientPref) { | ||
| // Initialize the number of donors and create a list of unmatched donors |
There was a problem hiding this comment.
I'd use this array as a stack because that's efficient.
| let unmatchedDonors = Array.from({ length: n }, (_, i) => i) | ||
|
|
||
| // Records of which recipient each donor is paired with and vice versa | ||
| let donorRecord = Array(n).fill(-1) // Donor to recipient |
There was a problem hiding this comment.
I'd compute donorRecord at the end (or not at all even and instead just return recRecord and document that because the two are equivalent). That way you save yourself having to remember to update it during the algorithm, and the constant factor of the
|
|
||
| // While there are unmatched donors | ||
| while (unmatchedDonors.length > 0) { | ||
| // Take the first unmatched donor |
There was a problem hiding this comment.
Don't. Take the last one instead; use the array as a stack (you could also use a queue, but why do that when a stack suffices). If you take the first one, you have to do a costly
|
|
||
| // Find the next recipient this donor prefers | ||
| let recipient = donorPreference[numDonations[donor]] | ||
| numDonations[donor] += 1 |
There was a problem hiding this comment.
| numDonations[donor] += 1 | |
| numDonations[donor]++ |
|
|
||
| // Records of which recipient each donor is paired with and vice versa | ||
| let donorRecord = Array(n).fill(-1) // Donor to recipient | ||
| let recRecord = Array(n).fill(-1) // Recipient to donor |
There was a problem hiding this comment.
I also think it'd be idiomatic to use null rather than -1 to signal the absence of a value.
|
|
||
| // If recipient is already matched, check if they prefer the new donor | ||
| if (prevDonor !== -1) { | ||
| if (recPreference.indexOf(prevDonor) > recPreference.indexOf(donor)) { |
There was a problem hiding this comment.
This also wrecks time complexity by adding a linear factor. Instead you should precompute the inverse mappings initially as lookup tables (this is what's responsible for the
Describe your change:
Checklist:
Example:
UserProfile.jsis allowed butuserprofile.js,Userprofile.js,user-Profile.js,userProfile.jsare not