I have been looking around for a good place to get my code reviewed for some time. I just stumbled on this site and I was hoping some people could tell me if I'm progressing in the right direction. This is a snippet from a sudoku game I'm making. The full source code can be viewed at http://www.lesshardtofind.com/Sudoku/sudoku.js and a working example (only the user input segment) at http://www.lesshardtofind.com/Sudoku/main.html

function Cell(X, Y){                                                      // Object that contains all the data needed for a sudoku cell
  this.Size = CELLSIZE;                                                     // This object's size
  this.Value = 0;                                                           // Displayed value 1-9
  this.Color = '#DDDDFF';                                                   // I wanted to use CellColor but a glitch caused some of them to turn black     
  this.BorderColor = 'black'                                                
  this.Xloc = X;                                                            // Xcoordinate value on the canvas
  this.Yloc = Y;                                                            // Ycoordinate value on the canvas
  this.Draw = function(){                                                   // The function to draw the cell on the screen
    var CNV = Get(CANVASID);                                                // setup the context
    var CTX = CNV.getContext('2d');
    CTX.fillStyle = this.Color;                                             // setup and draw the rectangle
    CTX.fillRect(this.Xloc, this.Yloc, this.Size, this.Size);
    CTX.moveTo(this.Xloc, this.Yloc);                                       // draw the boarder
    CTX.lineTo(this.Xloc+this.Size, this.Yloc);
    CTX.lineTo(this.Xloc+this.Size, this.Yloc+this.Size);
    CTX.lineTo(this.Xloc, this.Yloc+this.Size);
    CTX.lineTo(this.Xloc, this.Yloc);
    CTX.strokeStyle = this.BorderColor;                                     
    CTX.lineWidth = 1;
    if(this.Value){                                                         // if the value isn't 0 draw the current value to the screen
      CTX.fillStyle = 'black';
      CTX.font = "30px kaushan_scriptregular";
      CTX.fillText(this.Value, this.Xloc+14, this.Yloc+33);
function Board(){                                                           // the main game board object definition
  this.Rows = new Array;                                                    // the array to hold the cells of the board
  this.Born = false;                                                        // boolean to say if this is the initiation of the object
  this.Setup = function(){                                                  // setup the board and initiate all variables to appropriate values and instantiate objects
    var CurX = 280;                                                         // The starting X coordinate
    var CurY = 50;                                                          // The starting Y coordinate
    for(var x = 0; x < 9; x++)                                              // Set all the Rows array data position as arrays creating a 9x9 grid
      this.Rows[x] = new Array;                    
    for(var x = 0; x < 9; x++){                                             // loop through all 81 cells and create them at the right location
      for(var y = 0; y < 9; y++){
        this.Rows[x][y] = new Cell(CurX, CurY);
        this.Rows[x][y].Value = 0;
        CurX += this.Rows[x][y].Size;
      CurY += this.Rows[x][0].Size;
      CurX = 280;
    this.Born = true;                                                       // set the flag that the board was initiated
  this.Draw = function(){                                                   // draw method to handle all objects within the board
    for(var r = 0; r < 9; r++){                                             // loop through all cells and call their draw method
      for(var c = 0; c < 9; c++){

1 Answer 1


I don't seem to see the need for the methods to be declared inside the constructor. You aren't making any property "private". Also, declaring methods this way makes copies of the methods for each instance.

I suggest you move them out to the prototype instead. That way, they are declared once, and shared across instances, saving memory.

function Cell(x,y){
  this.size = CELLSIZE;
  //properties here

//Methods out here
Cell.prototype = {
  constructor : Cell, //refer back the constructor
  draw : function(){...},
  //more methods for Cell here

I also notice CELLSIZE and CANVASID. If they are globals, then I suggest you not make them globals.

Lastly, I suggest you use a canvas framework instead of drawing the shapes by hand. Learning the basics is good, but most developers don't code from scratch (unless there's a compelling need to do so). I suggest using PaperJS or KineticJS for drawing your shapes.

  • \$\begingroup\$ CELLSIZE and CANVASID are CONSTs inside the Sudoku Object therefore would still not conflict with any other JavaScript code, but could still be shared by all methods and only changed in one place. Is this bad? \$\endgroup\$ Commented Sep 2, 2013 at 18:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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