Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

#Keep it simple

Keep it simple

Your algorithm is too complex, with too many iterations and uses too much memory. It can be simplified using only a single outer iteration to get a row, and the row's max height. Then an inner loop to set the new height values. (see rewrite)

Some points

  • document.querySelectorAll returns an iterable object. There is no need to convert it to an array. You can use for of or any of the iteration features on it. EG document.querySelectorAll("query string").forEach( and [...document.querySelectorAll("query string")]

  • You only need to use .getAttribute when the attribute is not part of the DOM. const height = element.height is more concise.

  • You can use the Math function max to get the maximum of 2 or more values at a time. maxHeight = Math.max(maxHeight, element.height)

  • Don't pollute the global scope. Use a self invoking function to encapsulate your variables. ;(()=>{ /* your code in here */ })();

##Rewrite

Rewrite

;(() => {
 "use strict";
 const itemsPerRow = 3;
 const query = ".summary-image-wrapper";
 const minHeight = 100; // Maybe a min height just in case
 const row = [];
 var maxHeight = minHeight;
 document.querySelectorAll(query)
 .forEach((imgWrap, i, imgWrappers) => {
 row.push(imgWrap);
 maxHeight = Math.max(imgWrap.firstElementChild.height , maxHeight);
 if (row.length === itemsPerRow || i === imgWrappers.length - 1) {
 row.forEach(imgWrap => imgWrap.style.height = `${maxHeight}px`);
 maxHeight = minHeight;
 row.length = 0;
 }
 });
})();

#Keep it simple

Your algorithm is too complex, with too many iterations and uses too much memory. It can be simplified using only a single outer iteration to get a row, and the row's max height. Then an inner loop to set the new height values. (see rewrite)

Some points

  • document.querySelectorAll returns an iterable object. There is no need to convert it to an array. You can use for of or any of the iteration features on it. EG document.querySelectorAll("query string").forEach( and [...document.querySelectorAll("query string")]

  • You only need to use .getAttribute when the attribute is not part of the DOM. const height = element.height is more concise.

  • You can use the Math function max to get the maximum of 2 or more values at a time. maxHeight = Math.max(maxHeight, element.height)

  • Don't pollute the global scope. Use a self invoking function to encapsulate your variables. ;(()=>{ /* your code in here */ })();

##Rewrite

;(() => {
 "use strict";
 const itemsPerRow = 3;
 const query = ".summary-image-wrapper";
 const minHeight = 100; // Maybe a min height just in case
 const row = [];
 var maxHeight = minHeight;
 document.querySelectorAll(query)
 .forEach((imgWrap, i, imgWrappers) => {
 row.push(imgWrap);
 maxHeight = Math.max(imgWrap.firstElementChild.height , maxHeight);
 if (row.length === itemsPerRow || i === imgWrappers.length - 1) {
 row.forEach(imgWrap => imgWrap.style.height = `${maxHeight}px`);
 maxHeight = minHeight;
 row.length = 0;
 }
 });
})();

Keep it simple

Your algorithm is too complex, with too many iterations and uses too much memory. It can be simplified using only a single outer iteration to get a row, and the row's max height. Then an inner loop to set the new height values. (see rewrite)

Some points

  • document.querySelectorAll returns an iterable object. There is no need to convert it to an array. You can use for of or any of the iteration features on it. EG document.querySelectorAll("query string").forEach( and [...document.querySelectorAll("query string")]

  • You only need to use .getAttribute when the attribute is not part of the DOM. const height = element.height is more concise.

  • You can use the Math function max to get the maximum of 2 or more values at a time. maxHeight = Math.max(maxHeight, element.height)

  • Don't pollute the global scope. Use a self invoking function to encapsulate your variables. ;(()=>{ /* your code in here */ })();

Rewrite

;(() => {
 "use strict";
 const itemsPerRow = 3;
 const query = ".summary-image-wrapper";
 const minHeight = 100; // Maybe a min height just in case
 const row = [];
 var maxHeight = minHeight;
 document.querySelectorAll(query)
 .forEach((imgWrap, i, imgWrappers) => {
 row.push(imgWrap);
 maxHeight = Math.max(imgWrap.firstElementChild.height , maxHeight);
 if (row.length === itemsPerRow || i === imgWrappers.length - 1) {
 row.forEach(imgWrap => imgWrap.style.height = `${maxHeight}px`);
 maxHeight = minHeight;
 row.length = 0;
 }
 });
})();
added 153 characters in body
Source Link
Blindman67
  • 22.8k
  • 2
  • 16
  • 40

#Keep it simple

Your algorithm is too complex, with too many iterations and uses too much memory. It can be simplified using only a single outer iteration to get a row, and the row's max height. Then an inner loop to set the new height values. (see rewrite)

Some points

  • document.querySelectorAll returns an iterableiterable object. There is no need to convert it to an array. You can use for of or any of the array iteration functionsfeatures on it. EG document.querySelectorAll("query string").forEach( and [...document.querySelectorAll("query string")]

  • You only need to use .getAttribute when the attribute is not part of the DOM. const height = element.height is more concise.

  • You can use the Math function max to get the maximum of 2 or more values at a time. maxHeight = Math.max(maxHeight, element.height)

  • Don't pollute the global scope. Use a self invoking function to encapsulate your variables. ;(()=>{ /* your code in here */ })();

##Rewrite

;(() => {
 "use strict";
 const itemsPerRow = 3;
 const query = ".summary-image-wrapper";
 const minHeight = 100; // Maybe a min height just in case
 const row = [];
 var maxHeight = minHeight;
 document.querySelectorAll(query)
 .forEach((imgWrap, i, imgWrappers) => {
 row.push(imgWrap);
 maxHeight = Math.max(imgWrap.firstElementChild.height , maxHeight);
 if (row.length === itemsPerRow || i === imgWrappers.length - 1) {
 row.forEach(imgWrap => imgWrap.style.height = `${maxHeight}px`);
 maxHeight = minHeight;
 row.length = 0;
 }
 });
})();

#Keep it simple

Your algorithm is too complex, with too many iterations and uses too much memory. It can be simplified using only a single outer iteration to get a row, and the row's max height. Then an inner loop to set the new height values. (see rewrite)

Some points

  • document.querySelectorAll returns an iterable object. There is no need to convert it to an array. You can use for of or any of the array iteration functions on it. document.querySelectorAll("query string").forEach

  • You only need to use .getAttribute when the attribute is not part of the DOM. const height = element.height is more concise.

  • You can use the Math function max to get the maximum of 2 or more values at a time. maxHeight = Math.max(maxHeight, element.height)

  • Don't pollute the global scope. Use a self invoking function to encapsulate your variables. ;(()=>{ /* your code in here */ })();

##Rewrite

;(() => {
 "use strict";
 const itemsPerRow = 3;
 const query = ".summary-image-wrapper";
 const minHeight = 100; // Maybe a min height just in case
 const row = [];
 var maxHeight = minHeight;
 document.querySelectorAll(query)
 .forEach((imgWrap, i, imgWrappers) => {
 row.push(imgWrap);
 maxHeight = Math.max(imgWrap.firstElementChild.height , maxHeight);
 if (row.length === itemsPerRow || i === imgWrappers.length - 1) {
 row.forEach(imgWrap => imgWrap.style.height = `${maxHeight}px`);
 maxHeight = minHeight;
 row.length = 0;
 }
 });
})();

#Keep it simple

Your algorithm is too complex, with too many iterations and uses too much memory. It can be simplified using only a single outer iteration to get a row, and the row's max height. Then an inner loop to set the new height values. (see rewrite)

Some points

  • document.querySelectorAll returns an iterable object. There is no need to convert it to an array. You can use for of or any of the iteration features on it. EG document.querySelectorAll("query string").forEach( and [...document.querySelectorAll("query string")]

  • You only need to use .getAttribute when the attribute is not part of the DOM. const height = element.height is more concise.

  • You can use the Math function max to get the maximum of 2 or more values at a time. maxHeight = Math.max(maxHeight, element.height)

  • Don't pollute the global scope. Use a self invoking function to encapsulate your variables. ;(()=>{ /* your code in here */ })();

##Rewrite

;(() => {
 "use strict";
 const itemsPerRow = 3;
 const query = ".summary-image-wrapper";
 const minHeight = 100; // Maybe a min height just in case
 const row = [];
 var maxHeight = minHeight;
 document.querySelectorAll(query)
 .forEach((imgWrap, i, imgWrappers) => {
 row.push(imgWrap);
 maxHeight = Math.max(imgWrap.firstElementChild.height , maxHeight);
 if (row.length === itemsPerRow || i === imgWrappers.length - 1) {
 row.forEach(imgWrap => imgWrap.style.height = `${maxHeight}px`);
 maxHeight = minHeight;
 row.length = 0;
 }
 });
})();
added 2 characters in body
Source Link
Blindman67
  • 22.8k
  • 2
  • 16
  • 40

#Keep it simple

Your algorithm is too complex, with too many iterations and uses too much memory. It can be simplified using only a single outer iteration to get a row, and the row's max height. Then an inner loop to set the new height values. (see rewrite)

Some points

  • document.querySelectorAll returns an iterable object. TheThere is no need to convert it to an array. You can use for of or any of the array iteration functions on it. document.querySelectorAll("query string").forEach

  • You only need to use .getAttribute when the attribute is not part of the DOM. const height = element.height is more concise.

  • You can use the Math function max to get the maximum of 2 or more values at a time. maxHeight = Math.max(maxHeight, element.height)

  • Don't pollute the global scope. Use a self invoking function to encapsulate your variables. ;(()=>{ /* your code in here */ })();

##Rewrite

;(() => {
 "use strict";
 const itemsPerRow = 3;
 const query = ".summary-image-wrapper";
 const minHeight = 100; // Maybe a min height just in case
 const row = [];
 var maxHeight = minHeight;
 document.querySelectorAll(query)
 .forEach((imgWrap, i, imgWrappers) => {
 row.push(imgWrap);
 maxHeight = Math.max(imgWrap.firstElementChild.height , maxHeight);
 if (row.length === itemsPerRow || i === imgWrappers.length - 1) {
 row.forEach(imgWrap => imgWrap.style.height = `${maxHeight}px`);
 maxHeight = minHeight;
 row.length = 0;
 }
 });
})();

#Keep it simple

Your algorithm is too complex, with too many iterations and uses too much memory. It can be simplified using only a single outer iteration to get a row, and the row's max height. Then an inner loop to set the new height values. (see rewrite)

Some points

  • document.querySelectorAll returns an iterable object. The is no need to convert it to an array. You can use for of or any of the array iteration functions on it. document.querySelectorAll("query string").forEach

  • You only need to use .getAttribute when the attribute is not part of the DOM. const height = element.height is more concise.

  • You can use the Math function max to get the maximum of 2 or more values at a time. maxHeight = Math.max(maxHeight, element.height)

  • Don't pollute the global scope. Use a self invoking function to encapsulate your variables. ;(()=>{ /* your code in here */ })();

##Rewrite

;(() => {
 "use strict";
 const itemsPerRow = 3;
 const query = ".summary-image-wrapper";
 const minHeight = 100; // Maybe a min height just in case
 const row = [];
 var maxHeight = minHeight;
 document.querySelectorAll(query)
 .forEach((imgWrap, i, imgWrappers) => {
 row.push(imgWrap);
 maxHeight = Math.max(imgWrap.firstElementChild.height , maxHeight);
 if (row.length === itemsPerRow || i === imgWrappers.length - 1) {
 row.forEach(imgWrap => imgWrap.style.height = `${maxHeight}px`);
 maxHeight = minHeight;
 row.length = 0;
 }
 });
})();

#Keep it simple

Your algorithm is too complex, with too many iterations and uses too much memory. It can be simplified using only a single outer iteration to get a row, and the row's max height. Then an inner loop to set the new height values. (see rewrite)

Some points

  • document.querySelectorAll returns an iterable object. There is no need to convert it to an array. You can use for of or any of the array iteration functions on it. document.querySelectorAll("query string").forEach

  • You only need to use .getAttribute when the attribute is not part of the DOM. const height = element.height is more concise.

  • You can use the Math function max to get the maximum of 2 or more values at a time. maxHeight = Math.max(maxHeight, element.height)

  • Don't pollute the global scope. Use a self invoking function to encapsulate your variables. ;(()=>{ /* your code in here */ })();

##Rewrite

;(() => {
 "use strict";
 const itemsPerRow = 3;
 const query = ".summary-image-wrapper";
 const minHeight = 100; // Maybe a min height just in case
 const row = [];
 var maxHeight = minHeight;
 document.querySelectorAll(query)
 .forEach((imgWrap, i, imgWrappers) => {
 row.push(imgWrap);
 maxHeight = Math.max(imgWrap.firstElementChild.height , maxHeight);
 if (row.length === itemsPerRow || i === imgWrappers.length - 1) {
 row.forEach(imgWrap => imgWrap.style.height = `${maxHeight}px`);
 maxHeight = minHeight;
 row.length = 0;
 }
 });
})();
Source Link
Blindman67
  • 22.8k
  • 2
  • 16
  • 40
Loading
default

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