Return to Revisions

Good start! I like the idea of rendering these matrices. The code is straightforward and readable and your variable names are generally good.

Remarks:

• Always scope variables with let or const. Never use bare variables like mat=0 without let to scope it to the block. If you have two variables with the same name, or accidentally access a variable when it shouldn't exist, bugs often ensue. Try to tightly scope state as much as possible to reduce complexity.

• The Matrix class doesn't accomplish much, since it consists of a single variable and no functions at the moment. Perhaps soon you'll add operations to it, but for now, a matrix can simply be a variable. Classes are primarily useful when you want to group a bunch of related variables and functions together, and establish a set of rules for accessing and manipulating that state.

  Once you do add a class, you'll probably not want to let the user of the class access the .M property directly, or they could mess up state within the class.

• Avoid counter-based for loops unless necessary. Prefer for ... of loops; they look cleaner and are less prone to off-by-one bugs.

• Use prettier to format your code in a way that reduces the friction for yourself and other JS programmers to read your code.

• Instead of let matrices = [];, since you never reassign this variable, you can use const matrices = [];, best communicating your intent and avoiding an accidental reassignment.

• Since matrices is unrelated to P5 you can move it out of the setup function and simply declare it once, without modifying the array with push.

• textSize(20); is always the same in the middle of the loops, so you can save some CPU cycles moving it outside of the loops.

• Code like text( matrices[mat].M[row][col], 500+20*col, 100+120*mat+30*row); is difficult to read. You can move the arguments out to named variables, value, x and y, then use text(value, x, y). Another trick I like to use for calls like this is to spread it across multiple lines, with comments:

  text(
    matrices[mat].M[row][col],
    500 + 20 * col, // x
    100 + 120 * mat + 30 * row // y
  );
  

• Consider making the application more friendly to various screens with dynamic sizes (not done in my rewrite below, but left as an exercise). You can see my version overflows the screen.

Suggested rewrite:

const matrices = [
  [
    [0, 1, 2],
    [1, 2, 3],
    [2, 3, 4],
  ],
  [
    [1, 2, 3],
    [2, 3, 4],
    [3, 4, 5],
  ],
  [
    [2, 3, 4],
    [3, 4, 5],
    [4, 5, 6],
  ],
];

function setup() {
  createCanvas(windowWidth, windowHeight);
}

function draw() {
  background("black");
  fill("white");
  textSize(20);
  let sum = 0;

  for (const [matIndex, matrix] of matrices.entries()) {
    for (const [rowIndex, row] of matrix.entries()) {
      for (const [colIndex, value] of row.entries()) {
        sum += value;
        const x = 500 + 20 * colIndex;
        const y = 100 + 120 * matIndex + 30 * rowIndex;
        text(value, x, y);
      }
    }
  }

  textSize(30);
  text("Sum=", 500, 500);
  text(sum, 500, 540);
}

<script src="https://cdnjs.cloudflare.com/ajax/libs/p5.js/1.11.0/p5.js"></script>