A departing programmer left me a mess to deal with.

[spiritedDedication]

A departing programmer has written the following function that you will be expected to maintain.

The function's comment and signature are correct, but the implementation is not.

Fix all the potential bugs and programming errors, and refactor any bad practices that you can find in the function below. Use comments to explain your reasoning.


The errors I caught:
1. index in for loop needed to be unsigned
2. CToP was reducing 'C' by player, and i switched it to have player be reduced by 'C'
3. I added abs() to CToP to make sure value is positive

Are there any other bad practices I am not catching?

  class Character
{
public:
	Vec3D Location;
	float MoveSpeed;
};

// Finds the Character that could reach the player's current location soonest
Character* FindCharacterToFirstReachPlayer(std::vector<Character*>& CharacterList, Character* Player)
{
	// Find the closest character
	Character* Closest; 
	float MinDistance = 10000.0f;

	// index needs to be unsigned
	for (unsigned int i = 0; i < CharacterList.size(); i++)
	{
		Character* C = CharacterList.at(i);
		// Player location needs to be reduced by character location to find difference
                // before it was C->Location - Player->Location and I switched it.
		Vec3D CToP = Player->Location - C->Location;
		
		// Need to make sure difference will be a positive number
                // This is returning errors, im not sure why.
		CToP.x = std::abs(CToP.x);
		CToP.y = std::abs(CToP.y);
		CToP.z = std::abs(CToP.z);

		float Distance = sqrt(CToP.x * CToP.x + CToP.y * CToP.y + CToP.z * CToP.z);
		if (Distance < MinDistance)
		{
			Closest = C;
		}
	}

	return Closest;
}

[keskiverto]

This is your homework, so I let you think about a couple of things:

1. Let me use your function:

std::vector<Character*> foo;
Character* bar;
Character* gaz = FindCharacterToFirstReachPlayer( foo, bar );
// How does gaz feel now? 

2. "that could reach the player's current location soonest"
Mr Sloth, who moves 10 feet per minute is just 20 feet away, while Flash (the faster than light fellow) is a mile away. Which gets to me first?

3. What is the purpose of the MinDistance?

4. If I'm a mile from Flash, isn't Flash a mile from me? Distance between two points is same both ways.

5. A square of a real value is always positive. There is no "negative distance".

6. What methods does the Vec3D offer?

[jonnin]

what is mindistance doing exactly? The closest character is the closest character.
what happens if all the characters are less than min dist away?
what happens if there are no characters in the list?
closest, returned, is uninitialized, which covers some of those problems... should it be init to NULL?

you don't need abs as the distance is squared which eliminates signs. You could eliminate the square root, but that is an insignificant performance tweak, not an error.



public access to class memebers instead of accessor functions is frowned upon in polite society. I prefer public, but I am sort of a neanderthal.

if the subtraction of vec3d is not behaving, look to fix that class or if not, do the subtraction by hand instead of trusting the operator. If this is a built in type (not sure?) or trusted, look again for a problem, but if you can't find one and the type is defined in the code somewhere, look there...

you don't need abs but for floats your want fabs, not abs.. this matters... (having a moment, it may be 'dabs' or all 3.. I can't remember as I never use it).

[Ihatov]

You should initialize the Closest to nullptr since if CharacterList.size() is zero the function will return a pointer to an undefined location. A recipe for a segfault.

If the CharacterList contains the Player you'll want to check if C != Player.

You might also want to check if Player is NULL

And you really want to initialize the arguments as const since you're not changing any.

I'm not counting the fact that fields in class Character are public:

You can also speed up the algorithm by doing away with sqrt:

float Distance = (CToP.x * CToP.x + CToP.y * CToP.y + CToP.z * CToP.z);
if (Distance < MinDistance * MinDistance)
{
	Closest = C;
}

Although at the cost of precision.

[spiritedDedication]

Thank you all so much for your replies. Definitely see everything I was missing, my fault for not setting up a test to see what I was missing. I wasnt thinking about null checking and double checking for duplicates or other redundancies that I actually created myself in the math.

[TheIdeasMan]

The errors I caught:
1. index in for loop needed to be unsigned

Consider using std::siz_t for anything to do with a container size. This is what all the STL containers use, the size functions return this type. At least save the implicit conversion.

With NULL, use nullptr instead. It was invented to get around problems associated with comparisons to pointers.

Also consider using double rather than float, that latter precision is easily exceeded, particularly if one is squaring with values about 1000. One usually has to take extra precautions if using float.