Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

refactor(lodash): replace orderBy #698

Merged
merged 3 commits into from
May 9, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/SearchResults/generate-hierarchical-tree.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@

module.exports = generateTrees;

var orderBy = require('lodash/orderBy');

var orderBy = require('../functions/orderBy');
var find = require('../functions/find');
var prepareHierarchicalFacetSortBy = require('../functions/formatSort');

Expand Down
3 changes: 1 addition & 2 deletions src/SearchResults/index.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
'use strict';

var orderBy = require('lodash/orderBy');

var merge = require('lodash/merge');

var isFunction = require('lodash/isFunction');
Expand All @@ -10,6 +8,7 @@ var partial = require('lodash/partial');
var partialRight = require('lodash/partialRight');

var defaultsPure = require('../functions/defaultsPure');
var orderBy = require('../functions/orderBy');
var compact = require('../functions/compact');
var find = require('../functions/find');
var findIndex = require('../functions/findIndex');
Expand Down
79 changes: 79 additions & 0 deletions src/functions/orderBy.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
'use strict';

function compareAscending(value, other) {
if (value !== other) {
var valIsDefined = value !== undefined;
var valIsNull = value === null;

var othIsDefined = other !== undefined;
var othIsNull = other === null;

if (
(!othIsNull && value > other) ||
(valIsNull && othIsDefined) ||
!valIsDefined
) {
return 1;
}
if (
(!valIsNull && value < other) ||
(othIsNull && valIsDefined) ||
!othIsDefined
) {
return -1;
}
}
return 0;
}

/**
* @param {Array<object>} collection object with keys in attributes
* @param {Array<string>} iteratees attributes
* @param {Array<string>} orders asc | desc
*/
function orderBy(collection, iteratees, orders) {
if (!Array.isArray(collection)) {
return [];
Haroenv marked this conversation as resolved.
Show resolved Hide resolved
}

if (!Array.isArray(orders)) {
orders = [];
Haroenv marked this conversation as resolved.
Show resolved Hide resolved
}

var result = collection.map(function(value, index) {
return {
criteria: iteratees.map(function(iteratee) {
return value[iteratee];
}),
index: index,
value: value
};
});

result.sort(function comparer(object, other) {
var index = -1;

while (++index < object.criteria.length) {
var res = compareAscending(object.criteria[index], other.criteria[index]);
if (res) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could change this block to

if (res) {
  if (orders[index] === 'desc') {
    return -res;
  } else {
    return res;
  }
}

If index is out of range, it will fail === 'desc' anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about that, but I think this was added in lodash to shortcut the loop, although reading it again, it might not make a difference

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm no idea 🤷🏼‍♂️

if (index >= orders.length) {
return res;
}
if (orders[index] === 'desc') {
return -res;
}
return res;
}
}

// This ensures a stable sort in V8 and other engines.
// See https://bugs.chromium.org/p/v8/issues/detail?id=90 for more details.
return object.index - other.index;
});

return result.map(function(res) {
return res.value;
});
}

module.exports = orderBy;
60 changes: 60 additions & 0 deletions test/spec/functions/orderBy.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
'use strict';

var orderBy = require('../../../src/functions/orderBy');

var objects = [
{a: 'x', b: 3},
{a: 'y', b: 4},
{a: 'x', b: 1},
{a: 'y', b: 2}
];

it('should sort by a single property by a specified order', function() {
expect(orderBy(objects, ['a'], ['desc'])).toEqual([
objects[1],
objects[3],
objects[0],
objects[2]
]);
});

it('should sort by multiple properties by specified orders', function() {
expect(orderBy(objects, ['a', 'b'], ['desc', 'asc'])).toEqual([
objects[3],
objects[1],
objects[2],
objects[0]
]);
});

it('should sort by a property in ascending order when its order is not specified', function() {
expect(orderBy(objects, ['a', 'b'])).toEqual([
objects[2],
objects[0],
objects[3],
objects[1]
]);

expect(orderBy(objects, ['a', 'b'], ['desc'])).toEqual([
objects[3],
objects[1],
objects[2],
objects[0]
]);

[null, undefined, false, 0, NaN, ''].forEach(function(order) {
expect(orderBy(objects, ['a', 'b'], ['desc', order])).toEqual([
objects[3],
objects[1],
objects[2],
objects[0]
]);
});
});

it('should return an empty array when collections is no array', function() {
expect(orderBy(undefined)).toEqual([]);
expect(orderBy(false)).toEqual([]);
expect(orderBy({})).toEqual([]);
expect(orderBy({}, [], [])).toEqual([]);
});