5
\$\begingroup\$

I made a little drawing program in OOP.

The game consists of a canvas which is basically a 2d array where the user can draw various shapes on it. There can be multiple shapes overlapped on top of each other at the same spot. The shape that was drawn the last would appear on the top of the canvas and that is what gets printed out when we print the canvas - only those shapes at the top would be printed out.

Here I have two classes Rectangle and Canvas


class Rectangle {
  constructor(positions) {
    this.positions = positions
    this.character = Symbol()
  }

  move(dx, dy) {
    for (const position of this.positions) {
      const nx = position[0] + dx
      const ny = position[1] + dy
      position[0] = nx
      position[1] = ny
    }
  }
}

class Canvas {
  constructor(size) {
    this.canvas = Array.from({ length: size }, () =>
      Array.from({ length: size }, () => null)
    )
  }

  draw(rectangle) {
    const { positions, character } = rectangle
    for (const [row, col] of positions) {
      if (this.canvas[row][col] === null) this.canvas[row][col] = [character]
      else this.canvas[row][col].push(character)
    }
  }

  remove(rectangle) {
    const { positions, character } = rectangle
    if (positions.length === 0) return
    for (const position of positions) {
      const list = this.canvas[position[0]][position[1]]
      list.splice(list.indexOf(character), 1)
    }
  }

  print(palette) {
    let rows = ''

    for (let i = 0; i < this.canvas.length; i++) {
      for (let j = 0; j < this.canvas[i].length; j++) {
        if (this.canvas[i][j] === null || this.canvas[i][j].length === 0) {
          rows += '*' // empty
        } else {
          rows += palette[this.canvas[i][j][this.canvas[i][j].length - 1]]
        }
      }

      rows += '\n'
    }
    console.log(rows)
  }

  moveToTop(rectangle) {
    this.remove(rectangle)
    this.draw(rectangle)
  }

  drag(startPosition, endPosition, map) {
    const list = this.canvas[startPosition[0]][startPosition[1]]
    if (list?.length === 0) return

    const character = list[list.length - 1]

    const rectangle = map[character]
    const dx = endPosition[0] - startPosition[0]
    const dy = endPosition[1] - startPosition[1]
    this.remove(rectangle)
    rectangle.move(dx, dy)
    this.draw(rectangle)
  }
}

And I have a map that maps the Rectangle's character to that Rectangle instance. and another object called palette which tells the canvas how that shape or pixel would look like when being printed (initially palette is part of canvas but I feel like I should decouple them so canvas only holds the state of the current canvas and is not opinionated on how the shapes should look like when they are printed out).

const rectangle1 = new Rectangle(
  [
    [0, 1],
    [1, 2],
    [0, 2],
  ]
)
const rectangle2 = new Rectangle(
  [
    [0, 0],
    [0, 1],
    [1, 0],
  ]
)
const rectangle3 = new Rectangle([[0, 0]])


const palette = {
  [rectangle1.character]: 'A',
  [rectangle2.character]: 'B',
  [rectangle3.character]: 'C',
}

const map = {
  [rectangle1.character]: rectangle1,
  [rectangle2.character]: rectangle2,
  [rectangle3.character]: rectangle3,
}

here is the link to a live demo

Please feel free to give me any feedback. Specifically, I would love to know:

  1. if there is a better to approach the OO design here? Did I do a good job at separating the concerns here? Does every class have the methods here?
  2. I think I need some help to refactor the map and palette. Specifically map, it is manually constructed right now. I am not sure if I need to create two classes for them.
  3. Is there any alternatives to approach implementing such a canvas? What are some of the pros and cons?
\$\endgroup\$
0
1
\$\begingroup\$

Question responses

if there is a better to approach the OO design here?

It seems okay. As was pointed out in answers to your other recent questions there currently is no use of OO features like inheritance.

Did I do a good job at separating the concerns here?

The separation seems okay.

Suggestions

Simplify move method

The Rectangle method move is currently implemented as this:

move(dx, dy) {
    for (const position of this.positions) {
      const nx = position[0] + dx
      const ny = position[1] + dy
      position[0] = nx
      position[1] = ny
    }
  }

Variables nx and ny are only used once. The method could greatly be simplified:

position[0] += dx;
position[1] += dy;

functional approach to print

In the Canvas method print two for loops are used. A functional approach, similar to how the canvas is created in the constructor, would allow fewer lines and the variable rows could be assigned in one statement, which would allow const to be used, though this would not be wise to change if performance is a major concern.

print(palette) {
    const rows = this.canvas.map(column => column.map(cell => {
      if (cell === null || !cell.length) {
        return '*';
      }
      return palette[cell[cell.length - 1]];
    }).join('') + "\n" ).join('');

check parameter types

Since JavaScript is loosely typed, there isn’t much support for ensuring a method is called with parameters of certain types. However, if a method expects to be called with certain types of arguments (e.g. Canvas::draw() and ::remove() expect a Rectangle instance) then the type of those arguments can be checked - e.g. using the typeof keyword. If an argument doesn’t match an expected type then perhaps an Error should be thrown.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.