On right click draw triangle to the canvas

Question 1

This is a simple program that using easelJS adds functionality to the right click, when the right mouse button is clicked a triangle is drawn to the screen. I am just wanting to get the opinions of others on my coding style and how it could be improved.

body{
 margin: 0;
 background-color: #d3d3d3;
 font-family: Sans-Serif;
}
h1{
 font-weight: 100;
 text-align: center;
}
canvas{
 display: block;
 margin: auto;
 background-color: #ffffff;
 border: solid 1px #000000;
}

<!DOCTYPE html>
<html>
 <head>
 <title>Draw Triangle</title>
 <link rel="stylesheet" href="styles.css"/>
 <script src="https://code.createjs.com/easeljs-0.8.2.min.js"></script>
 <script>
 var stage;
 var triangle;
 function init() {
 stage = new createjs.Stage("myCanvas");
 stage.addEventListener("stagemousedown", function(evt) {
 var e = evt.nativeEvent;
 var key = e.which || e.keyCode;
 if(key !== 1){
 addTriangle(evt.stageX, evt.stageY);
 }
 });
 }
 function addTriangle(x, y){
 triangle = new createjs.Shape();
 triangle.graphics.beginFill("#000000");
 triangle.graphics.moveTo(x, y).lineTo(x-100, y).lineTo(x-50,y-100).lineTo(x, y);
 stage.addChild(triangle);
 stage.update();
 }
 </script>
 </head>
 <body onload="init();">
 <h1>Right click to draw a Triangle to the canvas</h1>
 <canvas id="myCanvas" width="900" height="700"></canvas>
 </body>
</html>

Question 2

General problems

