Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

Quill already did a good job speeding up your code, so I'll focus on the code you have right now.


#Give me the object!

Give me the object!

(Not how to get it)

Your drawGrid function takes the id of a DOM element that the function is expected to find on its own. However, this is not good practice. Instead, you should pass the DOM element itself. This could also speed up your code by a lot if used correctly.

For more on why it's bad practice, think about it this way: if you need to do some specific checking on an element after you've found it before you are ready to give it to the function, how are you supposed to pass that prepared element to the function?

Don't let the function worry about how to get what it needs; just give it what it needs.


#Fetch me the context! Now throw it away!

Fetch me the context! Now throw it away!

var ctx = canvas.getContext('2d');

This is created every time your function is called. Assuming more drawing will take place in this code, why not put both the canvas and the context in a global scope? There is no point in finding the context, putting it in a variable, and then destroying it at the end of the function every time it is called.


###Ask the canvas where the context is. Then, go ask the context where the canvas is.

Ask the canvas where the context is. Then, go ask the context where the canvas is.

var canvas = document.getElementById(id);
...
ctx.canvas.width = w;
ctx.canvas.height = h;

This makes absolutely zero sense. First, you use the canvas to get the ctx. Then, you use the ctx to get the canvas again by accessing its properties. Why not just use the canvas that you literally just defined?


#There is always another way

There is always another way

Yes, the way you are currently forming the grid is very slow. As Quill has already shown, there are other options:

  • Render it as SVG (Quill has already shown this)
  • Get a single image that is the portion of a grip, then copy pasta that image around the area you need.
  • Draw out the squares.

I did not test these out, so I don't know how much of a speed boost they provide (if any). However, feel free to try some of them.

Quill already did a good job speeding up your code, so I'll focus on the code you have right now.


#Give me the object!

(Not how to get it)

Your drawGrid function takes the id of a DOM element that the function is expected to find on its own. However, this is not good practice. Instead, you should pass the DOM element itself. This could also speed up your code by a lot if used correctly.

For more on why it's bad practice, think about it this way: if you need to do some specific checking on an element after you've found it before you are ready to give it to the function, how are you supposed to pass that prepared element to the function?

Don't let the function worry about how to get what it needs; just give it what it needs.


#Fetch me the context! Now throw it away!

var ctx = canvas.getContext('2d');

This is created every time your function is called. Assuming more drawing will take place in this code, why not put both the canvas and the context in a global scope? There is no point in finding the context, putting it in a variable, and then destroying it at the end of the function every time it is called.


###Ask the canvas where the context is. Then, go ask the context where the canvas is.

var canvas = document.getElementById(id);
...
ctx.canvas.width = w;
ctx.canvas.height = h;

This makes absolutely zero sense. First, you use the canvas to get the ctx. Then, you use the ctx to get the canvas again by accessing its properties. Why not just use the canvas that you literally just defined?


#There is always another way

Yes, the way you are currently forming the grid is very slow. As Quill has already shown, there are other options:

  • Render it as SVG (Quill has already shown this)
  • Get a single image that is the portion of a grip, then copy pasta that image around the area you need.
  • Draw out the squares.

I did not test these out, so I don't know how much of a speed boost they provide (if any). However, feel free to try some of them.

Quill already did a good job speeding up your code, so I'll focus on the code you have right now.


Give me the object!

(Not how to get it)

Your drawGrid function takes the id of a DOM element that the function is expected to find on its own. However, this is not good practice. Instead, you should pass the DOM element itself. This could also speed up your code by a lot if used correctly.

For more on why it's bad practice, think about it this way: if you need to do some specific checking on an element after you've found it before you are ready to give it to the function, how are you supposed to pass that prepared element to the function?

Don't let the function worry about how to get what it needs; just give it what it needs.


Fetch me the context! Now throw it away!

var ctx = canvas.getContext('2d');

This is created every time your function is called. Assuming more drawing will take place in this code, why not put both the canvas and the context in a global scope? There is no point in finding the context, putting it in a variable, and then destroying it at the end of the function every time it is called.


Ask the canvas where the context is. Then, go ask the context where the canvas is.

var canvas = document.getElementById(id);
...
ctx.canvas.width = w;
ctx.canvas.height = h;

This makes absolutely zero sense. First, you use the canvas to get the ctx. Then, you use the ctx to get the canvas again by accessing its properties. Why not just use the canvas that you literally just defined?


There is always another way

Yes, the way you are currently forming the grid is very slow. As Quill has already shown, there are other options:

  • Render it as SVG (Quill has already shown this)
  • Get a single image that is the portion of a grip, then copy pasta that image around the area you need.
  • Draw out the squares.

I did not test these out, so I don't know how much of a speed boost they provide (if any). However, feel free to try some of them.

added 29 characters in body
Source Link
SirPython
  • 13.4k
  • 3
  • 38
  • 93

Quill already did a good job speeding up your code, so I'll focus on the code you have right now.


#Give me the object!

(Not how to get it)

Your drawGrid function takes the id of a DOM element that the function is expected to find on its own. However, this is not good practice. Instead, you should pass the DOM element itself. This could also speed up your code by a lot if used correctly.

For more on why it's bad practice, think about it this way: if you need to do some specific checking on an element after you've found it before you are ready to give it to the function, how are you supposed to pass that prepared element to the function?

Don't let the function worry about how to get what it needs; just give it what it needs.


#Fetch me the context! Now throw it away!

var ctx = canvas.getContext('2d');

