On Sun, Sep 24, 2023, 4:45 PM Lars Müller ***@***.***> wrote: ***@***.**** requested changes on this pull request. Please also add a proper JSDoc comment. Try to reuse the tests of existing sorting algorithms (but don't just copy and paste; simply execute the same tests for multiple sorting algorithms that offer the same API). ------------------------------ In Sorts/BitonicSort.js <#1370 (comment)> : > @@ -0,0 +1,74 @@ +/* + + * JS program for bitonic sort + * Bitonic Sort is a parallel sorting algorithm that works by dividing the + array into two parts, recursively sorting them, and then merging them in a + specific way. + * more information: https://en.wikipedia.org/wiki/Bitonic_sorter + + */ +function compAndSwap (a, i, j, order) { Rather than taking an order parameter, this should take a comparison function. ------------------------------ In Sorts/BitonicSort.js <#1370 (comment)> : > + + */ +function compAndSwap (a, i, j, order) { + if ((a[i] > a[j] && order === 1) || (a[i] < a[j] && order === 0)) { + // Swapping elements + const temp = a[i] + a[i] = a[j] + a[j] = temp + } +} + +// It recursively sorts a bitonic sequence in ascending +// order, if order = 1 +function bitonicMergeArr (a, low, cnt, dir) { + if (cnt > 1) { + const k = parseInt(cnt / 2) Why use parseInt instead of cnt >> 1 for floor division by 2? ------------------------------ In Sorts/BitonicSort.js <#1370 (comment)> : > + */ +function compAndSwap (a, i, j, order) { + if ((a[i] > a[j] && order === 1) || (a[i] < a[j] && order === 0)) { + // Swapping elements + const temp = a[i] + a[i] = a[j] + a[j] = temp + } +} + +// It recursively sorts a bitonic sequence in ascending +// order, if order = 1 +function bitonicMergeArr (a, low, cnt, dir) { + if (cnt > 1) { + const k = parseInt(cnt / 2) + for (let i = low; i < low + k; i++) { compAndSwap(a, i, i + k, dir) } Why not inline compAndSwap? ------------------------------ In Sorts/BitonicSort.js <#1370 (comment)> : > +} + +// It recursively sorts a bitonic sequence in ascending +// order, if order = 1 +function bitonicMergeArr (a, low, cnt, dir) { + if (cnt > 1) { + const k = parseInt(cnt / 2) + for (let i = low; i < low + k; i++) { compAndSwap(a, i, i + k, dir) } + bitonicMergeArr(a, low, k, dir) + bitonicMergeArr(a, low + k, k, dir) + } +} + +function bitonicSort (a, low, cnt, order) { // (arr 0 arrLen AS-Ds-order) + if (cnt > 1) { + const k = parseInt(cnt / 2) Same here (use cnt >> 1) ------------------------------ In Sorts/BitonicSort.js <#1370 (comment)> : > + + // sort in ascending order since order here is 1 + bitonicSort(a, low, k, 1) + + // sort in descending order since order here is 0 + bitonicSort(a, low + k, k, 0) + + // Will merge whole sequence in ascending order + // since dir=1. + bitonicMergeArr(a, low, cnt, order) + } +} + +// Calling of bitonicSort func for sorting the entire array +// of length N in ASCENDING order +// here up=1 for ASCENDING & up=0 for DESCENDING Again, I'd prefer a comparator. ------------------------------ In Sorts/BitonicSort.js <#1370 (comment)> : > + + // Will merge whole sequence in ascending order + // since dir=1. + bitonicMergeArr(a, low, cnt, order) + } +} + +// Calling of bitonicSort func for sorting the entire array +// of length N in ASCENDING order +// here up=1 for ASCENDING & up=0 for DESCENDING +function sort (a, N, up) { + bitonicSort(a, 0, N, up) +} + +// displaying array +function logArray (arr) { This function doesn't belong here. It should be removed. ------------------------------ In Sorts/BitonicSort.js <#1370 (comment)> : > + +// Calling of bitonicSort func for sorting the entire array +// of length N in ASCENDING order +// here up=1 for ASCENDING & up=0 for DESCENDING +function sort (a, N, up) { + bitonicSort(a, 0, N, up) +} + +// displaying array +function logArray (arr) { + for (let i = 0; i < arr.length; ++i) { console.log(arr[i] + ' ') } +} + +export { sort } + +// Test Case method This doesn't belong here. ------------------------------ In Sorts/test/BitonicSort.test.js <#1370 (comment)> : > @@ -0,0 +1,36 @@ +import {sort} from '../BitonicSort' This should use proper Jest tests. — Reply to this email directly, view it on GitHub <#1370 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AXFPHSQARWGOBP7473RJ6KTX4AI3RANCNFSM6AAAAAA5EULERY> . You are receiving this because you authored the thread.Message ID: ***@***.***>