2
\$\begingroup\$

I have written a simple linked list visualization program in JavaScript:

Linked list diagram

I am just a beginner in html5 canvas. I am pretty much satisfied with how it works, but I´d like to know if I made some things, that could have been done better.

<!DOCTYPE html>
<html>
<head>
  <meta charset="utf-8">
  <meta name="viewport" content="width=device-width">
  <title>JS Bin</title>
</head>
<body>
<canvas id="canvas"></canvas>
<script>
	

var Shape=Shape || {
	x:0,
	y:0,
	w:0,
	h:0,
	t:0,
	rects:[],
	create:function(){
		var obj=Object.create(this);
		return obj;
	},
	add:function(x,y,w,h,t){
		this.x=x;
		this.y=y;
		this.w=w;
		this.h=h;
		this.t=t;
		this.rects.push({x,y,w,h,t});
	},
	draw:function(context,text){
		for (var i in this.rects) {
        oRec = this.rects[i];
        context.fillStyle = 'red'
        context.fillRect(oRec.x, oRec.y, oRec.w, oRec.h);
   			context.fillText(oRec.t, oRec.x,oRec.y);
        context.beginPath();
	      context.moveTo(oRec.x+oRec.w/2,oRec.y+oRec.h/2);
	      context.lineTo(oRec.x+100,oRec.y+12);
	      context.stroke();
	      context.closePath();
	      if(i==this.rects.length){

	      }
   

    }
	}

};
window.onload=function(){
	  var canvas=document.getElementById("canvas");
	  var ctx=canvas.getContext("2d");
	  var h=canvas.width=window.innerWidth;
	  var w=canvas.height=window.innerHeight;
	  var node=Shape.create();
	  node.add(10, 100, 25, 25,1)
	  node.add(100,100, 25, 25,2)
	  node.add(200,100, 25, 25,3)
	  node.add(300,100, 25, 25,4)
	  node.add(400,100, 25, 25,5)
	  node.add(500,100, 25, 25,6)
	  node.draw(ctx);
	  
};


</script>
</body>
</html>

Improve this question
\$\endgroup\$
1
  • 1
    \$\begingroup\$ You have data (coordinates and labels for the nodes) and a way to draw that data. There is no need to solve this with mutation. That is, get rid of your add method. Instead, just have a draw function that accepts an array of objects, and draws them. \$\endgroup\$
    – Jonah
    May 9, 2016 at 5:08

1 Answer 1

Reset to default
1
\$\begingroup\$

I'm not sure why are you using Object.create to instantiate your shape. You should know that operator instanceof only works with real javascript pseudoclasses (that is, if you even think you need pseudoclass for this), which look like this:

function Shape() {
  this.x = 0;
  this.y = 0;
  this.w = 0;
  this.h = 0;
  this.t = 0;
  this.rects = [];
}
Shape.prototype = {
  add:function(x,y,w,h,t) {
    this.x=x;
    this.y=y;
    this.w=w;
    this.h=h;
    this.t=t;
    this.rects.push({x,y,w,h,t});
  },
  draw:function(context,text) {
    for (var i in this.rects) {
      oRec = this.rects[i];
      context.fillStyle = 'red'
      context.fillRect(oRec.x, oRec.y, oRec.w, oRec.h);
      context.fillText(oRec.t, oRec.x,oRec.y);
      context.beginPath();
      context.moveTo(oRec.x+oRec.w/2,oRec.y+oRec.h/2);
      context.lineTo(oRec.x+100,oRec.y+12);
      context.stroke();
      context.closePath();
      if(i==this.rects.length){
        // wtf is this exactly?
      }
    }
  }
}

There's also one very important difference, which is that in your case rects property is shared between all instances!. Here's a demonstration using your Shape. Here is the same example with my code.

Except for that, there's nothing much to say about the code. I'd be more interested how does it actually handle a linked list. Does it check for cycles? Does it show whether the list is circular or null-terminated?

Maybe just make the text black and put it one pixel up for readability. It looks better.

      context.fillStyle = 'black';
      context.fillText(oRec.t, oRec.x,oRec.y-1);
Improve this answer
\$\endgroup\$

Your Answer

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

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