Sniberb•14mo ago

snowberb – 10-49 Oct 6

Focusing only in code readability, maintainability and quality, which piece of code would you choose? (Both do the same and work) The objective is to calculate the bounds of a point chart Piece 1:

  const bounds = { maxX: 0, minX: 0, maxY: 0, minY: 0 };
  for (const index in dataArray) {
    const { return: returnData, volatility } = dataArray[index];

    if (volatility > bounds.maxX) bounds.maxX = volatility;
    if (volatility < bounds.minX) bounds.minX = volatility;
    if (returnData > bounds.maxY) bounds.maxY = returnData;
    if (returnData < bounds.minY) bounds.minY = returnData;
  }

  const bounds = { maxX: 0, minX: 0, maxY: 0, minY: 0 };
  for (const index in dataArray) {
    const { return: returnData, volatility } = dataArray[index];

    if (volatility > bounds.maxX) bounds.maxX = volatility;
    if (volatility < bounds.minX) bounds.minX = volatility;
    if (returnData > bounds.maxY) bounds.maxY = returnData;
    if (returnData < bounds.minY) bounds.minY = returnData;
  }

Piece 2:

 export const findMinMax = (arr: { low: number; high: number }[]) => {
  if (!arr.length) return [0, 0];

  let min = arr[0].low,
    max = arr[0].high;

  for (let i = 1, len = arr.length; i < len; i++) {
    const v = arr[i].low;

    min = v < min ? v : min;
    max = v > max ? v : max;
  }

  return [min, max];
};

  const ranges_1 = findMinMax(
    dataArray.map(({ return: returnData, volatility }) => ({
      low: returnData || 0,
      high: volatility || 0,
    })) || [],
  );
  const ranges_2 = findMinMax(
    dataArray.map(({ return: returnData, volatility }) => ({
      low: volatility || 0,
      high: returnData || 0,
    })) || [],
  );
  const bounds = { maxX: ranges_2[1], minX: ranges_1[0], maxY: ranges_1[1], minY: ranges_2[0] };

 export const findMinMax = (arr: { low: number; high: number }[]) => {
  if (!arr.length) return [0, 0];

  let min = arr[0].low,
    max = arr[0].high;

  for (let i = 1, len = arr.length; i < len; i++) {
    const v = arr[i].low;

    min = v < min ? v : min;
    max = v > max ? v : max;
  }

  return [min, max];
};

  const ranges_1 = findMinMax(
    dataArray.map(({ return: returnData, volatility }) => ({
      low: returnData || 0,
      high: volatility || 0,
    })) || [],
  );
  const ranges_2 = findMinMax(
    dataArray.map(({ return: returnData, volatility }) => ({
      low: volatility || 0,
      high: returnData || 0,
    })) || [],
  );
  const bounds = { maxX: ranges_2[1], minX: ranges_1[0], maxY: ranges_1[1], minY: ranges_2[0] };

6 Replies

lebouwski•14mo ago

what about

const data = [
  { x: 4, y: 8 },
  // etc
];

const xAxis = data.map((point) => point.x);
const yAxis = data.map((point) => point.y);

const bounds = {
  maxX: Math.max(...xAxis),
  minX: Math.min(...xAxis),
  maxY: Math.max(...yAxis),
  minY: Math.min(...yAxis),
};

const data = [
  { x: 4, y: 8 },
  // etc
];

const xAxis = data.map((point) => point.x);
const yAxis = data.map((point) => point.y);

const bounds = {
  maxX: Math.max(...xAxis),
  minX: Math.min(...xAxis),
  maxY: Math.max(...yAxis),
  minY: Math.min(...yAxis),
};

SniberbOP•14mo ago

yeah that's way better I often overcomplicate and I dont know how to improve on that

ScriptyChris•14mo ago

@⛄Snowberb⛄ oh, second example is IMO way more difficult to understand and it's longer 😄 Evert gave nice code with Math.min and Math.max instead of conditions Although I think that yours first example, with replaced ifs to mentioned Math methods would be better than Evert's example, because there would only be 1 loop 🤔 And you should avoid using for..in for arrays - use for..of instead and you can destructure stuff directly at the iterator declaration

lebouwski•14mo ago

for single loop you could do

const bounds = data.reduce(
  ({ maxX, minX, maxY, minY }, { x, y }) => ({
    maxX: Math.max(maxX, x),
    minX: Math.min(minX, x),
    maxY: Math.max(maxY, y),
    minY: Math.min(minY, y),
  }),
  {
    maxX: Number.NEGATIVE_INFINITY,
    minX: Number.POSITIVE_INFINITY,
    maxY: Number.NEGATIVE_INFINITY,
    minY: Number.POSITIVE_INFINITY,
  }
);

const bounds = data.reduce(
  ({ maxX, minX, maxY, minY }, { x, y }) => ({
    maxX: Math.max(maxX, x),
    minX: Math.min(minX, x),
    maxY: Math.max(maxY, y),
    minY: Math.min(minY, y),
  }),
  {
    maxX: Number.NEGATIVE_INFINITY,
    minX: Number.POSITIVE_INFINITY,
    maxY: Number.NEGATIVE_INFINITY,
    minY: Number.POSITIVE_INFINITY,
  }
);

if it's actually faster you'd have to profile

SniberbOP•14mo ago

Faster or slower doesnt matter here I know I gave this without context, but taking into account the findMinMax function already exists and is used in other places of the project I do think it's way more readable than the first example, and what is most important, it's rehusable and maintainable but yeah with the min max math functions is the best way of doing this the shortest and the clearest

reactibot•14mo ago

This thread hasn’t had any activity in 36 hours, so it’s now locked. Threads are closed automatically after 36 hours. If you have a followup question, you may want to reply to this thread so other members know they're related. https://discord.com/channels/102860784329052160/565213527673929729/1159804662548013056