# Possible Refactor

4.3 Mono

### Question

I wrote this function that takes in the position of a room and finds the closest position that aligns it to a tilemap, however since the width and height logic is so similar I thought about reducing the scope of the function to only look at the x and y one at a time and just call it twice with different parameters but that doesnt seem very efficient either. Any suggestions to improve the efficiency of this code…

``````public static Vector2I GridSnap(Vector2I position, int width, int height, int cellSize)
{
int newX = int.MinValue;
int newY = int.MinValue;

if (width / cellSize % 2 != 0) {
for (int upX = position.X; upX < position.X + cellSize; upX++) {
if (upX % (cellSize / 2) == 0 && upX % cellSize != 0) {
newX = upX;
break;
}
}
for (int downX = position.X; downX > position.X - cellSize; downX--) {
if (downX % (cellSize / 2) == 0 && downX % cellSize != 0) {
if (newX - position.X > position.X - downX) {
newX = downX;
break;
}
}
}
}
else {
for (int upX = position.X; upX < position.X + cellSize; upX++) {
if (upX % cellSize == 0) {
newX = upX;
break;
}
}
for (int downX = position.X; downX > position.X - cellSize; downX--) {
if (downX % cellSize == 0) {
if (newX - position.X > position.X - downX) {
newX = downX;
break;
}
}
}
}
if (height / cellSize % 2 != 0) {
for (int upY = position.Y; upY < position.Y + cellSize; upY++) {
if (upY % (cellSize / 2) == 0 && upY % cellSize != 0) {
newY = upY;
break;
}
}
for (int downY = position.Y; downY > position.Y - cellSize; downY--) {
if (downY % (cellSize / 2) == 0 && downY % cellSize != 0) {
if (position.Y - newY > downY - position.Y) {
newY = downY;
break;
}
}
}
}
else {
for (int upY = position.Y; upY < position.Y + cellSize; upY++) {
if (upY % cellSize == 0) {
newY = upY;
break;
}
}
for (int downY = position.Y; downY > position.Y - cellSize; downY--) {
if (downY % cellSize == 0 ) {
if (newY - position.Y > position.Y - downY) {
newY = downY;
break;
}
}
}
}
return new Vector2I(newX,newY);
}
``````
1 Like

I am pretty confident you can reduce your algorithm down to a truncated or rounded division and multiplication.

``````int newX = Mathf.RoundToInt(position.X / cellSize) * cellSize;
int newY = Mathf.RoundToInt(position.Y / cellSize) * cellSize;
``````

If you want to keep your for-loop logic I would make use of boolean math to reduce the divided if statements.

``````public static Vector2I GridSnap(Vector2I position, int width, int height, int cellSize)
{
int newX = int.MinValue;
int newY = int.MinValue;

bool width_odd = (width / cellSize) % 2 != 0;
bool height_odd = (width / cellSize) % 2 != 0;

// it will always find a downX value, so upX is overwritten completely. remove this loop
//for (int upX = position.X; upX < position.X + cellSize; upX++) {
//if (upX % (cellSize / 2) == 0 && upX % cellSize != 0) {
//newX = upX;
//break;
//}
//}
for (int downX = position.X; downX > position.X - cellSize; downX--) {
bool divisable = downX % cellSize == 0;
bool half_divisable = downX % (cellSize/2) == 0;

if (divisible != width_odd and (!width_odd or half_divisible))
if (newX - position.X > position.X - downX) {
newX = downX;
break;
}
}
}

for (int downY = position.Y; downY > position.Y - cellSize; downY--) {
bool divisable = downY % cellSize == 0;
bool half_divisable = downY % (cellSize/2) == 0;

if (divisable != height_odd and (!height_odd or half_divisable)) {
if (position.Y - newY > downY - position.Y) {
newY = downY;
break;
}
}
}
return new Vector2I(newX,newY);
}
``````
1 Like

It will always find an up and down value i just need both to see which one is closer to the original position and without newX being set in the up loop the logic in the downloop doesnt work

1 Like

Ah then definitely try using the rounding functions I mentioned.

1 Like