t0ri-make-school-coursework/breakout

MDN Break Out Tutorial for FEW 1.2 at Make School.

breakout's Issues

use dependency injection here

https://github.com/t0ri/breakout/blob/5372aad6ecb7a401198b52814b844b7a41fc0d18/js/script.js#L17

Should be passing ctx as a parameter here.

What's wrong here?

https://github.com/t0ri/breakout/blob/5372aad6ecb7a401198b52814b844b7a41fc0d18/js/script.js#L12

Depending on external global variable ctx not good practice. If you're class needs a reference to this pass it as a parameter on instantiation, if you only need it for a subroutine pass it as a parameter in that function. If you only need a piece of info from an object like the width or height, pass those numbers in.

These are best practices you should apply them everywhere!

Why not const?

https://github.com/t0ri/breakout/blob/5372aad6ecb7a401198b52814b844b7a41fc0d18/js/script.js#L333

Did you reassign these variables to another object? Probably not.

Were you intending to do that? Probably not

If you did would it break things? It might

For all of these reasons these should all be const!

And you also get the added bonus of signaling your intent with the code. These values are not supposed to change anyone can read that clearly when you use const. With let and var it's not clear.

Great code quality

https://github.com/t0ri/breakout/blob/5372aad6ecb7a401198b52814b844b7a41fc0d18/js/script.js#L1

Great code quality good work on the comments and formatting.

Overall

Your game looks good. Looks like you solved many of the challenges!

Your readme looks good, thanks!

Nice work

https://github.com/t0ri/breakout/blob/5372aad6ecb7a401198b52814b844b7a41fc0d18/js/script.js#L10

Great job here. Putting the background drawing routines in a class is a good idea. It makes this portable (more on this in a moment), and encapsulates this functionality in one place. It allows you to easily expand on this in the future. You can add a feature that changes the background be sending a message to the instance of this class.

Use dependency injection

You missed the concept of dependency injection. This is an important concept that you should apply here.

What's wrong? Your classes rely on an external global variable: ctx. If you were use these classes in another project it's obvious that you'd need this global variable, and even then it relies on the name ctx which might not be available.

Better, pass this dependency in as a parameter. This way the requirement is obvious it's a parameter, while the parameter is named ctx externally you can use any name.

Here you defined ctx:

https://github.com/t0ri/breakout/blob/5372aad6ecb7a401198b52814b844b7a41fc0d18/js/script.js#L5

You should pass ctx in here as a parameter:

https://github.com/t0ri/breakout/blob/5372aad6ecb7a401198b52814b844b7a41fc0d18/js/script.js#L17

Here you should pass ctx as a parameter:

https://github.com/t0ri/breakout/blob/5372aad6ecb7a401198b52814b844b7a41fc0d18/js/script.js#L351

You should follow this pattern for all of your classes!

Depends on external global var

https://github.com/t0ri/breakout/blob/5372aad6ecb7a401198b52814b844b7a41fc0d18/js/script.js#L27

Using canvas here is not best practice. Looks like you really only need the width and height.

You need to get rid of the dependance on canvas here. Choose your route:

1. pass canvas as a parameter
2. pass the width and height as a parameters
3. Get the width and height from ctx
4. add a property that has a reference to canvas to the class
5. Add properties for width and height to the class

Some of these solutions are better than others. Study them closely. Run a simulation. Apply the calipers and slide rule. I expect a report on my desk Monday morning! Or at the very least apply one of the solutions to your work.