-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
IComparable in Vector2 and unit tests for it #47
IComparable in Vector2 and unit tests for it #47
Conversation
Hypercube.Math/Vectors/Vector2.cs
Outdated
|
||
public int CompareTo(Vector2 other) | ||
{ | ||
return Length.CompareTo(other.Length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Касаемо этого, я думал насчёт использование чистой длины, но как мне известно для большого количество итераций, взятие корня будет вызывать лаги (А мы ведь используем вектора в физике и отрисовке). Суть в том, что лучше будет использовать LengthSqueared
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Действительно, взятие корня будет накладно по производительности. Исправлю на сравнение по квадратам площади
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
На будущее, я пока не написал гайдлайны, если есть возможность лучше использовать var
, вместо прямого указание типа, хоть это и не сходится с нотацией от Microsoft
namespace Hypercube.UnitTests.Math; | ||
|
||
[TestFixture] | ||
public class CompareToTest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Классы, которые мы не предполагаем, что будут наследованы (Когда либо). Мы помечаем как sealed
Hypercube.Math/Vectors/Vector2.cs
Outdated
} | ||
} | ||
|
||
public enum ComparisonType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Думаю, это может использоваться для Vector2Int
или Vector3
т.к. по именованию он не привязан Vector2
, тогда нам его нужно вынести. Но данная практика мне кажется немного странной. По сколько мы и так можем легко сравнить vector.X > a
, это не возымеет трудностей, но вот enum
вносит непорядок в нашу реализацию, если интересно, на эту тему можно подискутировать.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Я думаю, что можно оставить этот enum в коде, но переместить его в более подходящее место.
У такого подхода есть свои плюсы:
- Впоследствии этот enum может быть полезен и в других классах (как вы и написали в Vector2Int или Vector3 )
- Реализация без этого enum выглядит достаточно топорно и не очень красиво. В данном случае можно создать отдельный метод под каждый из вариантов сравнения,
либо сделать тоже самое, но оставить CompareTo для сравнения по длине, и последний вариант - сделать нечто подобное:
public int CompareTo<T>(T other, Func<Vector2, T, int> comparisonFunction)
{
return comparisonFunction(this, other);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Мне все же кажется что лаконичнее вызывать vector.X.CompareTo(vlaue);
чем использовать большой енам
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Мне все же кажется что лаконичнее вызывать
vector.X.CompareTo(vlaue);
чем использовать большой енам
Добавил реализацию, надеюсь верно Вас понял
Вообще реализация |
Да, мне будет интересно помочь Вам с этой задачей |
Готово: #56 |
Hypercube.Math/Vectors/Vector2.cs
Outdated
} | ||
} | ||
|
||
public enum ComparisonType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Мне все же кажется что лаконичнее вызывать vector.X.CompareTo(vlaue);
чем использовать большой енам
Спасибо ;3 |
Fixes #44