This is created every time your function is called. If you haveAssuming more drawing will take place in this global canvas variablecode, why not also haveput both the canvas and the context in a global ctx variablescope? There is no point in finding the context, putting it in a variable, and then destroying it at the end of the function every time it is called.


###Ask the canvas where the context is. Then, go ask the context where the canvas is.

var canvas = document.getElementById(id);
...
ctx.canvas.width = w;
ctx.canvas.height = h;

This makes absolutely zero sense. First, you use the canvas to get the ctx. Then, you use the ctx to get the canvas again by accessing its properties. Why not just use the canvas that you literally just defined?


#There is always another way

Yes, the way you are currently forming the grid is very slow. As Quill has already shown, there are other options:

  • Render it as SVG (Quill has already shown this)
  • Get a single image that is the portion of a grip, then copy pasta that image around the area you need.
  • Draw out the squares.

I did not test these out, so I don't know how much of a speed boost they provide (if any). However, feel free to try some of them.

Quill already did a good job speeding up your code, so I'll focus on the code you have right now.


#Give me the object!

(Not how to get it)

Your drawGrid function takes the id of a DOM element that the function is expected to find on its own. However, this is not good practice. Instead, you should pass the DOM element itself. This could also speed up your code by a lot if used correctly.

For more on why it's bad practice, think about it this way: if you need to do some specific checking on an element after you've found it before you are ready to give it to the function, how are you supposed to pass that prepared element to the function?

Don't let the function worry about how to get what it needs; just give it what it needs.


#Fetch me the context! Now throw it away!

var ctx = canvas.getContext('2d');

This is created every time your function is called. If you have this global canvas variable, why not also have a global ctx variable? There is no point in finding the context, putting it in a variable, and then destroying it at the end of the function every time it is called.


###Ask the canvas where the context is. Then, go ask the context where the canvas is.

var canvas = document.getElementById(id);
...
ctx.canvas.width = w;
ctx.canvas.height = h;

This makes absolutely zero sense. First, you use the canvas to get the ctx. Then, you use the ctx to get the canvas again by accessing its properties. Why not just use the canvas that you literally just defined?


#There is always another way

Yes, the way you are currently forming the grid is very slow. As Quill has already shown, there are other options:

  • Render it as SVG (Quill has already shown this)
  • Get a single image that is the portion of a grip, then copy pasta that image around the area you need.
  • Draw out the squares.

I did not test these out, so I don't know how much of a speed boost they provide (if any). However, feel free to try some of them.

Quill already did a good job speeding up your code, so I'll focus on the code you have right now.


#Give me the object!

(Not how to get it)

Your drawGrid function takes the id of a DOM element that the function is expected to find on its own. However, this is not good practice. Instead, you should pass the DOM element itself. This could also speed up your code by a lot if used correctly.

For more on why it's bad practice, think about it this way: if you need to do some specific checking on an element after you've found it before you are ready to give it to the function, how are you supposed to pass that prepared element to the function?

Don't let the function worry about how to get what it needs; just give it what it needs.


#Fetch me the context! Now throw it away!

var ctx = canvas.getContext('2d');

This is created every time your function is called. Assuming more drawing will take place in this code, why not put both the canvas and the context in a global scope? There is no point in finding the context, putting it in a variable, and then destroying it at the end of the function every time it is called.


###Ask the canvas where the context is. Then, go ask the context where the canvas is.

var canvas = document.getElementById(id);
...
ctx.canvas.width = w;
ctx.canvas.height = h;

This makes absolutely zero sense. First, you use the canvas to get the ctx. Then, you use the ctx to get the canvas again by accessing its properties. Why not just use the canvas that you literally just defined?


#There is always another way

Yes, the way you are currently forming the grid is very slow. As Quill has already shown, there are other options:

  • Render it as SVG (Quill has already shown this)
  • Get a single image that is the portion of a grip, then copy pasta that image around the area you need.
  • Draw out the squares.

I did not test these out, so I don't know how much of a speed boost they provide (if any). However, feel free to try some of them.

Source Link
SirPython
  • 13.4k
  • 3
  • 38
  • 93

Quill already did a good job speeding up your code, so I'll focus on the code you have right now.


#Give me the object!

(Not how to get it)

Your drawGrid function takes the id of a DOM element that the function is expected to find on its own. However, this is not good practice. Instead, you should pass the DOM element itself. This could also speed up your code by a lot if used correctly.

For more on why it's bad practice, think about it this way: if you need to do some specific checking on an element after you've found it before you are ready to give it to the function, how are you supposed to pass that prepared element to the function?

Don't let the function worry about how to get what it needs; just give it what it needs.


#Fetch me the context! Now throw it away!

var ctx = canvas.getContext('2d');

This is created every time your function is called. If you have this global canvas variable, why not also have a global ctx variable? There is no point in finding the context, putting it in a variable, and then destroying it at the end of the function every time it is called.


###Ask the canvas where the context is. Then, go ask the context where the canvas is.

var canvas = document.getElementById(id);
...
ctx.canvas.width = w;
ctx.canvas.height = h;

This makes absolutely zero sense. First, you use the canvas to get the ctx. Then, you use the ctx to get the canvas again by accessing its properties. Why not just use the canvas that you literally just defined?


#There is always another way

Yes, the way you are currently forming the grid is very slow. As Quill has already shown, there are other options:

  • Render it as SVG (Quill has already shown this)
  • Get a single image that is the portion of a grip, then copy pasta that image around the area you need.
  • Draw out the squares.

I did not test these out, so I don't know how much of a speed boost they provide (if any). However, feel free to try some of them.

default

AltStyle によって変換されたページ (->オリジナル) /