adammyhre / Unity-Utils

Extension Methods and Utils for Unity Game Dev
The Unlicense
386 stars 33 forks source link

Vector2Extensions #2

Closed AndrzejKebab closed 11 months ago

AndrzejKebab commented 11 months ago

I did same for Vector2

adammyhre commented 11 months ago

Codespaces has a known issue where I can't publish an actual code review due to permissions on a forked repository. https://github.com/microsoft/vscode-pull-request-github/issues/5325

So, I can just add some comments here.

  1. In the Add method, this Vector3 should be this Vector2
  2. I think the method name Remove might convey the meaning of total removal (as in setting the value to zero). A better name would be the symmetrical word for Add, which is Subtract. However, either naming convention could also cause some issues with clarity, as the User from the outside may be unsure if they need to supply a positive or negative number without looking at the code. The best approach is delete the Remove method, and just keep the Add method as was done with the Vector3 extension class - the User of the method can then choose for themselves if they want to add a negative or positive amount.
  3. There are 2 typos in the last method. InRangeOff should be InRangeOf and curret should be current

Thanks for your contribution, always nice to see it grow!

AndrzejKebab commented 11 months ago

Oh i didn't noticed this typos, and maybe instead Add/Subtract it should be named Modify?

adammyhre commented 11 months ago

Modify is an ok suggestion, but again will cause some ambiguity with the With method name - it's not immediately clear to the User what the modification will be - replacement, addition, or something else? Ideally you want the extension methods to read like an English sentence.

I want a new Vector2.zero with x: 1 I want a a new Vector2.up but add y:-1

It's easy to infer what's going to happen in both those cases without looking at the code. I think it's ok to leave the naming convention the same as in the Vector3 extensions class and use the Add method name - plus it's already an established convention in videos.

adammyhre commented 11 months ago

One more comment:

In the <summary of the Add and With methods, remove the reference to the z param

adammyhre commented 11 months ago

Perfect, looks great