search

Sheepolution / how-to-love

LÖVE tutorials
MIT License
102 stars 25 forks source link

Impove checkCollision() in chapter13 #21

Closed AlpacaMax closed 3 years ago

AlpacaMax commented 3 years ago

The return part in checkCollision() is changed and the new implementation is generally considered a better practice for 2 reasons:

  1. The condition of the if statement itself is a boolean value. Since the code returns true or false based on whether the if condition is true or false, we can directly return the condition. In this way we reduce the redundancy and improves the readability of the code.
  2. The and operator is moved to the front of each condition. This improves the readability of the code as it is clearer what operator is used in the expressions. This is trivial in this code as each boolean expression connected by and is very short. However considering a lot of the readers of this tutorial are beginners I think it's necessary to show them this type of "clean code" thinking.
Sheepolution commented 3 years ago

The reason it's like this is because we want to focus on teaching the collision check. If in addition to that we also teach that statements can be used as return value the reader might get overwhelmed with information. So I appreciate the effort but I will have to decline your PR. Though I do agree with placing the and on the next line.

AlpacaMax commented 3 years ago

I see. But do you think it will be better if we have a follow-up after that code saying that the return part can be simplified into my implementation? I guess it will introduce less confusion.

Sheepolution commented 3 years ago

Sure, we can do that.

Sheepolution commented 3 years ago

Thanks!

AlpacaMax commented 3 years ago

No problem! However I just realized that you mentioned the exact same thing in chapter23. So do we also change that part just to maintain the consistency?(I'll open another PR for that)

Sorry to keep bothering you with these small details BTW. I just feel that they are important.

Sheepolution commented 3 years ago

Perhaps change it into a reminder that return statements work like that?

Don't be sorry. I'm happy that you want to help.

AlpacaMax commented 3 years ago

Great! I'll start working on it when I have time.

BTW, I forgot to mention that in this PR I also changed chapter14, in which I moved 'and' to the front in checkCollision(). So you need to update that chapter on the website as well.