Skip to content

Commit dbd70e0

Browse files
shunjizhanshunjizhan
committed
performance improvement
Removed unnecessary deepclone in state transition functions. Now we only do deepclone once when initializing state, to separate internal state from outside, subsequent state operations then can be done in-place to improve performance. need more tests and verifications before deploying!!!
1 parent 3e441ea commit dbd70e0

File tree

3 files changed

+43
-51
lines changed

3 files changed

+43
-51
lines changed

‎src/components/FolderTree/FolderTree.jsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import React, {
55
import PropTypes from 'prop-types';
66

77
import {
8-
addUniqIds,
8+
initStateWithUniqIds,
99
checkNode,
1010
setAllCheckedStatus,
1111
isValidCheckedStatus,
@@ -40,7 +40,7 @@ const FolderTree = ({
4040
};
4141

4242
useEffect(() => {
43-
let initState = addUniqIds(data);
43+
let initState = initStateWithUniqIds(data);
4444

4545
switch (initCheckedStatus) {
4646
case 'unchecked':

‎src/utils/utils.js

Lines changed: 23 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,8 @@
11
export const deepClone = x => JSON.parse(JSON.stringify(x));
22

3-
// TODO: add description on the top of all functions
4-
// TODO: replace deepclone with { ...rootnode } and make sure it still works
5-
// TODO: implement isValidCheckedStatus()
6-
7-
// assign uniq ids to each node
8-
export const addUniqIds = rootNode => {
3+
// deepclone the initial data for internal use, and assign uniq ids to each node
4+
// deepclone only happens once at initialization, other operations will be in-place
5+
export const initStateWithUniqIds = rootNode => {
96
let curId = 0;
107
const _addId = node => {
118
node._id = curId; // eslint-disable-line
@@ -33,9 +30,14 @@ const setStatusDown = (node, status) => {
3330
setStatusDown(child, status);
3431
}
3532
}
36-
return node;
33+
return { ...node };
3734
};
3835

36+
// set checked status for all nodes
37+
export const setAllCheckedStatus = (rootNode, status) => (
38+
setStatusDown(rootNode, status)
39+
);
40+
3941
// calculate the check status of a node based on the check status of it's children
4042
export const getNewCheckStatus = node => {
4143
const { children } = node;
@@ -66,15 +68,9 @@ export const updateStatusUp = nodes => {
6668
updateStatusUp(nodes);
6769
};
6870

69-
// set checked status for all nodes
70-
export const setAllCheckedStatus = (rootNode, status) => (
71-
setStatusDown(deepClone(rootNode), status)
72-
);
73-
7471
// handle state change when user (un)check a TreeNode
7572
export const checkNode = (rootNode, path, status) => {
76-
const _rootNode = deepClone(rootNode);
77-
let curNode = _rootNode;
73+
let curNode = rootNode;
7874
const parentNodes = [curNode]; // parent nodes for getNewCheckStatus() upwards
7975

8076
for (const idx of path) {
@@ -87,33 +83,30 @@ export const checkNode = (rootNode, path, status) => {
8783
parentNodes.pop(); // don't need to check this node's level
8884
updateStatusUp(parentNodes); // update check status up, from this nodes parent, in place
8985

90-
return _rootNode;
86+
return { ...rootNode };
9187
};
9288

9389
export const renameNode = (rootNode, path, newName) => {
94-
const _rootNode = deepClone(rootNode);
95-
let curNode = _rootNode;
90+
let curNode = rootNode;
9691
for (const idx of path) {
9792
curNode = curNode.children[idx];
9893
}
9994
curNode.name = newName;
10095

101-
return _rootNode;
96+
return { ...rootNode };
10297
};
10398

10499
export const deleteNode = (rootNode, path) => {
105-
const _rootNode = deepClone(rootNode);
106-
100+
let curNode = rootNode;
107101
if (path.length === 0) {
108102
// this is root node
109103
// just remove every children and reset check status to 0
110-
_rootNode.children = [];
111-
_rootNode.checked = 0;
104+
curNode.children = [];
105+
curNode.checked = 0;
112106

113-
return _rootNode;
107+
return curNode;
114108
}
115109

116-
let curNode = _rootNode;
117110
const parentNodes = [curNode];
118111
const lastIdx = path.pop();
119112

@@ -125,7 +118,7 @@ export const deleteNode = (rootNode, path) => {
125118
curNode.children.splice(lastIdx, 1); // remove target node
126119
updateStatusUp(parentNodes); // update check status up, from this nodes
127120

128-
return _rootNode;
121+
return { ...rootNode };
129122
};
130123

131124
export const findMaxId = rootNode => {
@@ -141,8 +134,7 @@ export const addNode = (rootNode, path, type = 'file') => {
141134
const id = findMaxId(rootNode) + 1;
142135
const isFile = type === 'file';
143136

144-
const _rootNode = deepClone(rootNode);
145-
let curNode = _rootNode;
137+
let curNode = rootNode;
146138
for (const idx of path) {
147139
curNode = curNode.children[idx];
148140
}
@@ -167,12 +159,11 @@ export const addNode = (rootNode, path, type = 'file') => {
167159
}
168160
}
169161

170-
return _rootNode;
162+
return { ...rootNode };
171163
};
172164

173165
export const toggleOpen = (rootNode, path, isOpen) => {
174-
const _rootNode = deepClone(rootNode);
175-
let curNode = _rootNode;
166+
let curNode = rootNode;
176167
for (const idx of path) {
177168
curNode = curNode.children[idx];
178169
}
@@ -182,7 +173,7 @@ export const toggleOpen = (rootNode, path, isOpen) => {
182173
curNode.isOpen = isOpen;
183174
}
184175

185-
return _rootNode;
176+
return { ...rootNode };
186177
};
187178

188179
export const setAllOpenStatus = (node, isOpen) => {
@@ -216,4 +207,5 @@ export const isValidOpenStatus = node => {
216207
};
217208

218209
// check if the initial custom checked status is valid
210+
// TODO: implement isValidCheckedStatus()
219211
export const isValidCheckedStatus = rootNode => true; /* eslint-disable-line */

‎src/utils/utils.test.js

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import {
55
} from './testData';
66
import {
77
deepClone,
8-
addUniqIds,
8+
initStateWithUniqIds,
99
checkNode,
1010
setAllCheckedStatus,
1111
isValidCheckedStatus,
@@ -19,15 +19,15 @@ import {
1919
addNode,
2020
} from './utils';
2121

22-
describe('addUniqIds', () => {
22+
describe('initStateWithUniqIds', () => {
2323
it('add uniq ids to all nodes', () => {
24-
expect(addUniqIds(testData)).toEqual(testDataWithId);
25-
expect(addUniqIds({})).toEqual({ _id: 0 });
24+
expect(initStateWithUniqIds(testData)).toEqual(testDataWithId);
25+
expect(initStateWithUniqIds({})).toEqual({ _id: 0 });
2626
});
2727
});
2828

2929
describe('setAllCheckedStatus', () => {
30-
const initData = setAllCheckedStatus(addUniqIds(testData), 0);
30+
const initData = setAllCheckedStatus(initStateWithUniqIds(testData), 0);
3131

3232
it('set checked status to 0 for all nodes', () => {
3333
expect(initData).toEqual(initializedTestData);
@@ -93,7 +93,7 @@ describe('getNewCheckStatus', () => {
9393
});
9494

9595
describe('checkNode', () => {
96-
const initData = setAllCheckedStatus(addUniqIds(testData), 0);
96+
const initData = setAllCheckedStatus(initStateWithUniqIds(testData), 0);
9797

9898
describe('when check root node', () => {
9999
it('returns correct state', () => {
@@ -241,8 +241,8 @@ describe('toggleOpen', () => {
241241
],
242242
};
243243

244-
expect(toggleOpen(node, [], true)).toEqual(expected);
245-
expect(toggleOpen(expected, [], false)).toEqual(node);
244+
expect(toggleOpen(deepClone(node), [], true)).toEqual(expected);
245+
expect(toggleOpen(deepClone(expected), [], false)).toEqual(node);
246246
});
247247

248248
it('correctly toggle second layer', () => {
@@ -263,8 +263,8 @@ describe('toggleOpen', () => {
263263
],
264264
};
265265

266-
expect(toggleOpen(node, [2], false)).toEqual(expected);
267-
expect(toggleOpen(expected, [2], true)).toEqual(node);
266+
expect(toggleOpen(deepClone(node), [2], false)).toEqual(expected);
267+
expect(toggleOpen(deepClone(expected), [2], true)).toEqual(node);
268268
});
269269

270270
it('correctly toggle last layer', () => {
@@ -285,8 +285,8 @@ describe('toggleOpen', () => {
285285
],
286286
};
287287

288-
expect(toggleOpen(node, [2, 0], false)).toEqual(expected);
289-
expect(toggleOpen(expected, [2, 0], true)).toEqual(node);
288+
expect(toggleOpen(deepClone(node), [2, 0], false)).toEqual(expected);
289+
expect(toggleOpen(deepClone(expected), [2, 0], true)).toEqual(node);
290290
});
291291
});
292292

@@ -857,8 +857,8 @@ describe('addNode', () => {
857857
],
858858
};
859859

860-
expect(addNode(node, [1], 'file')).toEqual(addFileExpected);
861-
expect(addNode(node, [1], 'folder')).toEqual(addFolderExpected);
860+
expect(addNode(deepClone(node), [1], 'file')).toEqual(addFileExpected);
861+
expect(addNode(deepClone(node), [1], 'folder')).toEqual(addFolderExpected);
862862
});
863863

864864
describe('when parent folder is half-checked', () => {
@@ -938,8 +938,8 @@ describe('addNode', () => {
938938
],
939939
};
940940

941-
expect(addNode(node, [1], 'file')).toEqual(addFileExpected);
942-
expect(addNode(node, [1], 'folder')).toEqual(addFolderExpected);
941+
expect(addNode(deepClone(node), [1], 'file')).toEqual(addFileExpected);
942+
expect(addNode(deepClone(node), [1], 'folder')).toEqual(addFolderExpected);
943943
});
944944

945945
describe('when parent folder is unchecked', () => {
@@ -1019,8 +1019,8 @@ describe('addNode', () => {
10191019
],
10201020
};
10211021

1022-
expect(addNode(node, [1], 'file')).toEqual(addFileExpected);
1023-
expect(addNode(node, [1], 'folder')).toEqual(addFolderExpected);
1022+
expect(addNode(deepClone(node), [1], 'file')).toEqual(addFileExpected);
1023+
expect(addNode(deepClone(node), [1], 'folder')).toEqual(addFolderExpected);
10241024
});
10251025
});
10261026

0 commit comments

Comments
 (0)