Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

#Review and alternatives.

Review and alternatives.

##Don't use JSON to compare.

Don't use JSON to compare.

##Redundent code

Redundent code

##Comments

Comments

##Rewrites

Rewrites

#Review and alternatives.

##Don't use JSON to compare.

##Redundent code

##Comments

##Rewrites

Review and alternatives.

Don't use JSON to compare.

Redundent code

Comments

Rewrites

Source Link
Blindman67
  • 22.9k
  • 2
  • 16
  • 40

#Review and alternatives.

  • Use strict mode. This should go without saying, however I add it as a point in reviews when I see bugs or potencial bugs that would be caught easily.
  • Never use a variable that has not been declared. An example in your code is encode and if you were in strict mode you would have found it before you posted the review.
  • Use constants when the variable does not change. In your example you declare result as a variable, and define it as an array. But you never change the variable. It should be declared as a constant. const result = [];
  • Comments are bad. They clutter up the source and are seldom of any help. Only coders will read your code and stating the obvious helps no one, and creating ambiguity via comments is dangerous. See more on comments below
  • Don't clutter your source with redundant code, less is best.
  • Don't repeat. You have the variable len and the only time you use it you subtract 1 from it, repeating computations for no reason. Better to subtract the one when you define the value const lastIdx = array.length;
  • When iterating use for of loops in favour of array methods as they are slightly quicker, can be exited early unlike many array methods, and tend to be more suited to the logic and need of the code.

##Don't use JSON to compare.

  • JSON.stringify is slow and should not be used unless you are saving or transporting data.
  • JSON.stringify will throw on cyclic references, meaning your compare function can throw even though the code state is good.
  • JSON.stringify converts undefined array items to null, meaning that is you compare [undefined] and [null] they will be equal.
  • JSON.stringify can be customised and thus there is no guarantee that it accurately reflects the data content.

Run length encoding is about the equality of items. A more detailed comparison will make for a better encoder. See rewrites for two examples.

##Redundent code

You had

 arrayEquals = (...arr) => {
 return arr[0].map(x => {
 return JSON.stringify(x);
 }).every((x, _, a) => {
 return x === a[0];
 }); 
 };

The returns are not needed

const arrayEquals = (...arr) => arr[0].map(x => JSON.stringify(x)).every((x, _, a) => x === a[0]);

or

const arrayEquals = (...arr) => {
 return arr[0]
 .map(x => JSON.stringify(x))
 .every((x, _, a) => x === a[0]);
};

or

const arrayEquals = (...arr) => {
 return arr[0].map(x => JSON.stringify(x)).every((x, _, a) => x === a[0]);
};

##Comments

Comments are rare and only when the immediate code (within eye shoot) is lacking needed abstract context, or has side effects beyond the scope of the current context.

Comment are not checked or vetted, and can remain in source even when the code that is commented has changed.

This mean it is easy for comments to lie, and an old comment can make your intent ambiguous. This is very dangerous when you later make changes, what do you believe the comment or the code.

If you feel some code needs a comment, before adding a comment ask yourself "Can I change the naming to make the code more readable?" Most of the time the answer will be yes.

Example of a poor comment in your code

 // if current value differs from last
 if (i !== len - 1) {

The comment is confusing, i is an index, \\ current val infers a data value, and \\ last is obscure so anyone reading the comment will still have to find len to workout what it is anyway

You could have commented

 if (i !== len - 1) { // is not last index
 

or better let the code say it all

 if (i !== arrayLen - 1) {

Personally I use idx for callback indexes, rather than i and resurve i, j, k for use in loops only.

Example of stating the obvious in your code

count = 0; // reset the count
 

Does that code really need clarification?

##Rewrites

The following rewrites simplify the encoding, while making the comparison more robust.

There are two versions. The first is cyclic unsafe and will throw a range error if the array contains references to itself. The second uses a depth limit to stop endless recursion marking such items as equal.

Cyclic unsafe version

const runLengthEncode = array => {
 "use strict";
 const result = [];
 var item, i = 0; 
 
 const isArr = arr => Array.isArray(arr);
 const compare = (item1, item2) => {
 if (isArr(item1) && isArr(item2)) {
 if (item1.length === item2.length) {
 for (let i = 0; i < item1.length; i++) {
 if (!compare(item1[i], item2[i])) { return false }
 }
 } else { return false }
 }else if(item1 !== item2) { return false }
 return true;
 }
 
 while (i < array.length) {
 if (item && compare(item[1], array[i])) { item[0] += 1 }
 else { result.push(item = [1, array[i]]) }
 i += 1;
 }
 
 return result;
}
// From here down is support code not part of answer
const array2Str = arr => "[" + arr.map(d => Array.isArray(d) ? array2Str(d) : d) + "]";
const log= data => { const div = document.createElement("div"); div.textContent = data; logEl.appendChild(div)}
const encode = d => {
 log("-------------------");
 log("Data " + array2Str(d));
 log("encode as " + array2Str(runLengthEncode(d)));
 
}
log(encode ([1, 1, 2, 2, 3, 3, 3, 3, 4]));
log(encode ([4, 4, 4, 4, 4]));
log(encode ([[1, 2, 3], [4, 5, 6], [7, 8, 9], [10, 11, 12]]));
log(encode ([[1, 2, 3], [1, 2, 3], [4, 5, 6]]));
log(encode ([[0], [0], [1], [2, 3], [4, 5, 6], [], []]));
<code id="logEl"></code>

Example 2 cyclic safe.

"use strict";
const runLengthEncode = (array, maxDepth = 4) => {
 const result = [];
 var item, i = 0, depth = 0; 
 const isArr = arr => Array.isArray(arr);
 const compare = (item1, item2) => {
 var res = true;
 if (depth++ < maxDepth) {
 if (isArr(item1) && isArr(item2)) {
 if (item1.length === item2.length) {
 for (let i = 0; i < item1.length && res; i++) {
 if (!compare(item1[i], item2[i])) { res = false }
 }
 } else { res = false }
 }else if(item1 !== item2) { res = false }
 }
 depth --;
 return res;
 }
 const compareItems = (item1, item2) => (depth = 0, compare(item1, item2));
 while(i < array.length){
 if (item && compareItems(item[1], array[i])) { item[0] += 1 }
 else { result.push(item = [1, array[i]]) }
 i += 1;
 }
 return result;
}
// From here down is support code not part of answer
const array2Str = arr => "[" + arr.map(d => Array.isArray(d) ? array2Str(d) : d) + "]";
const log= data => { const div = document.createElement("div"); div.textContent = data; logEl.appendChild(div)}
const encode = d => {
 log("-------------------");
 log("Data " + array2Str(d));
 log("encode as " + array2Str(runLengthEncode(d)));
 
}
const encodeC = d => {
 log("--Cyclic test (arrays not formated and Cyclic arrays return empty strings) ----------");
 log("Data " + d);
 log("encode as " + runLengthEncode(d));
}
log("Test cyclic array");
var a = [1,a];
a.push(a);
log(encodeC ([a, a, a]));
log("-------------------");
log("Standard tests");
log(encode ([1, 1, 2, 2, 3, 3, 3, 3, 4]));
log(encode ([4, 4, 4, 4, 4]));
log(encode ([[1, 2, 3], [4, 5, 6], [7, 8, 9], [10, 11, 12]]));
log(encode ([[1, 2, 3], [1, 2, 3], [4, 5, 6]]));
log(encode ([[0], [0], [1], [2, 3], [4, 5, 6], [], []]));
<code id="logEl"></code>

default

AltStyle によって変換されたページ (->オリジナル) /