Reputation: 614
I know, this is probably a very silly question.
I have a long code that repeats itself and uses a lot of copy+paste, I know this is a very bad practice to copy paste the same code but I can't figure out a way to short it! The code I'm struggling to write cleaner is a code that checks around a vector other vectors. I do it by changing the x by adding +1 and -1, then changing the y +1 and -1 and at the end z+1 and z-1, for each slight change I call the same function for the slightly changed vector. This creates a really ugly code:
void checkAirRadius(Voxel temp, Vector3 fireposition)
{
Vector3 original = fireposition;
if (temp.type != null)
{
if (temp.type.isFire != true && temp.type.burnable == true)
{
fireposition.x -= 1;
Voxel temp2 = env.GetVoxel(fireposition);
if (temp2.type == null)
if (temp2.hasContent != 1)
{
env.VoxelDestroy(original);
env.VoxelPlace(fireposition, firevoxel);
coroutine = WaitAndPrint(1.0f, fireposition, 1);
StartCoroutine(coroutine);
}
fireposition.x += 1;
fireposition.x += 1;
temp2 = env.GetVoxel(fireposition);
if (temp2.type == null)
if (temp2.hasContent != 1)
{
env.VoxelDestroy(original);
env.VoxelPlace(fireposition, firevoxel);
coroutine = WaitAndPrint(1.0f, fireposition, 1);
StartCoroutine(coroutine);
}
fireposition.x -= 1;
fireposition.y += 1;
temp2 = env.GetVoxel(fireposition);
if (temp2.type == null)
if (temp2.hasContent != 1)
{
env.VoxelDestroy(original);
env.VoxelPlace(fireposition, firevoxel);
coroutine = WaitAndPrint(1.0f, fireposition, 0);
StartCoroutine(coroutine);
}
fireposition.y -= 1;
fireposition.y -= 1;
temp2 = env.GetVoxel(fireposition);
if (temp2.type == null)
if (temp2.hasContent != 1)
{
env.VoxelDestroy(original);
env.VoxelPlace(fireposition, firevoxel);
coroutine = WaitAndPrint(1.0f, fireposition, 2);
StartCoroutine(coroutine);
}
fireposition.y += 1;
fireposition.z -= 1;
temp2 = env.GetVoxel(fireposition);
if (temp2.type == null)
if (temp2.hasContent != 1)
{
env.VoxelDestroy(original);
env.VoxelPlace(fireposition, firevoxel);
coroutine = WaitAndPrint(1.0f, fireposition, 1);
StartCoroutine(coroutine);
}
fireposition.z += 1;
fireposition.z += 1;
temp2 = env.GetVoxel(fireposition);
if (temp2.type == null)
if (temp2.hasContent != 1)
{
env.VoxelDestroy(original);
env.VoxelPlace(fireposition, firevoxel);
coroutine = WaitAndPrint(1.0f, fireposition, 1);
StartCoroutine(coroutine);
}
}
}
}
void FireUpdate()
{
for (int i = poolFire.Count - 1; i >= 0; i--)
{
if (Random.Range(1, 10) == 5 || VoxelPlayEnvironment.instance.GetVoxel(new Vector3(poolFire[i].x, poolFire[i].y - 1, poolFire[i].z)).type == null)
{
poolFire.RemoveAt(i);
return;
}
else
{
Vector3 fireposition = poolFire[i];
fireposition.x -= 1;
Voxel temp = VoxelPlayEnvironment.instance.GetVoxel(fireposition);
checkAirRadius(temp, fireposition);
fireposition.x += 1;
fireposition.x += 1;
temp = VoxelPlayEnvironment.instance.GetVoxel(fireposition);
checkAirRadius(temp, fireposition);
fireposition.x -= 1;
fireposition.y += 1;
temp = VoxelPlayEnvironment.instance.GetVoxel(fireposition);
checkAirRadius(temp, fireposition);
fireposition.y -= 1;
temp = VoxelPlayEnvironment.instance.GetVoxel(fireposition);
checkAirRadius(temp, fireposition);
fireposition.y -= 1;
temp = VoxelPlayEnvironment.instance.GetVoxel(fireposition);
checkAirRadius(temp, fireposition);
fireposition.z += 1;
temp = VoxelPlayEnvironment.instance.GetVoxel(fireposition);
checkAirRadius(temp, fireposition);
fireposition.z -= 2;
temp = VoxelPlayEnvironment.instance.GetVoxel(fireposition);
checkAirRadius(temp, fireposition);
}
}
I wonder if there is a simple function I can create that will take one vector and call functions for around that block without having to manually change x, y,z by 2 points every time.
Upvotes: 0
Views: 96
Reputation: 629
Following to the DRY (don't repeat yourself) principle, every time you see a code that's being reused, you can, and should, refactor it.
As for your code, the first thing I'd do is create a function that receives the new fireposition
. Let's say for example:
void ChangeFirePositionAndRecalculate (int newValue) {
fireposition.z += newValue;
temp = VoxelPlayEnvironment.instance.GetVoxel(fireposition);
checkAirRadius(temp, fireposition);
}
Instead of calling:
fireposition.z -= 2;
temp = VoxelPlayEnvironment.instance.GetVoxel(fireposition);
checkAirRadius(temp, fireposition);
You can call:
ChangeFirePositionAndRecalculate (-2);
ChangeFirePositionAndRecalculate (1);
But, then again, DRY. You can, for example, create an array, with all these positions:
public int [] positions;
This will even allow you to change the fireposition
directly from the editor. And with that in mind, you can now run:
for (int i = 0; i < position.lenght; i++) {
ChangeFirePositionAndRecalculate (positions [i]);
}
And you can figure the rest. This is just some things I'd do.
[EDIT] I forgot to mention that when creating methods, use CamelCase, I've noticed that some of your methods use but checkAirRadius
doesn't.
Upvotes: 4