From 1955e7b263a6eb9cf9fa8d687e3a90ef7c585d94 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 13 Oct 2021 17:00:50 -0400 Subject: [PATCH] Changeset: Replace output params with return values This improves readability and reduces the chances of introducing a bug. --- src/node/utils/padDiff.js | 6 +-- src/static/js/Changeset.js | 100 +++++++++++++------------------------ 2 files changed, 39 insertions(+), 67 deletions(-) diff --git a/src/node/utils/padDiff.js b/src/node/utils/padDiff.js index 193f24458..fa612c7c2 100644 --- a/src/node/utils/padDiff.js +++ b/src/node/utils/padDiff.js @@ -273,7 +273,7 @@ PadDiff.prototype._createDeletionChangeset = function (cs, startAText, apool) { let curChar = 0; let curLineOpIter = null; let curLineOpIterLine; - const curLineNextOp = Changeset.newOp('+'); + let curLineNextOp = Changeset.newOp('+'); const unpacked = Changeset.unpack(cs); const csIter = Changeset.opIterator(unpacked.ops); @@ -287,7 +287,7 @@ PadDiff.prototype._createDeletionChangeset = function (cs, startAText, apool) { let indexIntoLine = 0; let done = false; while (!done) { - curLineOpIter.next(curLineNextOp); + curLineNextOp = curLineOpIter.next(); if (indexIntoLine + curLineNextOp.chars >= curChar) { curLineNextOp.chars -= (curChar - indexIntoLine); done = true; @@ -307,7 +307,7 @@ PadDiff.prototype._createDeletionChangeset = function (cs, startAText, apool) { } if (!curLineNextOp.chars) { - curLineOpIter.next(curLineNextOp); + curLineNextOp = curLineOpIter.next(); } const charsToUse = Math.min(numChars, curLineNextOp.chars); diff --git a/src/static/js/Changeset.js b/src/static/js/Changeset.js index 4d3c30db5..ead0e97a9 100644 --- a/src/static/js/Changeset.js +++ b/src/static/js/Changeset.js @@ -947,7 +947,7 @@ const textLinesMutator = (lines) => { * @param {string} in2 - second Op string * @param {Function} func - Callback that applies an operation to another operation. Will be called * multiple times depending on the number of operations in `in1` and `in2`. `func` has signature - * `f(op1, op2, opOut)`: + * `opOut = f(op1, op2)`: * - `op1` is the current operation from `in1`. `func` is expected to mutate `op1` to * partially or fully consume it, and MUST set `op1.opcode` to the empty string once `op1` * is fully consumed. If `op1` is not fully consumed, `func` will be called again with the @@ -956,9 +956,9 @@ const textLinesMutator = (lines) => { * the empty string. * - `op2` is the current operation from `in2`, to apply to `op1`. Has the same consumption * and advancement semantics as `op1`. - * - `opOut` MUST be mutated to reflect the result of applying `op2` (before consumption) to - * `op1` (before consumption). If there is no result (perhaps `op1` and `op2` cancelled each - * other out), `opOut.opcode` MUST be set to the empty string. + * - `opOut` is the result of applying `op2` (before consumption) to `op1` (before + * consumption). If there is no result (perhaps `op1` and `op2` cancelled each other out), + * either `opOut` must be nullish or `opOut.opcode` must be the empty string. * @returns {string} the integrated changeset */ const applyZip = (in1, in2, func) => { @@ -967,15 +967,11 @@ const applyZip = (in1, in2, func) => { const assem = exports.smartOpAssembler(); const op1 = exports.newOp(); const op2 = exports.newOp(); - const opOut = exports.newOp(); while (op1.opcode || iter1.hasNext() || op2.opcode || iter2.hasNext()) { if ((!op1.opcode) && iter1.hasNext()) iter1.next(op1); if ((!op2.opcode) && iter2.hasNext()) iter2.next(op2); - func(op1, op2, opOut); - if (opOut.opcode) { - assem.append(opOut); - opOut.opcode = ''; - } + const opOut = func(op1, op2); + if (opOut && opOut.opcode) assem.append(opOut); } assem.endDocument(); return assem.toString(); @@ -1171,11 +1167,11 @@ exports.composeAttributes = (att1, att2, resultIsMutation, pool) => { * @param {Op} attOp - The op from the sequence that is being operated on, either an attribution * string or the earlier of two exportss being composed. * @param {Op} csOp - - * @param {Op} opOut - Mutated to hold the result of applying `csOp` to `attOp`. * @param {AttributePool} pool - Can be null if definitely not needed. + * @returns {Op} The result of applying `csOp` to `attOp`. */ -const slicerZipperFunc = (attOp, csOp, opOut, pool) => { - clearOp(opOut); +const slicerZipperFunc = (attOp, csOp, pool) => { + const opOut = exports.newOp(); if (attOp.opcode === '-') { copyOp(attOp, opOut); attOp.opcode = ''; @@ -1263,6 +1259,7 @@ const slicerZipperFunc = (attOp, csOp, opOut, pool) => { } } } + return opOut; }; /** @@ -1275,9 +1272,7 @@ const slicerZipperFunc = (attOp, csOp, opOut, pool) => { */ exports.applyToAttribution = (cs, astr, pool) => { const unpacked = exports.unpack(cs); - - return applyZip(astr, unpacked.ops, - (op1, op2, opOut) => slicerZipperFunc(op1, op2, opOut, pool)); + return applyZip(astr, unpacked.ops, (op1, op2) => slicerZipperFunc(op1, op2, pool)); }; exports.mutateAttributionLines = (cs, lines, pool) => { @@ -1293,16 +1288,13 @@ exports.mutateAttributionLines = (cs, lines, pool) => { const isNextMutOp = () => (lineIter && lineIter.hasNext()) || mut.hasMore(); - const nextMutOp = (destOp) => { + const nextMutOp = () => { if ((!(lineIter && lineIter.hasNext())) && mut.hasMore()) { const line = mut.removeLines(1); lineIter = exports.opIterator(line); } - if (lineIter && lineIter.hasNext()) { - lineIter.next(destOp); - } else { - destOp.opcode = ''; - } + if (!lineIter || !lineIter.hasNext()) return exports.newOp(); + return lineIter.next(); }; let lineAssem = null; @@ -1318,13 +1310,10 @@ exports.mutateAttributionLines = (cs, lines, pool) => { lineAssem = null; }; - const csOp = exports.newOp(); - const attOp = exports.newOp(); - const opOut = exports.newOp(); + let csOp = exports.newOp(); + let attOp = exports.newOp(); while (csOp.opcode || csIter.hasNext() || attOp.opcode || isNextMutOp()) { - if ((!csOp.opcode) && csIter.hasNext()) { - csIter.next(csOp); - } + if (!csOp.opcode && csIter.hasNext()) csOp = csIter.next(); if ((!csOp.opcode) && (!attOp.opcode) && (!lineAssem) && (!(lineIter && lineIter.hasNext()))) { break; // done } else if (csOp.opcode === '=' && csOp.lines > 0 && (!csOp.attribs) && @@ -1333,29 +1322,22 @@ exports.mutateAttributionLines = (cs, lines, pool) => { mut.skipLines(csOp.lines); csOp.opcode = ''; } else if (csOp.opcode === '+') { + const opOut = copyOp(csOp); if (csOp.lines > 1) { const firstLineLen = csBank.indexOf('\n', csBankIndex) + 1 - csBankIndex; - copyOp(csOp, opOut); csOp.chars -= firstLineLen; csOp.lines--; opOut.lines = 1; opOut.chars = firstLineLen; } else { - copyOp(csOp, opOut); csOp.opcode = ''; } outputMutOp(opOut); csBankIndex += opOut.chars; - opOut.opcode = ''; } else { - if ((!attOp.opcode) && isNextMutOp()) { - nextMutOp(attOp); - } - slicerZipperFunc(attOp, csOp, opOut, pool); - if (opOut.opcode) { - outputMutOp(opOut); - opOut.opcode = ''; - } + if (!attOp.opcode && isNextMutOp()) attOp = nextMutOp(); + const opOut = slicerZipperFunc(attOp, csOp, pool); + if (opOut.opcode) outputMutOp(opOut); } } @@ -1446,13 +1428,13 @@ exports.compose = (cs1, cs2, pool) => { const bankIter2 = exports.stringIterator(unpacked2.charBank); const bankAssem = exports.stringAssembler(); - const newOps = applyZip(unpacked1.ops, unpacked2.ops, (op1, op2, opOut) => { + const newOps = applyZip(unpacked1.ops, unpacked2.ops, (op1, op2) => { const op1code = op1.opcode; const op2code = op2.opcode; if (op1code === '+' && op2code === '-') { bankIter1.skip(Math.min(op1.chars, op2.chars)); } - slicerZipperFunc(op1, op2, opOut, pool); + const opOut = slicerZipperFunc(op1, op2, pool); if (opOut.opcode === '+') { if (op2code === '+') { bankAssem.append(bankIter2.take(opOut.chars)); @@ -1460,6 +1442,7 @@ exports.compose = (cs1, cs2, pool) => { bankAssem.append(bankIter1.take(opOut.chars)); } } + return opOut; }); return exports.pack(len1, len3, newOps, bankAssem.toString()); @@ -1788,9 +1771,8 @@ exports.copyAText = (atext1, atext2) => { exports.appendATextToAssembler = (atext, assem) => { // intentionally skips last newline char of atext const iter = exports.opIterator(atext.attribs); - const op = exports.newOp(); while (iter.hasNext()) { - iter.next(op); + const op = iter.next(); if (!iter.hasNext()) { // last op, exclude final newline if (op.lines <= 1) { @@ -1994,25 +1976,19 @@ exports.makeAttribsString = (opcode, attribs, pool) => { exports.subattribution = (astr, start, optEnd) => { const iter = exports.opIterator(astr); const assem = exports.smartOpAssembler(); - const attOp = exports.newOp(); + let attOp = exports.newOp(); const csOp = exports.newOp(); - const opOut = exports.newOp(); const doCsOp = () => { if (!csOp.chars) return; while (csOp.opcode && (attOp.opcode || iter.hasNext())) { - if (!attOp.opcode) iter.next(attOp); - + if (!attOp.opcode) attOp = iter.next(); if (csOp.opcode && attOp.opcode && csOp.chars >= attOp.chars && attOp.lines > 0 && csOp.lines <= 0) { csOp.lines++; } - - slicerZipperFunc(attOp, csOp, opOut, null); - if (opOut.opcode) { - assem.append(opOut); - opOut.opcode = ''; - } + const opOut = slicerZipperFunc(attOp, csOp, null); + if (opOut.opcode) assem.append(opOut); } }; @@ -2025,10 +2001,7 @@ exports.subattribution = (astr, start, optEnd) => { if (attOp.opcode) { assem.append(attOp); } - while (iter.hasNext()) { - iter.next(attOp); - assem.append(attOp); - } + while (iter.hasNext()) assem.append(iter.next()); } else { csOp.opcode = '='; csOp.chars = optEnd - start; @@ -2067,7 +2040,7 @@ exports.inverse = (cs, lines, alines, pool) => { let curChar = 0; let curLineOpIter = null; let curLineOpIterLine; - const curLineNextOp = exports.newOp('+'); + let curLineNextOp = exports.newOp('+'); const unpacked = exports.unpack(cs); const csIter = exports.opIterator(unpacked.ops); @@ -2081,7 +2054,7 @@ exports.inverse = (cs, lines, alines, pool) => { let indexIntoLine = 0; let done = false; while (!done && curLineOpIter.hasNext()) { - curLineOpIter.next(curLineNextOp); + curLineNextOp = curLineOpIter.next(); if (indexIntoLine + curLineNextOp.chars >= curChar) { curLineNextOp.chars -= (curChar - indexIntoLine); done = true; @@ -2099,9 +2072,7 @@ exports.inverse = (cs, lines, alines, pool) => { curLineNextOp.chars = 0; curLineOpIter = exports.opIterator(alinesGet(curLine)); } - if (!curLineNextOp.chars) { - curLineOpIter.next(curLineNextOp); - } + if (!curLineNextOp.chars) curLineNextOp = curLineOpIter.next(); const charsToUse = Math.min(numChars, curLineNextOp.chars); func(charsToUse, curLineNextOp.attribs, charsToUse === curLineNextOp.chars && curLineNextOp.lines > 0); @@ -2217,8 +2188,8 @@ exports.follow = (cs1, cs2, reverseInsertOrder, pool) => { const hasInsertFirst = exports.attributeTester(['insertorder', 'first'], pool); - const newOps = applyZip(unpacked1.ops, unpacked2.ops, (op1, op2, opOut) => { - clearOp(opOut); + const newOps = applyZip(unpacked1.ops, unpacked2.ops, (op1, op2) => { + const opOut = exports.newOp(); if (op1.opcode === '+' || op2.opcode === '+') { let whichToDo; if (op2.opcode !== '+') { @@ -2336,6 +2307,7 @@ exports.follow = (cs1, cs2, reverseInsertOrder, pool) => { newLen += opOut.chars; break; } + return opOut; }); newLen += oldLen - oldPos;