It is better to add events via addEventListener rather than in the HTML, this keeps the source code together making it easier to maintain. It also protects your code from 3rd party code overwriting the event handler. If some code later sets window.onload = theirFunction; it overwrites your event that will not get called.
Keep your variables out of the global scope if possible. stage and triangle do not need to be in global scope. triangle is only used in the one function so define it there.
Use const rather than var if you do not intend to modify a variable.
Don't let lines get too long. 80 character is the usual limit. If a line gets longer than that break it into multiple lines.
Right click will bring up the context menu. You can prevent this by listening to the contextmenu event and calling preventDefault on the event object.
Why the defaulting to keyCode? is that even defined for the easelJS event ?
Don't add needless code. You define a variable for evt.nativeEvent, and e.which. These are not really needed.
Use correct names for variables. You use key for which button is pressed, this can be confusing when code gets longer.
You say you want to add a triangle on the right click but the code you had will add a triangle for any button except the left button.
Make your code easier to read by using spaces after commas, between operators, after statements and befor { eg if(bar===poo){foo(a,b+2,c);} is better if (bar === poo) { foo(a, b + 2, c); }

The rewrite

Rewrite of your code using the above.

body{
 margin: 0;
 background-color: #d3d3d3;
 font-family: Sans-Serif;
}
h1{
 font-weight: 100;
 text-align: center;
}
canvas{
 display: block;
 margin: auto;
 background-color: #ffffff;
 border: solid 1px #000000;
}

<!DOCTYPE html>
<html>
 <head>
 <title>Draw Triangle</title>
 <link rel="stylesheet" href="styles.css"/>
 <script src="https://code.createjs.com/easeljs-0.8.2.min.js"></script>
 <script>
 addEventListener("load", init); // add the load event listener 
 function init() {
 // I am getting canvas as I need it to get the contextmenu event
 const canvas = document.getElementById("myCanvas");
 // use const for variables that will not change.
 const stage = new createjs.Stage(canvas);
 stage.addEventListener("stagemousedown", mouseDown);
 // to prevent the context menu popping up
 canvas.addEventListener("contextmenu", (event) => {
 event.preventDefault();
 });
 // simplify the mouseDown event
 function mouseDown(evt) {
 if (evt.nativeEvent.which === 3) {
 addTriangle(evt.stageX, evt.stageY);
 }
 }
 function addTriangle(x, y) {
 const triangle = new createjs.Shape(); // define triangle in the scope it is used
 triangle.graphics.beginFill("#000");
 triangle.graphics // break the line to make it more readable and easier to modify
 .moveTo(x, y) // use spaces after commas
 .lineTo(x - 100, y) // and spaces around operators
 .lineTo(x - 50, y - 100)
 .lineTo(x, y);
 stage.addChild(triangle);
 stage.update();
 }
 }
 </script>
 </head>
 <body>
 <h1>Right click to draw a Triangle to the canvas</h1>
 <canvas id = "myCanvas" width = "900" height = "700"></canvas>
 </body>
</html>

Additional notes.

I don't know what your intention is but the triangle seems a little odd not being centered on the mouse click. Also I would define the triangle as a abstract path rather than hard coded to the function.

Is there a reason you put the code in the head of the document. You could have placed it at the bottom of the page and avoided having to wait for the page load event.

Also, maybe you are wanting to learn easelJS, but the load time of easelJS is substantial and what you do can just as easily be done using the native 2D API.

I strongly believe that you should have a very good reason to use a 3rd party library, especially when you are learning. You are best spending your time learning the native (and very good and performant) APIs. You may not always be able to use the library, but you will always have access to the native API

Rewrite without easelJS

To match your code I have implemented a very simple stage. Though what you do does not require a stage object and could be a lot simplier.

// Please note this code uses a compact style for single line statements
// There is no ; before the closing } to comply with JSLint rule 
// "Semicolan must be followed by newline."
;(() => { // wrapping your code inside a function alows you to keep the global scope clean 
 // the ; befor the ( is to prevent code above it without a ; from 
 // mistaking the ( as a function call
 const canvas = document.getElementById("myCanvas");
 const ctx = canvas.getContext("2d");
 const shapes = { // you could add more shapes here
 triangle: [ [0, -20], [20, 20], [-20, 20] ], // coordinates of triangle
 };
 addEventListener("load", init); // add the load event listener 
 // implement a stage object to hold shapes
 const stage = {
 shapes: [],
 addShape(x, y, color, path) { this.shapes.push({x, y, color, path }) },
 update() { // function to redraw all shapes on the stage
 ctx.clearRect(0, 0, canvas.width, canvas.height);
 this.shapes.forEach((shape) => {
 var i;
 ctx.fillStyle = shape.color;
 ctx.beginPath();
 i = 0;
 while (i < shape.path.length) { 
 ctx.lineTo( // sometimes it is best to break up complex lines
 shape.path[i ][0] + shape.x,
 shape.path[i++][1] + shape.y
 ) 
 }
 ctx.closePath(); // this is really only needed if you are using stroke
 ctx.fill();
 });
 }
 };
 function init() {
 canvas.addEventListener("mousedown", mouseDown);
 canvas.addEventListener("contextmenu", event => event.preventDefault() );
 function mouseDown(event) {
 const bounds = event.target.getBoundingClientRect();
	 const x = event.pageX - bounds.left - scrollX;
	 const y = event.pageY - bounds.top - scrollY;
 if (event.which === 3) {
 stage.addShape(x, y, "black", shapes.triangle);
 stage.update();
 }
 }
 }
})();

body {
 margin: 0;
 background-color: #d3d3d3;
 font-family: Sans-Serif;
}
h1 {
 font-weight: 100;
 text-align: center;
}
canvas {
 display: block;
 margin: auto;
 background-color: #ffffff;
 border: solid 1px #000000;
}

<!DOCTYPE html>
<html>
<head>
 <title>Draw Triangle</title>
</head>
<body>
 <h1>Right click to draw a Triangle to the canvas</h1>
 <canvas id="myCanvas" width="900" height="700"></canvas>
 <script>/* you would put your script here */</script>
</body>
</html>

The simplest way to achieve the same functionality.

const canvas = document.getElementById("myCanvas");
const ctx = canvas.getContext("2d");
const triangle = [ [0, -20], [20, 20], [-20, 20] ];
canvas.addEventListener("mousedown", mouseDown);
canvas.addEventListener("contextmenu", e => e.preventDefault() );
function mouseDown(event) {
 const bounds = event.target.getBoundingClientRect();
 const x = event.pageX - bounds.left - scrollX;
 const y = event.pageY - bounds.top - scrollY;
 if (event.which === 3) { drawShape(x, y, triangle) }
}
function drawShape(x, y, shape) {
 var i = 0;
 ctx.fillStyle = "black";
 ctx.beginPath();
 while (i < shape.length) { ctx.lineTo( shape[i][0] + x, shape[i++][1] + y ) }
 ctx.fill();
}

body {
 margin: 0;
 background-color: #d3d3d3;
 font-family: Sans-Serif;
}
h1 {
 font-weight: 100;
 text-align: center;
}
canvas {
 display: block;
 margin: auto;
 background-color: #fff;
 border: solid 1px #000;
}

 <h1>Right click to draw a Triangle to the canvas</h1>
 <canvas id="myCanvas" width="900" height="700"></canvas>

Question 3

Why do you create function init outside of the addEventListener? Wouldn't it be better to use it directly? And why even bother? Should we use defer on the script and skip init event all together?

Question 4

@Akxe I prefer to use named functions as it allows for more readable stack trace. The load event is not needed, nor the deferring the script if at bottom of the page. I left the onload as that is the pattern the OP is likely safest with, and I did not have the time to explain, but for a quick mention. I was thinking to defer easelJS which would have required the onload to safely start the OPs script but all in all i will leave it for someone else to explain script loading, deferring, parsing, incomplete DOM, etc... but i will remove the init from the last snippet, missed that..

Blindman67 Blindman67 22.8k2 gold badges16 silver badges40 bronze badges · Accepted Answer · 2017-09-27 07:00:04Z

General problems

It is better to add events via addEventListener rather than in the HTML, this keeps the source code together making it easier to maintain. It also protects your code from 3rd party code overwriting the event handler. If some code later sets window.onload = theirFunction; it overwrites your event that will not get called.
Keep your variables out of the global scope if possible. stage and triangle do not need to be in global scope. triangle is only used in the one function so define it there.
Use const rather than var if you do not intend to modify a variable.
Don't let lines get too long. 80 character is the usual limit. If a line gets longer than that break it into multiple lines.
Right click will bring up the context menu. You can prevent this by listening to the contextmenu event and calling preventDefault on the event object.
Why the defaulting to keyCode? is that even defined for the easelJS event ?
Don't add needless code. You define a variable for evt.nativeEvent, and e.which. These are not really needed.
Use correct names for variables. You use key for which button is pressed, this can be confusing when code gets longer.
You say you want to add a triangle on the right click but the code you had will add a triangle for any button except the left button.
Make your code easier to read by using spaces after commas, between operators, after statements and befor { eg if(bar===poo){foo(a,b+2,c);} is better if (bar === poo) { foo(a, b + 2, c); }

The rewrite

Rewrite of your code using the above.

body{
 margin: 0;
 background-color: #d3d3d3;
 font-family: Sans-Serif;
}
h1{
 font-weight: 100;
 text-align: center;
}
canvas{
 display: block;
 margin: auto;
 background-color: #ffffff;
 border: solid 1px #000000;
}

<!DOCTYPE html>
<html>
 <head>
 <title>Draw Triangle</title>
 <link rel="stylesheet" href="styles.css"/>
 <script src="https://code.createjs.com/easeljs-0.8.2.min.js"></script>
 <script>
 addEventListener("load", init); // add the load event listener 
 function init() {
 // I am getting canvas as I need it to get the contextmenu event
 const canvas = document.getElementById("myCanvas");
 // use const for variables that will not change.
 const stage = new createjs.Stage(canvas);
 stage.addEventListener("stagemousedown", mouseDown);
 // to prevent the context menu popping up
 canvas.addEventListener("contextmenu", (event) => {
 event.preventDefault();
 });
 // simplify the mouseDown event
 function mouseDown(evt) {
 if (evt.nativeEvent.which === 3) {
 addTriangle(evt.stageX, evt.stageY);
 }
 }
 function addTriangle(x, y) {
 const triangle = new createjs.Shape(); // define triangle in the scope it is used
 triangle.graphics.beginFill("#000");
 triangle.graphics // break the line to make it more readable and easier to modify
 .moveTo(x, y) // use spaces after commas
 .lineTo(x - 100, y) // and spaces around operators
 .lineTo(x - 50, y - 100)
 .lineTo(x, y);
 stage.addChild(triangle);
 stage.update();
 }
 }
 </script>
 </head>
 <body>
 <h1>Right click to draw a Triangle to the canvas</h1>
 <canvas id = "myCanvas" width = "900" height = "700"></canvas>
 </body>
</html>

Additional notes.

I don't know what your intention is but the triangle seems a little odd not being centered on the mouse click. Also I would define the triangle as a abstract path rather than hard coded to the function.

Is there a reason you put the code in the head of the document. You could have placed it at the bottom of the page and avoided having to wait for the page load event.

Also, maybe you are wanting to learn easelJS, but the load time of easelJS is substantial and what you do can just as easily be done using the native 2D API.

I strongly believe that you should have a very good reason to use a 3rd party library, especially when you are learning. You are best spending your time learning the native (and very good and performant) APIs. You may not always be able to use the library, but you will always have access to the native API

Rewrite without easelJS

To match your code I have implemented a very simple stage. Though what you do does not require a stage object and could be a lot simplier.

// Please note this code uses a compact style for single line statements
// There is no ; before the closing } to comply with JSLint rule 
// "Semicolan must be followed by newline."
;(() => { // wrapping your code inside a function alows you to keep the global scope clean 
 // the ; befor the ( is to prevent code above it without a ; from 
 // mistaking the ( as a function call
 const canvas = document.getElementById("myCanvas");
 const ctx = canvas.getContext("2d");
 const shapes = { // you could add more shapes here
 triangle: [ [0, -20], [20, 20], [-20, 20] ], // coordinates of triangle
 };
 addEventListener("load", init); // add the load event listener 
 // implement a stage object to hold shapes
 const stage = {
 shapes: [],
 addShape(x, y, color, path) { this.shapes.push({x, y, color, path }) },
 update() { // function to redraw all shapes on the stage
 ctx.clearRect(0, 0, canvas.width, canvas.height);
 this.shapes.forEach((shape) => {
 var i;
 ctx.fillStyle = shape.color;
 ctx.beginPath();
 i = 0;
 while (i < shape.path.length) { 
 ctx.lineTo( // sometimes it is best to break up complex lines
 shape.path[i ][0] + shape.x,
 shape.path[i++][1] + shape.y
 ) 
 }
 ctx.closePath(); // this is really only needed if you are using stroke
 ctx.fill();
 });
 }
 };
 function init() {
 canvas.addEventListener("mousedown", mouseDown);
 canvas.addEventListener("contextmenu", event => event.preventDefault() );
 function mouseDown(event) {
 const bounds = event.target.getBoundingClientRect();
	 const x = event.pageX - bounds.left - scrollX;
	 const y = event.pageY - bounds.top - scrollY;
 if (event.which === 3) {
 stage.addShape(x, y, "black", shapes.triangle);
 stage.update();
 }
 }
 }
})();

body {
 margin: 0;
 background-color: #d3d3d3;
 font-family: Sans-Serif;
}
h1 {
 font-weight: 100;
 text-align: center;
}
canvas {
 display: block;
 margin: auto;
 background-color: #ffffff;
 border: solid 1px #000000;
}

<!DOCTYPE html>
<html>
<head>
 <title>Draw Triangle</title>
</head>
<body>
 <h1>Right click to draw a Triangle to the canvas</h1>
 <canvas id="myCanvas" width="900" height="700"></canvas>
 <script>/* you would put your script here */</script>
</body>
</html>

The simplest way to achieve the same functionality.

const canvas = document.getElementById("myCanvas");
const ctx = canvas.getContext("2d");
const triangle = [ [0, -20], [20, 20], [-20, 20] ];
canvas.addEventListener("mousedown", mouseDown);
canvas.addEventListener("contextmenu", e => e.preventDefault() );
function mouseDown(event) {
 const bounds = event.target.getBoundingClientRect();
 const x = event.pageX - bounds.left - scrollX;
 const y = event.pageY - bounds.top - scrollY;
 if (event.which === 3) { drawShape(x, y, triangle) }
}
function drawShape(x, y, shape) {
 var i = 0;
 ctx.fillStyle = "black";
 ctx.beginPath();
 while (i < shape.length) { ctx.lineTo( shape[i][0] + x, shape[i++][1] + y ) }
 ctx.fill();
}

body {
 margin: 0;
 background-color: #d3d3d3;
 font-family: Sans-Serif;
}
h1 {
 font-weight: 100;
 text-align: center;
}
canvas {
 display: block;
 margin: auto;
 background-color: #fff;
 border: solid 1px #000;
}

 <h1>Right click to draw a Triangle to the canvas</h1>
 <canvas id="myCanvas" width="900" height="700"></canvas>

Why do you create function init outside of the addEventListener? Wouldn't it be better to use it directly? And why even bother? Should we use defer on the script and skip init event all together?
@Akxe I prefer to use named functions as it allows for more readable stack trace. The load event is not needed, nor the deferring the script if at bottom of the page. I left the onload as that is the pattern the OP is likely safest with, and I did not have the time to explain, but for a quick mention. I was thinking to defer easelJS which would have required the onload to safely start the OPs script but all in all i will leave it for someone else to explain script loading, deferring, parsing, incomplete DOM, etc... but i will remove the init from the last snippet, missed that..

Stack Exchange Network

On right click draw triangle to the canvas

1 Answer 1

General problems

The rewrite

Additional notes.

Rewrite without easelJS

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Hot Network Questions

On right click draw triangle to the canvas

1 Answer 1

General problems

The rewrite

Additional notes.

Rewrite without easelJS

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Related

Hot Network Questions